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)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dholbert, Assigned: padenot)

References

Details

Attachments

(1 file, 3 obsolete files)

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
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.
Implements dholbert solution.
Attachment #709704 - Flags: review?(khuey)
Assignee: nobody → paul
Need a semicolon after "return 0". :) (my typo, from comment 1)
dholbert, good catch, thanks.
Attachment #709733 - Flags: review?(khuey)
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
Oh, right -- the strategy in the current patch (from comment 1) just adds an early-return for non-CPUID before the __get_cpuid() call -- but the *compiler* still sees the __get_cpuid() call, and will choke. The #if-wrapping strategies in comment 5 both seem to make sense.
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)
Attached patch updated patch as per comment 5 comments. (obsolete) (deleted) — Splinter Review
Attachment #709733 - Attachment is obsolete: true
Attachment #720646 - Flags: review?(khuey)
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.
Attachment #720646 - Attachment is obsolete: true
Attachment #720646 - Flags: review?(khuey)
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.

Attachment

General

Created:
Updated:
Size: