Closed
Bug 780474
Opened 12 years ago
Closed 11 years ago
Enable -Wno-mismatched-tags for clang, or at least -Wno-error=mismatched-tags
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla17
People
(Reporter: ayg, Assigned: ayg)
References
Details
Attachments
(2 files, 3 obsolete files)
2.42 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
2.30 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
I just had a try run for bug 780469 fail to build on 10.7 because I used "class" in a forward declaration where the definition used "struct". This works in all compilers, and is perfectly valid per spec -- but clang warns for it on -Wall, and the relevant dir apparently had -Werror enabled. The warning seems pointless to me -- it doesn't change behavior and it's not confusing -- so I'd say we should disable it entirely. Failing that, we should *definitely* make sure it's -Wno-error, because gcc doesn't support it, and there is no reason at all to break Clang builds over it if the patch was tested in gcc. Ehsan, what do you think?
Comment 1•12 years ago
|
||
Something i don't understand here. If clang has this warning to avoid breakages with MSVC++, how come you didn't break windows?
It's only a warning on Windows.
Assignee | ||
Comment 3•12 years ago
|
||
And FAIL_ON_WARNINGS applies only to clang and gcc, right? So the warning is fatal on clang but not VC.
Comment 4•12 years ago
|
||
(In reply to comment #1) > Something i don't understand here. If clang has this warning to avoid breakages > with MSVC++, how come you didn't break windows? What is the MSVC breakage that you're talking about here?
Comment 5•12 years ago
|
||
FWIW I think this warning is pretty useless and I'm in favor of turning it off.
Assignee | ||
Comment 6•12 years ago
|
||
I haven't a clue if this patch is correct -- I don't normally use clang to compile. Now that we use clang on some of the tryservers, though, I can look at the logs to see if any warnings were emitted. Try: https://tbpl.mozilla.org/?tree=Try&rev=801a7e52b9ef
Comment on attachment 651305 [details] [diff] [review] Patch Please put it in build/autoconf/compiler-opts.m4 so that we get the same behavior in js.
Attachment #651305 -
Flags: feedback-
Comment 8•12 years ago
|
||
Comment on attachment 651305 [details] [diff] [review] Patch Review of attachment 651305 [details] [diff] [review]: ----------------------------------------------------------------- What Rafael said, and then you need to ask for review from a build system peer, like glandium, but this basically looks sane to me.
Attachment #651305 -
Flags: review?(ehsan)
Assignee | ||
Comment 10•12 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=a1d66f89d84c
Attachment #651305 -
Attachment is obsolete: true
Attachment #651763 -
Flags: review?(mh+mozilla)
Comment 11•12 years ago
|
||
Comment on attachment 651763 [details] [diff] [review] Patch v2 The patch is empty.
Attachment #651763 -
Flags: review?(mh+mozilla) → review-
Assignee | ||
Comment 12•12 years ago
|
||
Well, that's certainly an r- that's hard to argue with.
Attachment #651763 -
Attachment is obsolete: true
Attachment #651768 -
Flags: review?(mh+mozilla)
Comment 13•12 years ago
|
||
Comment on attachment 651768 [details] [diff] [review] Patch v3 Review of attachment 651768 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/autoconf/compiler-opts.m4 @@ +85,5 @@ > ## returned by C functions. This is possible because we use knowledge about the ABI > ## to typedef it to a C type with the same layout when the headers are included > ## from C. > + ## > + ## -Wno-mismatched-tags: Useless, not supported in gcc (bug 780474) Changing the order would make the comment more useful imho, like "Useless (see bug 780474), but not supported in gcc."
Attachment #651768 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 14•12 years ago
|
||
I'm getting very odd errors on the try run: https://tbpl.mozilla.org/?tree=Try&rev=a1d66f89d84c For all the OS X debug builds, in mochitest-other, I consistently get crashes beginning with Assertion failure: throwing, at ../../../js/src/jscntxt.h:1380 in browser_aboutCrashesResubmit.js. I have no idea how a change to compiler warning options could cause an assert failure, given that it compiled. Any ideas? I'm CCing the author/reviews of the patch that added the assert. The parent revision has a green try run, so it really is something in this changeset that does it: https://hg.mozilla.org/try/rev/6b74cfd3ff23
(In reply to :Aryeh Gregor from comment #14) > I'm getting very odd errors on the try run: > > https://tbpl.mozilla.org/?tree=Try&rev=a1d66f89d84c Sorry, is this the right patch? I don't see compiler waring changes in it...
Assignee | ||
Comment 16•12 years ago
|
||
Hmm? Yes -- the only changes are compiler warning changes: https://hg.mozilla.org/try/rev/2b597eafdc14
Assignee | ||
Comment 17•12 years ago
|
||
(In reply to :Aryeh Gregor from comment #14) > The parent revision has a green try run, so it really is something in this > changeset that does it: https://hg.mozilla.org/try/rev/6b74cfd3ff23 I meant to link to the try run here, not the revision: https://tbpl.mozilla.org/?tree=Try&rev=6b74cfd3ff23
I tried to reproduce this locally on 10.8, but could not. It is interesting to note that I have no assert in jscntxt.h:1380 if I apply your patch on top of m-i's 782252, which is really strange. Are you sure that was really the only change pushed? Can you try pushing again on top of a clean m-i or m-c?
Assignee | ||
Comment 19•12 years ago
|
||
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #18) > Are you sure that was really the only change pushed? Yep, although I wouldn't be entirely surprised if it turns out to be a phantom problem anyway. > Can you try pushing again on top of a clean m-i or m-c? https://tbpl.mozilla.org/?tree=Try&rev=15f1ff56ea29
> https://tbpl.mozilla.org/?tree=Try&rev=15f1ff56ea29
It is looking good so far. I have requested more mochitest-other runs to be sure.
Comment 21•12 years ago
|
||
(In reply to comment #20) > > https://tbpl.mozilla.org/?tree=Try&rev=15f1ff56ea29 > > It is looking good so far. I have requested more mochitest-other runs to be > sure. Yep, it's good!
Assignee | ||
Comment 22•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f16429aa2210
Flags: in-testsuite-
Comment 23•12 years ago
|
||
Backed out for Android builds failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=19ec214f806c https://hg.mozilla.org/integration/mozilla-inbound/rev/22ce0a41fe21
Comment 24•12 years ago
|
||
Sorry, was looking at the wrong push. Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8e68d8d6d9
Comment 25•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3a8e68d8d6d9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 26•11 years ago
|
||
(In reply to :Aryeh Gregor from comment #3) > And FAIL_ON_WARNINGS applies only to clang and gcc, right? So the warning > is fatal on clang but not VC. So it turns out that we have started to make Windows fail-on-warnings, so this actually breaks people's builds now. Should I just back out the patch? (For reference: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=083e6703ed56)
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 27•11 years ago
|
||
Ideally I'd say we should suppress this warning on Windows too, since it's not actually a problem and we can't replicate the warning on gcc (right?). If that can't be done, then yeah, this should be backed out. At least that way clang users will catch the problem before they push, even if gcc users won't.
Comment 28•11 years ago
|
||
Attachment #717196 -
Flags: review?(mh+mozilla)
Comment 29•11 years ago
|
||
Sent to try server on top of the changeset in comment 26.
Comment 30•11 years ago
|
||
(In reply to comment #29) > Sent to try server on top of the changeset in comment 26. Erm, this was supposed to include the link! https://tbpl.mozilla.org/?tree=Try&rev=139280d1999d
Comment 31•11 years ago
|
||
(In reply to :Ehsan Akhgari from comment #30) > (In reply to comment #29) > > Sent to try server on top of the changeset in comment 26. > > Erm, this was supposed to include the link! > https://tbpl.mozilla.org/?tree=Try&rev=139280d1999d And that's evidence enough that this patch does the trick!
Comment 32•11 years ago
|
||
Comment on attachment 717196 [details] [diff] [review] Disable the warning on Windows too Review of attachment 717196 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/autoconf/compiler-opts.m4 @@ +94,5 @@ > _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags" > fi > > +case "$target" in > +*-mingw*) This matches building with mingw as well as msvc, you need to add a test -z "$GNU_CC"
Attachment #717196 -
Flags: review?(mh+mozilla) → review-
Comment 33•11 years ago
|
||
Attachment #717196 -
Attachment is obsolete: true
Attachment #717919 -
Flags: review?(mh+mozilla)
Updated•11 years ago
|
Attachment #717919 -
Flags: review?(mh+mozilla) → review+
Comment 35•11 years ago
|
||
(In reply to Ehsan Akhgari from comment #4) > (In reply to Mike Hommey from comment #1) > > Something i don't understand here. If clang has this warning to avoid breakages > > with MSVC++, how come you didn't break windows? > > What is the MSVC breakage that you're talking about here? Old versions of VC used to mangle classes and structs differently, but current versions don't. I couldn't find out when Microsoft fixed their compiler.
Comment 36•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/72ede8c87fed
Status: REOPENED → RESOLVED
Closed: 12 years ago → 11 years ago
Resolution: --- → FIXED
Comment 37•10 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #35) > (In reply to Ehsan Akhgari from comment #4) > > (In reply to Mike Hommey from comment #1) > > > Something i don't understand here. If clang has this warning to avoid breakages > > > with MSVC++, how come you didn't break windows? > > > > What is the MSVC breakage that you're talking about here? > > Old versions of VC used to mangle classes and structs differently, but > current versions don't. I couldn't find out when Microsoft fixed their > compiler. Turns out that this is false, and I made a huge mistake here. :(
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•