suppress numerous clang-style warnings when using clang-cl

RESOLVED FIXED in Firefox 46

Status

Firefox Build System
General
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks: 1 bug)

unspecified
mozilla46
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Suppressing these warnings significantly cuts down the warning spam on
clang-cl builds.
(Assignee)

Comment 1

3 years ago
Created attachment 8698577 [details] [diff] [review]
suppress numerous clang-style warnings when using clang-cl
Attachment #8698577 - Flags: review?(mh+mozilla)
(Assignee)

Updated

3 years ago
Blocks: 752004, 1042132
(Assignee)

Comment 2

3 years ago
I guess we should add these to js/src/configure.in too...
(Assignee)

Comment 3

3 years ago
Created attachment 8698586 [details] [diff] [review]
suppress numerous clang-style warnings when using clang-cl

Now with js/src/ modifications.
Attachment #8698586 - Flags: review?(mh+mozilla)
(Assignee)

Updated

3 years ago
Attachment #8698577 - Attachment is obsolete: true
Attachment #8698577 - Flags: review?(mh+mozilla)
Comment on attachment 8698586 [details] [diff] [review]
suppress numerous clang-style warnings when using clang-cl

Review of attachment 8698586 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +2287,5 @@
> +            CFLAGS="$CFLAGS -Wno-invalid-noreturn"
> +            CXXFLAGS="$CXXFLAGS -Wno-invalid-noreturn"
> +            # Missing |override| on virtual function declarations isn't
> +            # something that MSVC currently warns about.
> +            CXXFLAGS="$CXXFLAGS -Wno-inconsistent-missing-override"

Is that something we're hitting on system headers or windows platform code? Because IIRC, we're supposed to use override consistently in Gecko code.

@@ +2290,5 @@
> +            # something that MSVC currently warns about.
> +            CXXFLAGS="$CXXFLAGS -Wno-inconsistent-missing-override"
> +            # We declare |operator delete(void*)| differently than MSVC,
> +            # which causes a raft of warnings if we leave this enabled.
> +            CXXFLAGS="$CXXFLAGS -Wno-implicit-exception-spec-mismatch"

Can you provide more detail here, because this might actually be important.
Attachment #8698586 - Flags: review?(mh+mozilla) → feedback+
(Assignee)

Comment 5

3 years ago
(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 8698586 [details] [diff] [review]
> suppress numerous clang-style warnings when using clang-cl
> 
> Review of attachment 8698586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +2287,5 @@
> > +            CFLAGS="$CFLAGS -Wno-invalid-noreturn"
> > +            CXXFLAGS="$CXXFLAGS -Wno-invalid-noreturn"
> > +            # Missing |override| on virtual function declarations isn't
> > +            # something that MSVC currently warns about.
> > +            CXXFLAGS="$CXXFLAGS -Wno-inconsistent-missing-override"
> 
> Is that something we're hitting on system headers or windows platform code?
> Because IIRC, we're supposed to use override consistently in Gecko code.

These appear to be coming from Gecko graphics headers and Gecko accessibility headers.  Some of them are fixable; some of them come from macros defined by the Windows IDL compiler, so are not easily fixable.

We could fix all the warnings, but they're just going to come back, seeing as how nothing is checking for this with our normal Windows builds...

> @@ +2290,5 @@
> > +            # something that MSVC currently warns about.
> > +            CXXFLAGS="$CXXFLAGS -Wno-inconsistent-missing-override"
> > +            # We declare |operator delete(void*)| differently than MSVC,
> > +            # which causes a raft of warnings if we leave this enabled.
> > +            CXXFLAGS="$CXXFLAGS -Wno-implicit-exception-spec-mismatch"
> 
> Can you provide more detail here, because this might actually be important.

The declaration from <new> says:

void __CRTDECL operator delete(void *) _THROW0();

and the warning says:

function previously declared with an explicit exception specification redeclared with an implicit exception specification

Hm.  I think that means our clang isn't turning off exceptions, because <xstddef> only defines THROW0 to something if _HAS_EXCEPTIONS.  Maybe that's not a good thing...
(Assignee)

Comment 6

3 years ago
There's also a lot of inconsistent override spam from webrtc, which is just annoying.
(Assignee)

Comment 7

3 years ago
(In reply to Nathan Froyd [:froydnj] from comment #5)
> > @@ +2290,5 @@
> > > +            # something that MSVC currently warns about.
> > > +            CXXFLAGS="$CXXFLAGS -Wno-inconsistent-missing-override"
> > > +            # We declare |operator delete(void*)| differently than MSVC,
> > > +            # which causes a raft of warnings if we leave this enabled.
> > > +            CXXFLAGS="$CXXFLAGS -Wno-implicit-exception-spec-mismatch"
> > 
> > Can you provide more detail here, because this might actually be important.
> 
> The declaration from <new> says:
> 
> void __CRTDECL operator delete(void *) _THROW0();
> 
> and the warning says:
> 
> function previously declared with an explicit exception specification
> redeclared with an implicit exception specification
> 
> Hm.  I think that means our clang isn't turning off exceptions, because
> <xstddef> only defines THROW0 to something if _HAS_EXCEPTIONS.  Maybe that's
> not a good thing...

I don't completely understand this warning now, because we *are* passing -D_HAS_EXCEPTIONS=0, so there's no explicit exception specification on the function.  Unless clang is defining |operator delete| internally and then complaining that MSVC's headers (not ours) redeclare it improperly?  In which case I don't think there's anything we can do here, unless clang-cl supports -fno-exceptions?

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3d2670d0cc5e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Comment 10

3 years ago
Sigh, apparently mistyped the bug number for a bustage fix.

Mike, WDYT about comment 5 and following?
Status: RESOLVED → REOPENED
Flags: needinfo?(mh+mozilla)
Resolution: FIXED → ---
(In reply to Nathan Froyd [:froydnj] from comment #7)
> I don't completely understand this warning now, because we *are* passing
> -D_HAS_EXCEPTIONS=0, so there's no explicit exception specification on the
> function.  Unless clang is defining |operator delete| internally and then
> complaining that MSVC's headers (not ours) redeclare it improperly?  In
> which case I don't think there's anything we can do here, unless clang-cl
> supports -fno-exceptions?

Could you check what a .i looks like for one of the files where the warning happens?
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 12

3 years ago
(In reply to Mike Hommey [:glandium] from comment #11)
> Could you check what a .i looks like for one of the files where the warning
> happens?

Sure.  delete is declared via <new> as:

|void __cdecl operator delete(void *) ;|

(None of the |delete| or |new| variants have a throwing declaration on them)
Flags: needinfo?(mh+mozilla)
Comment on attachment 8698586 [details] [diff] [review]
suppress numerous clang-style warnings when using clang-cl

Review of attachment 8698586 [details] [diff] [review]:
-----------------------------------------------------------------

Considering comment 12, let's go for it.
Attachment #8698586 - Flags: review+
Flags: needinfo?(mh+mozilla)

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0912f477bd73
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
Resolution: --- → FIXED

Updated

3 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.