Last Comment Bug 734335 - Build failure because of missing NoBarrier_Store on non-arm non-x86
: Build failure because of missing NoBarrier_Store on non-arm non-x86
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: Trunk
: Other Linux
: -- normal (vote)
: mozilla13
Assigned To: Mike Hommey [:glandium]
:
:
Mentors:
Depends on: 743428
Blocks: 699918 735871 744026
  Show dependency treegraph
 
Reported: 2012-03-08 23:49 PST by Mike Hommey [:glandium]
Modified: 2012-04-10 09:30 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Only build SPS on supported platforms (5.84 KB, patch)
2012-03-09 05:57 PST, Mike Hommey [:glandium]
b56girard: review+
khuey: review+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2012-03-08 23:49:05 PST
tools/profiler/sps/platform.h: In member function 'void Sampler::SetActive(bool)':
tools/profiler/sps/platform.h:271:63: error: 'NoBarrier_Store' was not declared in this scope
Comment 1 Mike Hommey [:glandium] 2012-03-09 00:09:12 PST
There are IMHO two fundamental problems:
- The code is being built on platforms that don't support it.
- The code is using custom definitions of types coming from chromium, when they could be using headers from ipc/chromium/src (like atomicops.h)

I'll address the former.
Comment 2 Mike Hommey [:glandium] 2012-03-09 00:41:03 PST
I don't know if the intent for the --enable-sps was to always be enabled, but that's how it currently is. Is SPS expected to be enabled on all builds? If yes, then it should be a --disable-sps.
Comment 3 Mike Hommey [:glandium] 2012-03-09 05:57:37 PST
Created attachment 604389 [details] [diff] [review]
Only build SPS on supported platforms

This still enables sps by default, assuming this is what was meant, but disable it by default on unsupported platforms. This also makes --disable-sps actually do something, which wasn't the case before.
Comment 4 Benoit Girard (:BenWa) 2012-03-09 06:43:49 PST
Comment on attachment 604389 [details] [diff] [review]
Only build SPS on supported platforms

Looks good. Thanks for the clean up.
Comment 6 Landry Breuil (:gaston) 2012-03-13 02:08:48 PDT
I'm only reading the diff and didnt actually check it on OpenBSD (buildbot will as soon as inbound is merged to central), but isn't the logic reversed ?

From my understanding, by default MOZ_ENABLE_PROFILER_SPS=1 is set/defined, and unset on supported platforms... and --disable-sps unsets it. Shouldn't it be the other way round ?
Comment 7 Mike Hommey [:glandium] 2012-03-13 02:12:51 PDT
(In reply to Landry Breuil from comment #6)
> by default MOZ_ENABLE_PROFILER_SPS=1 is set/defined,
> and unset on supported platforms... and --disable-sps unsets it. Shouldn't
> it be the other way round ?

It defaults to 1 on supported platforms and nothing on unsupported platforms. Then --disable-sps allows to unset it on supported platforms.
Comment 8 Landry Breuil (:gaston) 2012-03-13 02:34:06 PDT
Doh, i see where my confusion comes from... misread the case.

Wouldnt it be simpler/clearer to do :

MOZ_ENABLE_PROFILER_SPS=
case "${OS_TARGET}" in
   Android)
   case "${CPU_ARCH}" in
       x86 | arm) MOZ_ENABLE_PROFILER_SPS=1 ;;
       *) ;;
   esac
   Linux)
   case "${CPU_ARCH}" in
       x86 | x86_64) MOZ_ENABLE_PROFILER_SPS=1 ;;
       *) ;;
   esac
   WINNT|Darwin) MOZ_ENABLE_PROFILER_SPS=1 ;;
   *) ;;
esac   


Or it's a rule of thumb to have somewhat reversed logic in configure.in ?
Comment 9 Marco Bonardo [::mak] 2012-03-13 07:00:09 PDT
https://hg.mozilla.org/mozilla-central/rev/5168ba8c86f0

Note You need to log in before you can comment on or make changes to this bug.