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)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(2 files, 3 obsolete files)

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?
Something i don't understand here. If clang has this warning to avoid breakages with MSVC++, how come you didn't break windows?
And FAIL_ON_WARNINGS applies only to clang and gcc, right?  So the warning is fatal on clang but not VC.
(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?
FWIW I think this warning is pretty useless and I'm in favor of turning it off.
Attached patch Patch (obsolete) — Splinter Review
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
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #651305 - Flags: review?(ehsan)
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 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)
Attached patch Patch v2 (obsolete) — Splinter Review
Try: https://tbpl.mozilla.org/?tree=Try&rev=a1d66f89d84c
Attachment #651305 - Attachment is obsolete: true
Attachment #651763 - Flags: review?(mh+mozilla)
Comment on attachment 651763 [details] [diff] [review]
Patch v2

The patch is empty.
Attachment #651763 - Flags: review?(mh+mozilla) → review-
Attached patch Patch v3Splinter Review
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 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+
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...
Hmm?  Yes -- the only changes are compiler warning changes:

https://hg.mozilla.org/try/rev/2b597eafdc14
(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?
(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.
(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!
Sorry, was looking at the wrong push. Relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8e68d8d6d9
https://hg.mozilla.org/mozilla-central/rev/3a8e68d8d6d9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #717196 - Flags: review?(mh+mozilla)
Sent to try server on top of the changeset in comment 26.
(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
(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 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-
Attachment #717196 - Attachment is obsolete: true
Attachment #717919 - Flags: review?(mh+mozilla)
Attachment #717919 - Flags: review?(mh+mozilla) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/72ede8c87fed
Status: REOPENED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
(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. :(
Depends on: 1026535
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: