Fails to build breakpad with clang-cl because of c++11 narrowing checks

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ting, Assigned: froydnj)

Tracking

Trunk
mozilla53
Unspecified
Windows
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
clang: r284701

Note there're many errors like this in breakpad:

 0:35.73 c:/w/fx/mc/toolkit/crashreporter/google-breakpad/src/processor/minidump_processor.cc(1153,14):  error: case value evaluates to -1073741681, which cannot be narrowed to type 'uint32_t' (aka 'unsigned int') [-Wc++11-narrowing]
 0:35.73         case MD_EXCEPTION_CODE_WIN_FLOAT_INEXACT_RESULT:
 0:35.74              ^
(Assignee)

Comment 1

2 years ago
(In reply to Ting-Yu Chou [:ting] from comment #0)
> clang: r284701

Are you sure this is the right revision?  The commit email I have for that revision says that it concerns initialization, which doesn't seem related to this error message.
(Reporter)

Comment 2

2 years ago
ting@ting-nb MINGW64 /c/w/tools/llvm/src/tools/clang (master)
$ git log -n 1
commit c467cbcfd2dc2dc4e2e943f1268023ce88ba1465
Author: Richard Smith <richard-llvm@metafoo.co.uk>
Date:   Thu Oct 20 07:53:17 2016 +0000

    Work around MSVC rejects-valid. Apparenty (some versions of) MSVC will check
    that a member is default-initializable even if it's initialized by a default
    member initializer.


    git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@284701 91177308-0d34-0410-b5e6-96231b3b80d8
(Assignee)

Comment 3

2 years ago
Ted, can we add local patches to breakpad for things like this?  I think this is as simple as declaring the relevant enums as uint32_t, but haven't actually tried it yet.
Flags: needinfo?(ted)
We try not to take local patches to Breakpad, preferring to instead upstream them. I guess that enum is declared in a file that claims to be C99, so we can't just use an enum class there...

Can we tweak the call sites that use this code to not hit this warning?
Flags: needinfo?(ted)
(Assignee)

Comment 5

2 years ago
(In reply to Ting-Yu Chou [:ting] from comment #2)
> ting@ting-nb MINGW64 /c/w/tools/llvm/src/tools/clang (master)
> $ git log -n 1
> commit c467cbcfd2dc2dc4e2e943f1268023ce88ba1465
> Author: Richard Smith <richard-llvm@metafoo.co.uk>
> Date:   Thu Oct 20 07:53:17 2016 +0000
> 
>     Work around MSVC rejects-valid. Apparenty (some versions of) MSVC will
> check
>     that a member is default-initializable even if it's initialized by a
> default
>     member initializer.

Notice that this is a compilation fix for an earlier patch; MSVC 2015 (some updates only?) failed to compile r284685.  A reduced testcase still fails with r284684, so I don't believe that we've narrowed down the actual breaking change.  I'm going to file an LLVM bug with the reduced testcase, though.
(Assignee)

Comment 6

2 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> We try not to take local patches to Breakpad, preferring to instead upstream
> them. I guess that enum is declared in a file that claims to be C99, so we
> can't just use an enum class there...

Source https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/google_breakpad/common/minidump_exception_win32.h#33 says:

(This is C99 source, please don't corrupt it with C++.)

:(

(In reply to Nathan Froyd [:froydnj] from comment #5)
> I'm going to file an LLVM bug with the reduced testcase, though.

Actually, looking through the LLVM bugzilla suggests that they're not going to be that interested in fixing this. :(  But I can make a case based on C/C++ compat...maybe?

I don't know what a good fix here is.  MSVC is supposed to give that enum a uint32_t underlying type--at least it would be in C++--because the enum values don't fit in an int32_t.  But if we add casts to the switches that use those enum values, presumably other compilers (that do follow the spec) will warn?  Unless this code is win32-specific, in which case we probably don't care about other compilers.
It's not, that code is in the processor (which gets compiled into minidump_stackwalk etc).
(Reporter)

Comment 8

2 years ago
r284701 is just the revision of clang I'm using, not the revision that causes this failure.
(Assignee)

Comment 9

2 years ago
https://llvm.org/bugs/show_bug.cgi?id=30776 is the LLVM bug, and it didn't get shut down right away, so there's a possibility that it might get fixed.
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8804837 [details]
Bug 1312313 - turn off -Wc++11-narrowing on clang; .mielczarek

https://reviewboard.mozilla.org/r/88686/#review93488

::: toolkit/crashreporter/crashreporter.mozbuild:22
(Diff revision 1)
>          '-Wno-shadow',
>      ]
> -    if CONFIG['CLANG_CXX']:
> -        CXXFLAGS += ['-Wno-implicit-fallthrough']
>  
> +if CONFIG['CLANG_CXX'] or CONFIG['CLANG_CL']:

Would it be useful to have a config variable that covers both regular clang as well as clang-cl? (I don't know how often we wind up needing to test whether we're using either of them.)
Attachment #8804837 - Flags: review?(ted) → review+

Comment 12

2 years ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79568328c8a7
turn off -Wc++11-narrowing on clang; r=ted.mielczarek

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/79568328c8a7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

2 years ago
See Also: → bug 1318376
Assignee: nobody → nfroyd
status-firefox52: affected → ---
You need to log in before you can comment on or make changes to this bug.