Last Comment Bug 780474 - Enable -Wno-mismatched-tags for clang, or at least -Wno-error=mismatched-tags
: Enable -Wno-mismatched-tags for clang, or at least -Wno-error=mismatched-tags
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
:
: Gregory Szorc [:gps]
Mentors:
: 764255 (view as bug list)
Depends on: 1026535
Blocks: 764255
  Show dependency treegraph
 
Reported: 2012-08-05 08:45 PDT by Aryeh Gregor (:ayg) (next working March 28-April 26)
Modified: 2014-06-18 17:35 PDT (History)
8 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.19 KB, patch)
2012-08-13 02:43 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
respindola: feedback-
Details | Diff | Splinter Review
Patch v2 (222 bytes, patch)
2012-08-14 07:47 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
mh+mozilla: review-
Details | Diff | Splinter Review
Patch v3 (2.42 KB, patch)
2012-08-14 07:57 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
mh+mozilla: review+
Details | Diff | Splinter Review
Disable the warning on Windows too (2.19 KB, patch)
2013-02-22 10:30 PST, :Ehsan Akhgari
mh+mozilla: review-
Details | Diff | Splinter Review
Disable the warning on Windows too (2.30 KB, patch)
2013-02-25 09:11 PST, :Ehsan Akhgari
mh+mozilla: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-05 08:45:21 PDT
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 Mike Hommey [:glandium] 2012-08-05 08:47:00 PDT
Something i don't understand here. If clang has this warning to avoid breakages with MSVC++, how come you didn't break windows?
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-08-05 08:47:29 PDT
It's only a warning on Windows.
Comment 3 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-05 08:55:25 PDT
And FAIL_ON_WARNINGS applies only to clang and gcc, right?  So the warning is fatal on clang but not VC.
Comment 4 :Ehsan Akhgari 2012-08-09 14:34:15 PDT
(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 :Ehsan Akhgari 2012-08-09 14:34:40 PDT
FWIW I think this warning is pretty useless and I'm in favor of turning it off.
Comment 6 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-13 02:43:31 PDT
Created attachment 651305 [details] [diff] [review]
Patch

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 7 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-08-13 07:03:09 PDT
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.
Comment 8 :Ehsan Akhgari 2012-08-13 09:48:20 PDT
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.
Comment 9 :Ehsan Akhgari 2012-08-14 07:10:26 PDT
*** Bug 764255 has been marked as a duplicate of this bug. ***
Comment 10 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-14 07:47:49 PDT
Created attachment 651763 [details] [diff] [review]
Patch v2

Try: https://tbpl.mozilla.org/?tree=Try&rev=a1d66f89d84c
Comment 11 Mike Hommey [:glandium] 2012-08-14 07:49:48 PDT
Comment on attachment 651763 [details] [diff] [review]
Patch v2

The patch is empty.
Comment 12 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-14 07:57:19 PDT
Created attachment 651768 [details] [diff] [review]
Patch v3

Well, that's certainly an r- that's hard to argue with.
Comment 13 Mike Hommey [:glandium] 2012-08-14 08:00:15 PDT
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."
Comment 14 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-15 04:43:41 PDT
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
Comment 15 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-08-15 06:08:39 PDT
(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...
Comment 16 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-15 07:25:49 PDT
Hmm?  Yes -- the only changes are compiler warning changes:

https://hg.mozilla.org/try/rev/2b597eafdc14
Comment 17 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-15 07:38:33 PDT
(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
Comment 18 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-08-15 11:14:05 PDT
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?
Comment 19 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-16 05:06:10 PDT
(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
Comment 20 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-08-16 07:17:04 PDT
> 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 :Ehsan Akhgari 2012-08-16 10:03:36 PDT
(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!
Comment 22 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-17 03:29:07 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f16429aa2210
Comment 24 Ed Morley [:emorley] 2012-08-17 04:32:16 PDT
Sorry, was looking at the wrong push. Relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8e68d8d6d9
Comment 25 Ryan VanderMeulen [:RyanVM] 2012-08-17 19:23:36 PDT
https://hg.mozilla.org/mozilla-central/rev/3a8e68d8d6d9
Comment 26 :Ehsan Akhgari 2013-02-01 15:39:31 PST
(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)
Comment 27 Aryeh Gregor (:ayg) (next working March 28-April 26) 2013-02-20 04:10:30 PST
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 :Ehsan Akhgari 2013-02-22 10:30:07 PST
Created attachment 717196 [details] [diff] [review]
Disable the warning on Windows too
Comment 29 :Ehsan Akhgari 2013-02-22 10:37:08 PST
Sent to try server on top of the changeset in comment 26.
Comment 30 :Ehsan Akhgari 2013-02-22 11:26:12 PST
(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 :Ehsan Akhgari 2013-02-22 14:01:42 PST
(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 Mike Hommey [:glandium] 2013-02-25 02:38:48 PST
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"
Comment 33 :Ehsan Akhgari 2013-02-25 09:11:45 PST
Created attachment 717919 [details] [diff] [review]
Disable the warning on Windows too
Comment 35 neil@parkwaycc.co.uk 2013-02-25 14:42:31 PST
(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 Ryan VanderMeulen [:RyanVM] 2013-02-25 18:33:17 PST
https://hg.mozilla.org/mozilla-central/rev/72ede8c87fed
Comment 37 :Ehsan Akhgari 2014-06-18 17:32:16 PDT
(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. :(

Note You need to log in before you can comment on or make changes to this bug.