unittests: fix -Wnarrowing build errors (issue 1605153004 by vapier@chromium.org)

[vap...@chromium.org]

Reviewers: Lei Zhang (OOO), Ted Mielczarek, Mark Mentovai
CL: https://codereview.chromium.org/1605153004/

Message:
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); }

Description:
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

Base URL: https://chromium.googlesource.com/breakpad/breakpad.git@master

Affected files (+43, -24 lines):
M src/breakpad_googletest_includes.h
M src/common/dwarf/bytereader_unittest.cc
M src/common/dwarf/dwarf2diehandler_unittest.cc


Index: src/breakpad_googletest_includes.h
diff --git a/src/breakpad_googletest_includes.h
b/src/breakpad_googletest_includes.h
index
1cc324b2323a771139a4ef19177ffe055f065415..80f3d3633831eb9195b168b17e28eaaedbb9cbcf
100644
--- a/src/breakpad_googletest_includes.h
+++ b/src/breakpad_googletest_includes.h
@@ -54,4 +54,14 @@
# undef ADDRESS_SANITIZER
#endif

+// Newer C++ standards make -Wnarrowing both default to on and throw an
error.
+// This shows up for us when using char* buffers to hold raw bytes, but we
use
+// a value that is signed like 0xff. The compiler will sign extend and
convert
+// that into an integer (e.g. 0xffffffff) and then throw an error when we
try
+// to assign it to a char type. This macro lets us define raw unsigned
arrays
+// and then assign to char* buffers.
+// const char *data = CHAR_BUFFER(0xfe, 0x09);
+#define CHAR_BUFFER(...) \
+ (reinterpret_cast<const char *>((const unsigned char []){__VA_ARGS__}))
+
#endif // BREAKPAD_GOOGLETEST_INCLUDES_H__
Index: src/common/dwarf/bytereader_unittest.cc
diff --git a/src/common/dwarf/bytereader_unittest.cc
b/src/common/dwarf/bytereader_unittest.cc
index
4311ab6a25393b00190791f08d981e657a4cc20b..da3ece0d035e3d819f34e0b6e6fbf736be1edb5a
100644
--- a/src/common/dwarf/bytereader_unittest.cc
+++ b/src/common/dwarf/bytereader_unittest.cc
@@ -384,7 +384,7 @@ TEST_F(ReaderDeathTest, DW_EH_PE_omit) {
}

TEST_F(Reader, DW_EH_PE_absptr4) {
- static const char data[] = { 0x27, 0x57, 0xea, 0x40 };
+ static const char *data = CHAR_BUFFER(0x27, 0x57, 0xea, 0x40);
ByteReader reader(ENDIANNESS_LITTLE);
reader.SetAddressSize(4);
EXPECT_EQ(0x40ea5727U,
@@ -394,9 +394,9 @@ TEST_F(Reader, DW_EH_PE_absptr4) {
}

TEST_F(Reader, DW_EH_PE_absptr8) {
- static const char data[] = {
+ static const char *data = CHAR_BUFFER(
0x60, 0x27, 0x57, 0xea, 0x40, 0xc2, 0x98, 0x05, 0x01, 0x50
- };
+ );
ByteReader reader(ENDIANNESS_LITTLE);
reader.SetAddressSize(8);
EXPECT_EQ(0x010598c240ea5727ULL,
@@ -406,7 +406,7 @@ TEST_F(Reader, DW_EH_PE_absptr8) {
}

TEST_F(Reader, DW_EH_PE_uleb128) {
- static const char data[] = { 0x81, 0x84, 0x4c };
+ static const char *data = CHAR_BUFFER(0x81, 0x84, 0x4c);
ByteReader reader(ENDIANNESS_LITTLE);
reader.SetAddressSize(4);
EXPECT_EQ(0x130201U,
@@ -416,7 +416,7 @@ TEST_F(Reader, DW_EH_PE_uleb128) {
}

TEST_F(Reader, DW_EH_PE_udata2) {
- static const char data[] = { 0xf4, 0x8d };
+ static const char *data = CHAR_BUFFER(0xf4, 0x8d);
ByteReader reader(ENDIANNESS_BIG);
reader.SetAddressSize(4);
EXPECT_EQ(0xf48dU,
@@ -426,7 +426,7 @@ TEST_F(Reader, DW_EH_PE_udata2) {
}

TEST_F(Reader, DW_EH_PE_udata4) {
- static const char data[] = { 0xb2, 0x68, 0xa5, 0x62, 0x8f, 0x8b };
+ static const char *data = CHAR_BUFFER(0xb2, 0x68, 0xa5, 0x62, 0x8f,
0x8b);
ByteReader reader(ENDIANNESS_BIG);
reader.SetAddressSize(8);
EXPECT_EQ(0xa5628f8b,
@@ -436,9 +436,9 @@ TEST_F(Reader, DW_EH_PE_udata4) {
}

TEST_F(Reader, DW_EH_PE_udata8Addr8) {
- static const char data[] = {
+ static const char *data = CHAR_BUFFER(
0x27, 0x04, 0x73, 0x04, 0x69, 0x9f, 0x19, 0xed, 0x8f, 0xfe
- };
+ );
ByteReader reader(ENDIANNESS_LITTLE);
reader.SetAddressSize(8);
EXPECT_EQ(0x8fed199f69047304ULL,
@@ -448,9 +448,9 @@ TEST_F(Reader, DW_EH_PE_udata8Addr8) {
}

TEST_F(Reader, DW_EH_PE_udata8Addr4) {
- static const char data[] = {
+ static const char *data = CHAR_BUFFER(
0x27, 0x04, 0x73, 0x04, 0x69, 0x9f, 0x19, 0xed, 0x8f, 0xfe
- };
+ );
ByteReader reader(ENDIANNESS_LITTLE);
reader.SetAddressSize(4);
EXPECT_EQ(0x69047304ULL,
@@ -460,7 +460,7 @@ TEST_F(Reader, DW_EH_PE_udata8Addr4) {
}

TEST_F(Reader, DW_EH_PE_sleb128) {
- static const char data[] = { 0x42, 0xff, 0xfb, 0x73 };
+ static const char *data = CHAR_BUFFER(0x42, 0xff, 0xfb, 0x73);
ByteReader reader(ENDIANNESS_BIG);
reader.SetAddressSize(4);
EXPECT_EQ(-0x030201U & 0xffffffff,
@@ -470,7 +470,7 @@ TEST_F(Reader, DW_EH_PE_sleb128) {
}

TEST_F(Reader, DW_EH_PE_sdata2) {
- static const char data[] = { 0xb9, 0xbf };
+ static const char *data = CHAR_BUFFER(0xb9, 0xbf);
ByteReader reader(ENDIANNESS_LITTLE);
reader.SetAddressSize(8);
EXPECT_EQ(0xffffffffffffbfb9ULL,
@@ -480,7 +480,7 @@ TEST_F(Reader, DW_EH_PE_sdata2) {
}

TEST_F(Reader, DW_EH_PE_sdata4) {
- static const char data[] = { 0xa0, 0xca, 0xf2, 0xb8, 0xc2, 0xad };
+ static const char *data = CHAR_BUFFER(0xa0, 0xca, 0xf2, 0xb8, 0xc2,
0xad);
ByteReader reader(ENDIANNESS_LITTLE);
reader.SetAddressSize(8);
EXPECT_EQ(0xffffffffadc2b8f2ULL,
@@ -490,9 +490,9 @@ TEST_F(Reader, DW_EH_PE_sdata4) {
}

TEST_F(Reader, DW_EH_PE_sdata8) {
- static const char data[] = {
+ static const char *data = CHAR_BUFFER(
0xf6, 0x66, 0x57, 0x79, 0xe0, 0x0c, 0x9b, 0x26, 0x87
- };
+ );
ByteReader reader(ENDIANNESS_LITTLE);
reader.SetAddressSize(8);
EXPECT_EQ(0x87269b0ce0795766ULL,
@@ -502,7 +502,9 @@ TEST_F(Reader, DW_EH_PE_sdata8) {
}

TEST_F(Reader, DW_EH_PE_pcrel) {
- static const char data[] = { 0x4a, 0x8b, 0x1b, 0x14, 0xc8, 0xc4, 0x02,
0xce };
+ static const char *data = CHAR_BUFFER(
+ 0x4a, 0x8b, 0x1b, 0x14, 0xc8, 0xc4, 0x02, 0xce
+ );
ByteReader reader(ENDIANNESS_BIG);
reader.SetAddressSize(4);
DwarfPointerEncoding encoding =
@@ -515,7 +517,9 @@ TEST_F(Reader, DW_EH_PE_pcrel) {
}

TEST_F(Reader, DW_EH_PE_textrel) {
- static const char data[] = { 0xd9, 0x0d, 0x05, 0x17, 0xc9, 0x7a, 0x42,
0x1e };
+ static const char *data = CHAR_BUFFER(
+ 0xd9, 0x0d, 0x05, 0x17, 0xc9, 0x7a, 0x42, 0x1e
+ );
ByteReader reader(ENDIANNESS_LITTLE);
reader.SetAddressSize(4);
reader.SetTextBase(0xb91beaf0);
@@ -528,7 +532,9 @@ TEST_F(Reader, DW_EH_PE_textrel) {
}

TEST_F(Reader, DW_EH_PE_datarel) {
- static const char data[] = { 0x16, 0xf2, 0xbb, 0x82, 0x68, 0xa7, 0xbc,
0x39 };
+ static const char *data = CHAR_BUFFER(
+ 0x16, 0xf2, 0xbb, 0x82, 0x68, 0xa7, 0xbc, 0x39
+ );
ByteReader reader(ENDIANNESS_BIG);
reader.SetAddressSize(8);
reader.SetDataBase(0xbef308bd25ce74f0ULL);
@@ -541,7 +547,9 @@ TEST_F(Reader, DW_EH_PE_datarel) {
}

TEST_F(Reader, DW_EH_PE_funcrel) {
- static const char data[] = { 0x84, 0xf8, 0x14, 0x01, 0x61, 0xd1, 0x48,
0xc9 };
+ static const char *data = CHAR_BUFFER(
+ 0x84, 0xf8, 0x14, 0x01, 0x61, 0xd1, 0x48, 0xc9
+ );
ByteReader reader(ENDIANNESS_BIG);
reader.SetAddressSize(4);
reader.SetFunctionBase(0x823c3520);
@@ -617,14 +625,14 @@ TEST(UsableBase, ClearFunction) {

struct AlignedFixture {
AlignedFixture() : reader(ENDIANNESS_BIG) { reader.SetAddressSize(4); }
- static const char data[10];
+ static const char *data;
ByteReader reader;
size_t pointer_size;
};

-const char AlignedFixture::data[10] = {
+const char *AlignedFixture::data = CHAR_BUFFER(
0xfe, 0x6e, 0x93, 0xd8, 0x34, 0xd5, 0x1c, 0xd3, 0xac, 0x2b
-};
+);

class Aligned: public AlignedFixture, public Test { };

Index: src/common/dwarf/dwarf2diehandler_unittest.cc
diff --git a/src/common/dwarf/dwarf2diehandler_unittest.cc
b/src/common/dwarf/dwarf2diehandler_unittest.cc
index
c0a532aa01624e15c9620b49239c01e9b4b85411..1388339390bf8297e2e10b9972aab1c27ac720e9
100644
--- a/src/common/dwarf/dwarf2diehandler_unittest.cc
+++ b/src/common/dwarf/dwarf2diehandler_unittest.cc
@@ -185,8 +185,9 @@ TEST(Dwarf2DIEHandler, PassAttributeValues) {
MockRootDIEHandler mock_root_handler;
DIEDispatcher die_dispatcher(&mock_root_handler);

- const char buffer[10] = { 0x24, 0x24, 0x35, 0x9a, 0xca,
- 0xcf, 0xa8, 0x84, 0xa7, 0x18 };
+ const char *buffer = CHAR_BUFFER(
+ 0x24, 0x24, 0x35, 0x9a, 0xca, 0xcf, 0xa8, 0x84, 0xa7, 0x18
+ );
string str = "\xc8\x26\x2e\x0d\xa4\x9c\x37\xd6\xfb\x1d";

// Set expectations.

[ma...@chromium.org]

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.

https://codereview.chromium.org/1605153004/

[vap...@chromium.org]

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.

https://codereview.chromium.org/1605153004/

[bruce...@chromium.org]

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


https://codereview.chromium.org/1605153004/

[ma...@chromium.org]

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.

https://codereview.chromium.org/1605153004/

[vap...@chromium.org]

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.

https://codereview.chromium.org/1605153004/

[ma...@chromium.org]

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/bytereader.h
File src/common/dwarf/bytereader.h (right):

https://codereview.chromium.org/1605153004/diff/20001/src/common/dwarf/bytereader.h#newcode1
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/bytereader.h#newcode33
src/common/dwarf/bytereader.h:33:
#include <stdint.h>

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

https://codereview.chromium.org/1605153004/diff/20001/src/common/dwarf/bytereader_unittest.cc#newcode505
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/dwarf2reader_die_unittest.cc
File src/common/dwarf/dwarf2reader_die_unittest.cc (right):

https://codereview.chromium.org/1605153004/diff/20001/src/common/dwarf/dwarf2reader_die_unittest.cc#newcode135
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.

https://codereview.chromium.org/1605153004/

[vap...@chromium.org]

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

https://codereview.chromium.org/1605153004/

[ma...@chromium.org]

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

https://codereview.chromium.org/1605153004/

[ma...@chromium.org]

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

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

https://codereview.chromium.org/1605153004/

[vap...@chromium.org]

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

https://codereview.chromium.org/1605153004/

[the...@chromium.org]

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.

https://codereview.chromium.org/1605153004/

[vap...@chromium.org]

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

https://codereview.chromium.org/1605153004/

[the...@chromium.org]

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.

https://codereview.chromium.org/1605153004/

[ma...@chromium.org]

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

https://codereview.chromium.org/1605153004/

[vap...@chromium.org]

okie dokie, back to stdint.h

https://codereview.chromium.org/1605153004/