Closed
Bug 836824
Opened 12 years ago
Closed 12 years ago
libsoundtouch fails to build due to __get_cpuid() invocation, if you don't have xcode command line tools
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: dholbert, Assigned: padenot)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
As noted in bug 812667 comment 7, cpu_detect_x86.cpp (in libsoundtouch) fails to compile on Mac OS X configurations w/out XCode command-line tools installed, because we go ahead and invoke __get_cpuid, even though we've already noticed that cpuid.h isn't available.
Presumably we need to guard that __get_cpuid call with a "defined(HAVE_CPUID_H)" check... but I'm not sure if we want the non-cpuid.h systems to fall into the Windows-specific "else" clause there, or if they'll need another fallback condition.
MXR link:
https://mxr.mozilla.org/mozilla-central/source/media/libsoundtouch/src/cpu_detect_x86.cpp?mark=104-129#104
Reporter | ||
Comment 1•12 years ago
|
||
Based on these lines:
> 108 // Check if no cpuid support.
> 109 if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx)) return 0; // always disable extensions.
it looks like maybe we should just fix this with something like:
#if !defined(HAVE_CPUID_H)
// No cpuid.h --> no cpuid support
return 0
#endif
inside the __GNUC__ clause in comment 0's MXR link.
Assignee | ||
Comment 2•12 years ago
|
||
Implements dholbert solution.
Attachment #709704 -
Flags: review?(khuey)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → paul
Assignee | ||
Comment 4•12 years ago
|
||
dholbert, good catch, thanks.
Attachment #709733 -
Flags: review?(khuey)
Assignee | ||
Updated•12 years ago
|
Attachment #709704 -
Attachment is obsolete: true
Attachment #709704 -
Flags: review?(khuey)
Comment on attachment 709733 [details] [diff] [review]
libsoundtouch fails to build due to __get_cpuid() invocation, if you don't have xcode command line tools. r=
Review of attachment 709733 [details] [diff] [review]:
-----------------------------------------------------------------
Doesn't help on FreeBSD 9.0R i386 which ships with clang 3.0 and no cpuid.h:
media/libsoundtouch/src/cpu_detect_x86.cpp:113:10: error: use of undeclared identifier '__get_cpuid'
if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx)) return 0; // always disable extensions.
^
1 error generated.
::: media/libsoundtouch/src/cpu_detect_x86.cpp
@@ +103,5 @@
>
> #if defined(__GNUC__)
> +#if !defined(HAVE_CPUID_H)
> + // No cpuid.h --> no cpuid support
> + return 0;
The following works
#if defined(__GNUC__) && defined(HAVE_CPUID_H)
// GCC version of cpuid
...
#elif defined(__GNUC__)
// No cpuid.h --> no cpuid support
return 0;
#else
// Window / VS version of cpuid
...
#endif
but may be easier to read as
#if !defined(__GNUC__)
// Window / VS version of cpuid
...
#elif defined(HAVE_CPUID_H)
// GCC version of cpuid
...
#else
// Compatible with GCC but no cpuid.h ?
return 0;
#endif
Reporter | ||
Comment 6•12 years ago
|
||
Comment on attachment 709733 [details] [diff] [review]
libsoundtouch fails to build due to __get_cpuid() invocation, if you don't have xcode command line tools. r=
So per comment 5 this doesn't actually work? Ask for review again if that's not the case.
Attachment #709733 -
Flags: review?(khuey)
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #709733 -
Attachment is obsolete: true
Attachment #720646 -
Flags: review?(khuey)
Reporter | ||
Comment 9•12 years ago
|
||
Comment on attachment 720646 [details] [diff] [review]
updated patch as per comment 5 comments.
>diff --git a/media/libsoundtouch/moz-libsoundtouch.patch b/media/libsoundtouch/moz-libsoundtouch.patch
[...]
>++ // Compatible with GCC but no cpuid.h ?
>++ return 0;
>+ #endif
>diff --git a/media/libsoundtouch/src/cpu_detect_x86.cpp b/media/libsoundtouch/src/cpu_detect_x86.cpp
>--- a/media/libsoundtouch/src/cpu_detect_x86.cpp
>+ // Compatible with GCC but no cpuid.h ?
>+ return 0;
> #endif
Nit: Maybe drop the question mark at the end of that comment-sentence -- to me, it's saying "This would be ridiculous -- maybe it doesn't even really happen?", but we know it does happen.
Assignee | ||
Updated•12 years ago
|
Attachment #720646 -
Attachment is obsolete: true
Attachment #720646 -
Flags: review?(khuey)
Attachment #720702 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 11•12 years ago
|
||
Comment 12•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•