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

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Ting-Yu Chou [:ting] (away)]

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              ^

Comment 1

[Nathan Froyd [:froydnj]]

(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.

Comment 2

[Ting-Yu Chou [:ting] (away)]

ting@ting-nb MINGW64 /c/w/tools/llvm/src/tools/clang (master)
$ git log -n 1
commit c467cbcfd2dc2dc4e2e943f1268023ce88ba1465
Author: Richard Smith <[e-mail]>
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

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[(not currently active) Ted Mielczarek]

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?

Comment 5

[Nathan Froyd [:froydnj]]

(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 <[e-mail]>
> 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.

Comment 6

[Nathan Froyd [:froydnj]]

(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.

Comment 7

[(not currently active) Ted Mielczarek]

It's not, that code is in the processor (which gets compiled into minidump_stackwalk etc).

Comment 8

[Ting-Yu Chou [:ting] (away)]

r284701 is just the revision of clang I'm using, not the revision that causes this failure.

Comment 9

[Nathan Froyd [:froydnj]]

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 10

[Nathan Froyd [:froydnj]]

Attachment: Bug 1312313 - turn off -Wc++11-narrowing on clang; .mielczarek

Breakpad has enums that look like:

typedef enum {
  VAL1 = 0xc000008f,
  ...
} WeirdEnum;

The C++ standard says that the implementation needs to pick the
underlying type of the enum such that it can safely hold all values
of the enumeration.  For the above enum, uint32_t would be such a
type.  However, MSVC (and clang-cl) apparently do something quite
different, and clang-cl complains when you use the enum above like
so:

  uint32_t testval = ...;
  switch (testval) {
    case VAL1:
    ...
  };

since you can't convert VAL1 (a negative, signed 32-bit integer)
into a uint32_t to compare with testval.

The easiest way avoid this is to disable the warning for breakpad
code.  We also disable the related -Wmicrosoft-enum-value warning
globally, for similar reasons.

Review commit: https://reviewboard.mozilla.org/r/88686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88686/

Comment 11

[(not currently active) Ted Mielczarek]

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.)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79568328c8a7
turn off -Wc++11-narrowing on clang; r=ted.mielczarek

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/79568328c8a7