Closed
Bug 719659
Opened 12 years ago
Closed 11 years ago
compiling testABI.c gives warning: "Assertions.h:195:33: warning: anonymous variadic macros were introduced in C99 [-Wvariadic-macros]"
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: luke, Assigned: Waldo)
References
Details
Attachments
(1 file, 1 obsolete file)
1.64 KB,
patch
|
espindola
:
review+
|
Details | Diff | Splinter Review |
Waldo knows what to do.
Assignee | ||
Comment 1•12 years ago
|
||
We could turn off the warning using -Wno-variadic-macros, of course. But it seems better to opt into C99 mode instead. We're using increasingly more C99 features these days, and having to add an opt-out for every one of them will be a pain in the long run. Incidentally, I happened to notice that before this patch, qcms uses a variety of C99 features that trigger warnings with gcc. After this patch, those warnings disappear. So using -std=gnu99 is even more clearly the right thing to do for the benefits it provides to other code that already warns about C99 features. Of course, the addition of this flag doesn't mean we can use any more C99 than we could before. We can still only use what MSVC happens to support as extensions. But it does mean we can use compatible features with no extra effort to silence new compiler warnings.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #590074 -
Flags: review?(respindola)
I agree that using -std=? + warnings for things we don't want is probably the way to go. Clang defaults to c99 already, but it is known that the headers on the ancient build bots we use require gnu89 inline semantics. Don't you have to update both configure.in and js/src/configure.in? Can you put that in a .m4? Last but not least, can you try using "-std=c99 -fgnu89-inline"? I think I misunderstood you yesterday and didn't realize that you wanted to change the default we use :-)
Assignee | ||
Comment 3•12 years ago
|
||
This seems to pass try. Note that it's -std=gnu99 -fgnu89-inline, not -std=c99 -fgnu89-inline: -std=c99 sets __STRICT_ANSI__, and that has highly deleterious effects on macros defined by various system headers, on all gcc-style platforms and compilers. I tried to track through the -E output to figure out what was happening but didn't make particular progress on it (it looked like headers were now compiling in BSD mode (!)), and in the end it's not super-important, so I gave up on trying.
Attachment #590074 -
Attachment is obsolete: true
Attachment #590074 -
Flags: review?(respindola)
Attachment #590454 -
Flags: review?(respindola)
Assignee | ||
Comment 4•12 years ago
|
||
Oh: I did update both configure.in and js/src/configure.in. I'm not aware of a single m4 file centralizing commonalities between the two, especially not for this particular bit of stuff.
Comment on attachment 590454 [details] [diff] [review] Patch v2 There is no one .m4 file. We have some in build/autoconf. You would have to add one in there defining a macro and use the macro in both places. The advantage is that we enforce that build/autoconf and js/src/build/autoconf are in sync. Up to you if you want to add a new .m4 file or not.
Attachment #590454 -
Flags: review?(respindola) → review+
Assignee | ||
Comment 6•12 years ago
|
||
There are certain kinds of cleanups/consolidations I'm willing to do. Build system stuff isn't really my strong suit, tho, so I prefer leaving that sort of cleanup to someone else. As far as syncing goes, there's way too much already that's not in sync to specifically worry about exacerbating the confusion factor with this specific change. :-\ https://hg.mozilla.org/integration/mozilla-inbound/rev/886f040f2844
Target Milestone: --- → mozilla12
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/886f040f2844
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 8•12 years ago
|
||
Per espindola we ended up backing this out, his preference was for the warning in js rather than test against gcc 4.1's flag support. https://hg.mozilla.org/mozilla-central/rev/1a08877de7ed https://hg.mozilla.org/mozilla-central/rev/cdf89e1937eb (merge cset)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Justin Wood (:Callek) from comment #8) > Per espindola we ended up backing this out, his preference was for the > warning in js rather than test against gcc 4.1's flag support. Let me expand that my preference would be for us to just fix seamonkey to use a supported compiler. As I noted on that bug, it is really sad that a side project can harm us in this way.
BTW, I will check this back in immoderately after the release. Seamonkey will have a full release cycle to switch to a supported compiler.
Comment 11•12 years ago
|
||
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #10) > BTW, I will check this back in immoderately after the release. Seamonkey > will have a full release cycle to switch to a supported compiler. Again, this did not break *just* SeaMonkey, and the gcc-4.1 is not the only issue here (see the libffi breakage). Something which causes these kinds of problems needs to be backed out and those problems fixed before it lands again. I did say that SeaMonkey can fix the mozconfig in l10n issue during next cycle, so once the uplift happens, this is no longer blocked on SeaMonkey, but is blocked on *still fixing the regressions* for Firefox
Firefox doesn't use gcc 4.1
Comment 13•12 years ago
|
||
seamonkey uses gcc 4.5. In fact, it was using 4.5 before firefox.
Comment 14•12 years ago
|
||
The openbsd people are stuck on 4.1, though.
Comment 15•12 years ago
|
||
4.2.1, to be more precise.
Assignee | ||
Comment 16•12 years ago
|
||
4.2.1 shouldn't have problems; from what I could tell in a quick web search, -fgnu89-inline was first present in gcc 4.2. So I guess I'm off to file a releng bug to get l10n tinderboxen using newer compilers, then...
Comment 17•12 years ago
|
||
And even if FreeBSD now tries more and more to use clang (i don't know what's their progress so far), they are also stuck with 4.2.1. To be sure, building m-c with the patch now..
Assignee | ||
Comment 18•12 years ago
|
||
I filed bug 721837 for releng to update the systems in whatever way they should be updated.
Depends on: 721837
Comment 19•12 years ago
|
||
Fwiw, the patch adding -std=gnu99 -fgnu89-inline caused no fallout on OpenBSD/Gcc 4.2.1, although i didn't see those flags being applied everywhere in the build output (for example not under js/src, even with the js/src/configure.in hunk applied)
Assignee | ||
Comment 20•12 years ago
|
||
The patch only affected C compilation units, and JS only has a couple files in that category now. Look at the output for testABI.c's compilation, which definitely would have had the flags in it.
Can this land again or do we still block on someone using gcc 4.1?
Assignee | ||
Comment 22•12 years ago
|
||
It appears the l10n repack machines still use gcc 4.1, if I interpreted the log I linked in bug 721837 (just reopened) correctly. :-\
Assignee | ||
Comment 23•11 years ago
|
||
Now that we require 4.2 to build, and looking at an l10n repack log in bug 721837 suggests they've been updated for double-safety, I think we might be good with the -std=gnu99 patch again. Poked espindola on IRC, he's good letting the previous review hold still, so repushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/93822863127e If this sticks a few days and nobody complains due to infrastructury oldness, I think we're good here. Although, espindola did suggest the infrastructury requirement for -fgnu89-inline may no longer hold any more, so possibly that bit could be removed too. But one thing at a time! (Amusingly enough, now that SpiderMonkey has a C++ API, the original issue here no longer exists. But there are plenty of other C files in our tree that use C99isms, so it's still worth it.)
Target Milestone: mozilla12 → mozilla21
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/93822863127e
Status: REOPENED → RESOLVED
Closed: 12 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•