Cleaning up google-breakpad source code of signed-unsigned comparison warnings

Cleaning up google-breakpad source code of signed-unsigned comparison warnings

[Ivan Penkov]

A couple of signed-unsigned comparison warnings were recently introduced in
src/client/linux/handler/exception_handler.cc.  This is a nuisance during
imports of breakpad source code into external projects.  I'm basically
disallowing this warning in the Makefile.am file and fixing all occurrences of
this warning.

[Mark Mentovai]

I’m unable to perform reviews for at least the rest of this week.


On Mon, Oct 15, 2012 at 5:51 PM, <ivan.penkov@gmail.com> wrote:

> Reviewers: Ted Mielczarek, Mark Mentovai,
>
> Message:
> A couple of signed-unsigned comparison warnings were recently introduced
> in src/client/linux/handler/**exception_handler.cc.  This is a nuisance
> during imports of breakpad source code into external projects.  I'm
> basically disallowing this warning in the Makefile.am file and fixing
> all occurrences of this warning.
>
> Description:
> Cleaning up google-breakpad source code of signed-unsigned comparison
> warnings
>
> Please review this at
http://breakpad.appspot.com/**488002/<http://breakpad.appspot.com/488002/>
>
> Affected files:
>   M     Makefile.am
>   M     Makefile.in
>   M     src/client/linux/handler/**exception_handler.cc
>   M     src/client/linux/handler/**exception_handler_unittest.cc
>   M     src/client/linux/minidump_**writer/line_reader_unittest.cc
>   M     src/client/linux/minidump_**writer/linux_core_dumper_**unittest.cc
>   M     src/client/linux/minidump_**writer/linux_ptrace_dumper_**
> unittest.cc
>   M     src/client/linux/minidump_**writer/minidump_writer_**unittest.cc
>   M     src/common/linux/dump_symbols_**unittest.cc
>   M     src/common/linux/elf_core_**dump_unittest.cc
>   M     src/common/linux/file_id_**unittest.cc
>   M     src/common/linux/linux_libc_**support_unittest.cc
>   M     src/common/linux/memory_**mapped_file_unittest.cc
>   M     src/common/linux/safe_**readlink_unittest.cc
>   M     src/common/linux/synth_elf_**unittest.cc
>   M     src/common/memory_range_**unittest.cc
>   M     src/processor/basic_source_**line_resolver_unittest.cc
>   M     src/processor/binarystream_**unittest.cc
>   M     src/processor/disassembler_**x86_unittest.cc
>   M     src/processor/fast_source_**line_resolver_unittest.cc
>   M     src/processor/minidump_**stackwalk.cc
>   M     src/processor/minidump_**unittest.cc
>   M     src/processor/module_**serializer.h
>   M     src/processor/stackwalker_arm_**unittest.cc
>   M     src/processor/static_map.h
>   M     src/processor/static_range_**map-inl.h
>   M     src/processor/static_range_**map_unittest.cc
>   M     src/tools/linux/md2core/**minidump-2-core.cc
>   M     src/tools/linux/md2core/**minidump_memory_range_**unittest.cc
>
>
>

[Ivan Penkov]

On 2012/10/16 13:34:31, Ted Mielczarek wrote:

Ted, I don't see any of your comments.  Can you take another look?

Thanks,
-Ivan

[Ted Mielczarek]

LGTM

Sorry, I was just CCing the list last time, and then I forgot to actually review
the patch.

http://breakpad.appspot.com/488002/diff/1/src/client/linux/minidump_writer/li...
File src/client/linux/minidump_writer/line_reader_unittest.cc (right):

http://breakpad.appspot.com/488002/diff/1/src/client/linux/minidump_writer/li...
src/client/linux/minidump_writer/line_reader_unittest.cc:169:
ASSERT_EQ(static_cast<ssize_t>(sizeof(l)), r);
Can't you just use ssize_t for the type of r above? (That's the actual return
value of write anyway.)

http://breakpad.appspot.com/488002/diff/1/src/client/linux/minidump_writer/mi...
File src/client/linux/minidump_writer/minidump_writer_unittest.cc (right):

http://breakpad.appspot.com/488002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer_unittest.cc:384:
ASSERT_EQ(static_cast<ssize_t>(sizeof(junk)), nr);
Similarly here, you could just use ssize_t for the type of nr.

http://breakpad.appspot.com/488002/diff/1/src/processor/static_range_map-inl.h
File src/processor/static_range_map-inl.h (right):

http://breakpad.appspot.com/488002/diff/1/src/processor/static_range_map-inl....
src/processor/static_range_map-inl.h:109: if (index >=
static_cast<ssize_t>(GetCount())) {
Should we make the index parameter unsigned?

[Ivan Penkov]

Thanks for the comments.  I was able to address one of the concerns.

-Ivan

http://breakpad.appspot.com/488002/diff/1/src/client/linux/minidump_writer/li...
File src/client/linux/minidump_writer/line_reader_unittest.cc (right):

http://breakpad.appspot.com/488002/diff/1/src/client/linux/minidump_writer/li...
src/client/linux/minidump_writer/line_reader_unittest.cc:169:
ASSERT_EQ(static_cast<ssize_t>(sizeof(l)), r);
On 2012/11/20 12:59:56, Ted Mielczarek wrote:
> Can't you just use ssize_t for the type of r above? (That's the actual return
> value of write anyway.)

r is already signed - which is correct.  The problem is that "sizeof" is
unsigned and the comparison generates the warning.  The alternative: using an
unsigned type (like size_t) for r would be plain wrong, because write can return
-1.

http://breakpad.appspot.com/488002/diff/1/src/client/linux/minidump_writer/mi...
File src/client/linux/minidump_writer/minidump_writer_unittest.cc (right):

http://breakpad.appspot.com/488002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer_unittest.cc:384:
ASSERT_EQ(static_cast<ssize_t>(sizeof(junk)), nr);
On 2012/11/20 12:59:56, Ted Mielczarek wrote:
> Similarly here, you could just use ssize_t for the type of nr.

Exact same issue.  Please, check my other response.

http://breakpad.appspot.com/488002/diff/1/src/processor/static_range_map-inl.h
File src/processor/static_range_map-inl.h (right):

http://breakpad.appspot.com/488002/diff/1/src/processor/static_range_map-inl....
src/processor/static_range_map-inl.h:109: if (index >=
static_cast<ssize_t>(GetCount())) {
On 2012/11/20 12:59:56, Ted Mielczarek wrote:
> Should we make the index parameter unsigned?

I actually decided to change GetCount method to return signed int.  This way
StaticRangeMap is consistent with RangeMap.