Closed Bug 719659 Opened 8 years ago Closed 7 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)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: luke, Assigned: Waldo)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Waldo knows what to do.
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 :-)
Attached patch Patch v2Splinter Review
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)
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+
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
https://hg.mozilla.org/mozilla-central/rev/886f040f2844
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 721625
Depends on: 721744
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.
(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
seamonkey uses gcc 4.5. In fact, it was using 4.5 before firefox.
The openbsd people are stuck on 4.1, though.
4.2.1, to be more precise.
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...
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..
I filed bug 721837 for releng to update the systems in whatever way they should be updated.
Depends on: 721837
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)
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?
It appears the l10n repack machines still use gcc 4.1, if I interpreted the log I linked in bug 721837 (just reopened) correctly.  :-\
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
https://hg.mozilla.org/mozilla-central/rev/93822863127e
Status: REOPENED → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → FIXED
Blocks: 1261263
You need to log in before you can comment on or make changes to this bug.