User-specified CFLAGS/CXXFLAGS should be applied at the end of the commandline, not in the middle

RESOLVED FIXED in mozilla26

Status

RESOLVED FIXED
6 years ago
6 months ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla26
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 682051 [details] [diff] [review]
Patch

b2g ends up using CXXFLAGS to include a header, which eventually drags in <stdint.h>.  In bug 730805 I'm trying to define the various __STDC_*_MACROS for all Gecko code, done in mozilla-config.h.  That include is done on the command line -- but it currently is *after* CXXFLAGS.  So when <stdint.h> is included, __STDC_*_MACROS isn't yet defined, so the features hidden behind those ifdefs aren't enabled, which breaks stuff on b2g.  (And only b2g, for what it's worth.)

The fundamental problem is that CXXFLAGS and CFLAGS similarly are being applied in the middle of the commandline -- after some defaults have been specified but before others.  They should instead be applied at the end, so that no matter the semantics CXXFLAGS/CFLAGS specify, they can't prevent defaults from applying correctly.  (The user could change those default semantics, sure, but by that point it's the user's fault if he breaks stuff.)

Try run:

https://tbpl.mozilla.org/?tree=Try&rev=7a439c4efd18
Attachment #682051 - Flags: review?(khuey)
Comment on attachment 682051 [details] [diff] [review]
Patch

Review of attachment 682051 [details] [diff] [review]:
-----------------------------------------------------------------

We should be on the lookout for regressions from fiddling with the order here.
Attachment #682051 - Flags: review?(khuey) → review+
(Assignee)

Comment 2

6 years ago
Definitely.  Bug 730805 is ready to land along with this, but prudence does suggest holding off on landing that just yet, until anything lurking issues from this get smoked out.  So I landed this alone for now, and maybe in a week or so I'll go back to that bug and land it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f8721399b14
Target Milestone: --- → mozilla20

Comment 3

6 years ago
This unfortunately broken Windows PGO builds:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=WINNT%205.2%20mozilla-inbound%20pgo-build&fromchange=0c3ec1f82732&tochange=b44293987106

eg:
{
   Creating library xul.lib and object xul.exp
LINK : warning LNK4098: defaultlib 'LIBCMT' conflicts with use of other libs; use /NODEFAULTLIB:library
Generating code
e:\builds\moz2_slave\m-in-w32-pgo\build\layout\style\nscssparser.cpp(1071) : warning C4748: /GS can not protect parameters and local variables from local buffer overrun because optimizations are disabled in function
Finished generating code
PGOCVT : fatal error PG0001: An unexpected internal error was detected in source file 'f:\dd\vctools\compiler\utc\src\tools\pogo\cvtpgd\cvtpgd.cpp', line 800.
PGOCVT : fatal error PG0001: An unexpected internal error was detected in source file 'f:\dd\vctools\compiler\utc\src\tools\pogo\cvtpgd\cvtpgd.cpp', line 858.
}

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38f5af022bf7
Target Milestone: mozilla20 → ---

Comment 4

6 years ago
Just to add to what philor said in #developers, the error strings in comment 3 are apparently present in all PGO builds (I've filed bug 813989 for this); so the causes of the failure here is due to the crash later on in the build that philor mentioned.
(Assignee)

Updated

6 years ago
Blocks: 818689

Comment 5

6 years ago
Regarding comment 4, if this didn't break PGO, can this go back in?
(Assignee)

Comment 6

5 years ago
I thought the claim was that it *did* break PGO, and so couldn't go back in.

The semi-recent Windows PGO fiddling motivated me to retest this and see if it'd work, a few months ago.  It didn't.  Random other stuff broke, and I didn't have time to investigate the failures.  :-\  So this is in a state of limbo until I can find said time.
(Assignee)

Comment 7

5 years ago
So, on a lark I repushed this to try, and it's significantly greener (and apparent-green-promising) thus far than I remember it being the last time I tried it:

https://tbpl.mozilla.org/?tree=Try&rev=74b327094a5b

Maybe user error in the previous attempt?  I dunno.  Anyway, if this keeps panning out, definitely this is going back in posthaste.  :-)
Do note that Try won't do PGO builds for you unless you force it to.
(Assignee)

Comment 9

5 years ago
Bleh.  Repushed with PGO on to try:

https://tbpl.mozilla.org/?tree=Try&rev=1c2a29db9d88

RyanVM tells me that's green on the right stuff to land, so pushed.  Fingers crossed!

https://hg.mozilla.org/integration/mozilla-inbound/rev/00e7c35894e2
https://hg.mozilla.org/mozilla-central/rev/00e7c35894e2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Updated

6 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.