unittests: fix -Wnarrowing build errors

convert to uint8_t* for binary data to fix -Wnarrowing build errors

Newer gcc versions default to -Werror=narrowing when using newer C++
standards (which we do).  This causes issues when we try to stuff a
value like 0xea into a char -- the value is out of range for signed
char bytes.  That's when gcc throws an error:
.../bytereader_unittest.cc: In member function 'virtual void Reader_DW_EH_PE_absptr4_Test::TestBody()':
.../bytereader_unittest.cc:400:55: error: narrowing conversion of '234' from 'int' to 'char' inside { } [-Wnarrowing]

BUG=chromium:579384
TEST=`make check` passes
R=mark@chromium.org

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/258591ed4cb502925f1ffda8c8af57559c0c1c26

[vapier, 2016-01-20 23:53:44]

vapier@chromium.org changed reviewers:
+ mark@chromium.org, ted.mielczarek@gmail.com, thestig@chromium.org

[vapier, 2016-01-20 23:53:44]

i will be the first to admit i'm not terribly happy with this internal macro and
the magic casts it has.  i'm certainly open to alternatives.

the reason i went this route is that, imo, the alternatives i could think of are
worse:
(0) current code:
    static const char data[] = { ... };

(1) use a second var to cast through:
    static const unsigned char unsigned_data[] = { ... };
    static const char *data = reinterpret_cast<const char *>(unsigned_data);

(2) (sometimes?) cast each byte:
    static const char data[] = { (char)0xff, (char)0xe7, 0x07, (char)0x90 };

(3) try and use a string (which also implicitly appends a NUL byte):
    static const char *data = "\xff\xe7\x07\x90";

(4) update all the call sites to cast the buffer can require more than one cast
per var (if the var is used more than once) and causes the code to get much
uglier:
    static const unsigned char data[] = { ... }
    foo(reinterpret_cast<const char *>(data), ...);

(5) update the APIs of classes we pass in char* buffers to also provide a func
that takes an unsigned char*.  this is a bunch of call sites and would require
adding a bunch of trampoline funcs like:
    int foo(const unsigned char *buffer) { ... }
    int foo(const char *buffer) { return foo(reinterpret_cast<const unsigned
char *>(buffer); }

[Mark Mentovai, 2016-01-21 21:21:03]

I hate all of the options too.

Is this stuff ever even conceptually char data? Maybe instead of deciding
between char and unsigned char, everyone should just be dealing with uint8_t. I
know, that’s basically the same thing as unsigned char, but it doesn’t come with
the expectation that it ought to be usable as char (of any signedness).

This is like your (5) but also getting rid of the original |int foo(const char
*buffer)| version and their callers. If you hit all of the caller sites, you
don’t need the trampolines.

[vapier, 2016-01-21 21:24:00]

i can take a look at just how painful it would be to convert everything to
uint8_t.  in the meantime, i can probably update CrOS to use
-Wno-error=narrowing to avoid this problem so i can move forward.

[brucedawson, 2016-01-21 21:38:36]

> the value is implicitly sign-extended
> to an int (0xffffffff) before being truncated back to a char.

I'm not sure that description is accurate. The value 0xff is outside of the
range of a signed char (-128 to 127). int doesn't enter into it. If char were
unsigned it would fit but I guess on this platform it isn't.

I guess one question would be why were are putting unsigned char variables into
a char array. That suggests that the cleanest fix would be to change to using
uint8_t or unsigned char (options 1, 4, or 5). Clearly those come with their own
messiness but they seem to match the intention the best, I think.

[Mark Mentovai, 2016-01-21 21:49:51]

int does enter into it, because 0xea is a (signed) int. There’s no
sign-extension happening, but there is narrowing because 0xea doesn’t fit into a
(signed) char.

[vapier, 2016-01-23 18:38:39]

Description was changed from

==========
unittests: fix -Wnarrowing build errors

Newer gcc versions default to -Werror=narrowing when using newer C++
standards (which we do).  This causes issues when we try to start a
byte like 0xff into a char -- the value is implicitly sign-extended
to an int (0xffffffff) before being truncated back to a char.  That's
when gcc throws an error:
.../bytereader_unittest.cc: In member function 'virtual void
Reader_DW_EH_PE_absptr4_Test::TestBody()':
.../bytereader_unittest.cc:400:55: error: narrowing conversion of '234' from
'int' to 'char' inside { } [-Wnarrowing]
   static const char data[] = { 0x27, 0x57, 0xea, 0x40 };

BUG=chromium:579384
TEST=`make check` passes
==========

to

==========
convert to uint8_t* for binary data to fix -Wnarrowing build errors

Newer gcc versions default to -Werror=narrowing when using newer C++
standards (which we do).  This causes issues when we try to start a
byte like 0xff into a char -- the value is out of range for signed
char bytes.  That's when gcc throws an error:
.../bytereader_unittest.cc: In member function 'virtual void
Reader_DW_EH_PE_absptr4_Test::TestBody()':
.../bytereader_unittest.cc:400:55: error: narrowing conversion of '234' from
'int' to 'char' inside { } [-Wnarrowing]

BUG=chromium:579384
TEST=`make check` passes
==========

[vapier, 2016-01-23 18:42:41]

i rewrote the dwarf bytereader modules to use uint8_t.  was much larger, but
looks straight forward.  the dwarf2reader_cfi_unittest needed a bunch of
uint8_t->char casts because the CFISection.GetContents api currently operates on
std::string to hold binary data.  we could convert that to std::vector<uint8_t>,
but that would require more shaking out.  i think this is a good place to sync.

[Mark Mentovai, 2016-01-25 15:08:43]

LGTM

Although it touches more lines, this is fairly clean. I’m not too concerned with
the casts in dwarf2reader. As you mentioned, dwarf2reader_cfi_unittest is a
target for possible improvement, but I think it’s OK for this change.

https://codereview.chromium.org/1605153004/diff/20001/src/common/dwarf/bytere...
File src/common/dwarf/bytereader.h (right):

https://codereview.chromium.org/1605153004/diff/20001/src/common/dwarf/bytere...
src/common/dwarf/bytereader.h:1: // -*- mode: C++ -*-
CL description: “start a byte”—did you mean “stuff a byte?” And can you use 0xea
instead of 0xff in the text to match the compiler warning?

https://codereview.chromium.org/1605153004/diff/20001/src/common/dwarf/bytere...
src/common/dwarf/bytereader.h:33: 
#include <stdint.h>

https://codereview.chromium.org/1605153004/diff/20001/src/common/dwarf/bytere...
File src/common/dwarf/bytereader_unittest.cc (right):

https://codereview.chromium.org/1605153004/diff/20001/src/common/dwarf/bytere...
src/common/dwarf/bytereader_unittest.cc:505: static const uint8_t data[] = {
0x4a, 0x8b, 0x1b, 0x14, 0xc8, 0xc4, 0x02, 0xce };
This and the next few ones go past 80 characters now.

https://codereview.chromium.org/1605153004/diff/20001/src/common/dwarf/dwarf2...
File src/common/dwarf/dwarf2reader_die_unittest.cc (right):

https://codereview.chromium.org/1605153004/diff/20001/src/common/dwarf/dwarf2...
src/common/dwarf/dwarf2reader_die_unittest.cc:135:
section_map[".debug_info"].first  = reinterpret_cast<const uint8_t
*>(info_contents.data());
80 here and a couple of lines down.

[vapier, 2016-01-25 21:32:59]

Description was changed from

==========
convert to uint8_t* for binary data to fix -Wnarrowing build errors

Newer gcc versions default to -Werror=narrowing when using newer C++
standards (which we do).  This causes issues when we try to start a
byte like 0xff into a char -- the value is out of range for signed
char bytes.  That's when gcc throws an error:
.../bytereader_unittest.cc: In member function 'virtual void
Reader_DW_EH_PE_absptr4_Test::TestBody()':
.../bytereader_unittest.cc:400:55: error: narrowing conversion of '234' from
'int' to 'char' inside { } [-Wnarrowing]

BUG=chromium:579384
TEST=`make check` passes
==========

to

==========
convert to uint8_t* for binary data to fix -Wnarrowing build errors

Newer gcc versions default to -Werror=narrowing when using newer C++
standards (which we do).  This causes issues when we try to stuff a
value like 0xea into a char -- the value is out of range for signed
char bytes.  That's when gcc throws an error:
.../bytereader_unittest.cc: In member function 'virtual void
Reader_DW_EH_PE_absptr4_Test::TestBody()':
.../bytereader_unittest.cc:400:55: error: narrowing conversion of '234' from
'int' to 'char' inside { } [-Wnarrowing]

BUG=chromium:579384
TEST=`make check` passes
==========

[vapier, 2016-01-25 21:38:41]

should have all the feedback addressed here.  i'll land later today/tomorrow if
there's no other concerns ;).

[Mark Mentovai, 2016-01-25 21:50:04]

You should probably get <stdint.h> into the other headers here too, but LGTM
otherwise.

[Mark Mentovai, 2016-01-25 21:50:39]

https://codereview.chromium.org/1605153004/diff/60001/src/common/linux/dump_s...
File src/common/linux/dump_symbols.cc (right):

https://codereview.chromium.org/1605153004/diff/60001/src/common/linux/dump_s...
src/common/linux/dump_symbols.cc:1: // Copyright (c) 2011 Google Inc.
And I guess these tool .cc files too.

[vapier, 2016-01-25 22:14:18]

i've switched to <cstdint> rather than <stdint.h>, and added it to all these
files.  i keep trying to program like this is C code, not C++ :).

[Lei Zhang, 2016-01-25 22:24:55]

On 2016/01/25 22:14:18, vapier wrote:
> i've switched to <cstdint> rather than <stdint.h>, and added it to all these
> files.  i keep trying to program like this is C code, not C++ :).

BTW, Chromium uses stdint.h just about everywhere and not cstdint.

[vapier, 2016-01-25 22:35:36]

<cstdint> feels more natural to me in C++ code, but i don't really have a
preference either way.

[Lei Zhang, 2016-01-25 23:05:38]

On 2016/01/25 22:35:36, vapier wrote:
> <cstdint> feels more natural to me in C++ code, but i don't really have a
> preference either way.

Consider stdint.h for consistency with Chromium? I can't find a thread on the
topic, but someone must have made a decision to use stdint.h in Chromium over
cstdint.

[Mark Mentovai, 2016-01-25 23:34:48]

Please don’t use <cstdint> unless you want to call the type std::uint8_t
everywhere.

[vapier, 2016-01-25 23:45:23]

okie dokie, back to stdint.h

[vapier, 2016-01-26 20:38:24]

Description was changed from

==========
convert to uint8_t* for binary data to fix -Wnarrowing build errors

Newer gcc versions default to -Werror=narrowing when using newer C++
standards (which we do).  This causes issues when we try to stuff a
value like 0xea into a char -- the value is out of range for signed
char bytes.  That's when gcc throws an error:
.../bytereader_unittest.cc: In member function 'virtual void
Reader_DW_EH_PE_absptr4_Test::TestBody()':
.../bytereader_unittest.cc:400:55: error: narrowing conversion of '234' from
'int' to 'char' inside { } [-Wnarrowing]

BUG=chromium:579384
TEST=`make check` passes
==========

to

==========
convert to uint8_t* for binary data to fix -Wnarrowing build errors

Newer gcc versions default to -Werror=narrowing when using newer C++
standards (which we do).  This causes issues when we try to stuff a
value like 0xea into a char -- the value is out of range for signed
char bytes.  That's when gcc throws an error:
.../bytereader_unittest.cc: In member function 'virtual void
Reader_DW_EH_PE_absptr4_Test::TestBody()':
.../bytereader_unittest.cc:400:55: error: narrowing conversion of '234' from
'int' to 'char' inside { } [-Wnarrowing]

BUG=chromium:579384
TEST=`make check` passes
R=mark@chromium.org

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/258591ed4cb502925f1ffda...
==========

[vapier, 2016-01-26 20:38:25]

Committed patchset #6 (id:100001) manually as
258591ed4cb502925f1ffda8c8af57559c0c1c26 (presubmit successful).