Closed Bug 1312313 Opened 6 years ago Closed 6 years ago
Fails to build breakpad with clang-cl because of c++11 narrowing checks
58 bytes, text/x-review-board-request
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 ^
(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.
ting@ting-nb MINGW64 /c/w/tools/llvm/src/tools/clang (master) $ git log -n 1 commit c467cbcfd2dc2dc4e2e943f1268023ce88ba1465 Author: Richard Smith <email@example.com> 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
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.
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?
(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 <firstname.lastname@example.org> > 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.
(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).
r284701 is just the revision of clang I'm using, not the revision that causes this failure.
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 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+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/79568328c8a7 turn off -Wc++11-narrowing on clang; r=ted.mielczarek
You need to log in before you can comment on or make changes to this bug.