The end goal of this change is to boost the test coverage of breakpad in Google

Fixing various compiler warnings and applying minor tweaks to allow running of
the mojority of breakpad unittests in Google.

[ivanpe.google]

The main goal of this change is to allow the compilation and execution of most
breakpad unittests under google3.  My preliminary tests show that the Breakpad
test coverage will increase from 47% to 79%.  The increased test coverage won't
be the only benefit from this.  By compiling and running most unittests under
google3 we make sure that the code compiles with stricter compiler warning
levels and additionally we test for memory leaks.

The main obstacle was to allow the coexistence of std::string and a global
::string class which is widely used in the Google code base.

The changes are mostly mechanical with a few exceptions:

1. A couple of semi-mechanical replacements:
  - std::string  -->  string
  - using std::string  --> #include "common/using_std_string.h"
  - #include "testing/gtest/include/gtest/gtest.h"  --> #include
"breakpad_googletest_includes.h"

2. Updating a static initialization in synth_minidump_unittest_data.h to allow
compilation under google3

3. Getting rid of "using google_breakpad::SynthMinidump::Thread;" in
synth_minidump_unittest.cc in order to resolve a namespace conflict.

4. Adding a virtual destructor to "class Section" and "class Language" since
they have virtual methods.

5. Reducing the stack frame size consumed by certain functions where large local
variables are instantiated on the stack.  Replacing with dynamic instantioations
(using scoped_array to preserve semantics)

6. Added "chmod u+w" to handle a case where the test is modifying a read-only
file.  The same file is not read-only when tests are ran under the SVN source
root.

7. Removed a couple of unused local variables (to allow compilation under more
strict rules)

8. Updating a code fragment which locates a helper binary next to the current
binary to use an environment variable if such has been exported.

9. Updated a couple of old C-style casts to their safer C++ counterparts.  This
allows compilation under stricter rules.

Thanks,
-Ivan

[Mark Mentovai]

I’ve only looked at the first few files.

Can you split out the mechanical std::string changes into a separate change?
It’s rude to expect me to pick through a changelist that does at least 9 things
and figure out which ones I need to pay close attention to and which ones are
mechanical and safe.

To avoid regressing and introducing any new code that uses std::string, we
should have a presubmit check that looks for and forbids use of std::string.
Otherwise, given common programming practices, we’re almost guaranteed to have
people writing std::string in here.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
File client/linux/minidump_writer/linux_core_dumper.cc (right):

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
client/linux/minidump_writer/linux_core_dumper.cc:124: fprintf(stderr, "Could
not map core dump file '%s' into memory\n",
This is OK, but it’s an unrelated change. It’s better to split unrelated changes
up.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
File client/linux/minidump_writer/linux_core_dumper_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
client/linux/minidump_writer/linux_core_dumper_unittest.cc:37: #include
"processor/logging.h"
Why?

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
File client/linux/minidump_writer/linux_dumper_unittest_helper.cc (right):

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
client/linux/minidump_writer/linux_dumper_unittest_helper.cc:81:
google_breakpad::scoped_array<pthread_t> threads(new pthread_t[num_threads]);
Good catch, but also unrelated to the main std::string thrust of this change.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
File client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc:64: if
(getenv("bindir")) {
Rather than potentially calling getenv twice, cache its result in a local char*.

Same in minidump_writer_unittest.cc.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc:144: << "Error: "
<< strerror(errno) << std::endl;
You need <errno.h> and <string.h> to do this.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/min...
File client/linux/minidump_writer/minidump_writer_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/min...
client/linux/minidump_writer/minidump_writer_unittest.cc:78: string templ =
temp_dir.path() + "/minidump-writer-unittest";
Don’t you need to #include your new header to do this?

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/min...
client/linux/minidump_writer/minidump_writer_unittest.cc:216:
google_breakpad::scoped_array<char> buffer(new char[kMemorySize]);
kMemorySize is confusingly-named. It is not a constant in the sense of something
that should be named with a k. If it were, the original form of this line would
not be a flexible-sized alloca-based array and would have been safe. As it
stands, seeing you resort to a scoped_array to avoid a local array when the size
looks like it’s a constexpr looks wrong. kMemorySize ought to be renamed.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/min...
client/linux/minidump_writer/minidump_writer_unittest.cc:285: string
GetHelperBinary() {
This should be in an anonymous namespace, or be static.

Or it should be shared somehow with the almost-identical function in the other
file.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/min...
client/linux/minidump_writer/minidump_writer_unittest.cc:288: helper_path =
string(getenv("bindir")) + "/";
As before, you should only getenv once.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/min...
client/linux/minidump_writer/minidump_writer_unittest.cc:307: 
Extra blank line.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/min...
client/linux/minidump_writer/minidump_writer_unittest.cc:323:
google_breakpad::scoped_array<char> cmdline(new char[2 * PATH_MAX]);
On the other hand, 2*PATH_MAX is a constexpr and this does not need to be a
scoped_array.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/min...
client/linux/minidump_writer/minidump_writer_unittest.cc:324:
sprintf(cmdline.get(), "/bin/cp \"%s\" \"%s\"", helper_path.c_str(),
And this use of sprintf is still as unsafe as it ever was.

[Ted Mielczarek]

On Mon, Jun 18, 2012 at 10:15 PM,  <ivanpe@google.com> wrote:
> Reviewers: Mark Mentovai, Ted Mielczarek,
>
> Message:
> The main goal of this change is to allow the compilation and execution
> of most breakpad unittests under google3.  My preliminary tests show
> that the Breakpad test coverage will increase from 47% to 79%.  The
> increased test coverage won't be the only benefit from this.  By
> compiling and running most unittests under google3 we make sure that the
> code compiles with stricter compiler warning levels and additionally we
> test for memory leaks.

For those of us outside the Googleplex, can you explain what google3
is and why these sorts of changes are necessary?

-Ted

[Mark Mentovai]

Google has a funny internal string class that uses the same interface as
std::string but is not named std::string.

Sadly, it’s not named gstring, either.

[Ted Mielczarek]

On Tue, Jun 19, 2012 at 11:48 AM,  <mark@chromium.org> wrote:
> Google has a funny internal string class that uses the same interface as
> std::string but is not named std::string.
>
> Sadly, it’s not named gstring, either.

Okay, I get that, but clearly there's more to this, from the original
mail: " By compiling and running most unittests under google3 we make
sure that the code compiles with stricter compiler warning levels and
additionally we test for memory leaks."

What exactly is the rest of this?

-Ted

[ivanpe.google]

Hi Ted,

The rest of the changes fall into two main categories:
   - Compiler warnings
   - Minor tweaks to allow running the unitests in Google which should not
affect anyone else

1. Compiler warnings: Currently, Breakpad code generates several compile
warnings which are problematic.  As a result, the code doesn't compile in
environments which don't tolerate these warnings.  Instead of tweaking the
compiler to skip these warnings I decided to fix a subset of them.  In order to
ensure that these issues won't be regressed by consequent changes I enabled the
same set of warnings to be treated as errors in the Breakpad makefile.  Now
everyone who contributes to google-breakpad should be able to see the new
errors.

AM_CXXFLAGS = \
	-Werror=non-virtual-dtor \
	-Werror=vla \
	-Werror=unused-variable \
	-Werror=missing-braces \
	-Werror=overloaded-virtual

For some reason I was unable to enforce some of the warnings to show outside
Google. For these, I decided to simply suppress the warnings on our side.  Here
are the warnings that I wanted to enforce but I couldn't.  All changes that I
previously did to avoid these warnings were reverted:

	-Werror=frame-larger-than=16384 \
	-Werror=int-to-pointer-cast \

2. Minor tweaks to allow running the tests in Google.
   - All breakpad unit tests must include "breakpad_googletest_includes.h"
instead of "testing/gtest/include/gtest/gtest.h".  This ensures that the proper
version of gtest is used (this is where the extra "memory leak checks" come
from).  This pattern is not new - all unittests already have it.  Unfortunately,
I found that it is missing in two of the unittests and I'm fixing it:
      * linux_libc_support_unittest.cc
      * memory_unittest.cc
   - Another minor tweak is to locate the binary using an environment valiable
instead of the SafeLReadLink trick.

Thanks,
-Ivan

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
File client/linux/minidump_writer/linux_core_dumper.cc (right):

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
client/linux/minidump_writer/linux_core_dumper.cc:124: fprintf(stderr, "Could
not map core dump file '%s' into memory\n",
On 2012/06/19 15:40:33, Mark Mentovai wrote:
> This is OK, but it’s an unrelated change. It’s better to split unrelated
changes
> up.

Done. The std::string related changes were moved to
http://breakpad.appspot.com/400002/

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
File client/linux/minidump_writer/linux_core_dumper_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
client/linux/minidump_writer/linux_core_dumper_unittest.cc:37: #include
"processor/logging.h"
On 2012/06/19 15:40:33, Mark Mentovai wrote:
> Why?

I cleaned it up. This is a leftover from my earlier attempts to log stuff.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
File client/linux/minidump_writer/linux_dumper_unittest_helper.cc (right):

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
client/linux/minidump_writer/linux_dumper_unittest_helper.cc:81:
google_breakpad::scoped_array<pthread_t> threads(new pthread_t[num_threads]);
On 2012/06/19 15:40:33, Mark Mentovai wrote:
> Good catch, but also unrelated to the main std::string thrust of this change.

Done. The std::string related changes were moved to
http://breakpad.appspot.com/400002/

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
File client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc:64: if
(getenv("bindir")) {
On 2012/06/19 15:40:33, Mark Mentovai wrote:
> Rather than potentially calling getenv twice, cache its result in a local
char*.
> 
> Same in minidump_writer_unittest.cc.

The requested changes are done. I moved the function to a new location
(minidump_writer_unittest_utils.cc) so that it can be shared.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/lin...
client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc:144: << "Error: "
<< strerror(errno) << std::endl;
On 2012/06/19 15:40:33, Mark Mentovai wrote:
> You need <errno.h> and <string.h> to do this.

Done.

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/min...
File client/linux/minidump_writer/minidump_writer_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/min...
client/linux/minidump_writer/minidump_writer_unittest.cc:78: string templ =
temp_dir.path() + "/minidump-writer-unittest";
On 2012/06/19 15:40:33, Mark Mentovai wrote:
> Don’t you need to #include your new header to do this?

The std::string related changes were moved to
http://breakpad.appspot.com/400002/

http://breakpad.appspot.com/399002/diff/2001/client/linux/minidump_writer/min...
client/linux/minidump_writer/minidump_writer_unittest.cc:216:
google_breakpad::scoped_array<char> buffer(new char[kMemorySize]);
On 2012/06/19 15:40:33, Mark Mentovai wrote:
> kMemorySize is confusingly-named. It is not a constant in the sense of
something
> that should be named with a k. If it were, the original form of this line
would
> not be a flexible-sized alloca-based array and would have been safe. As it
> stands, seeing you resort to a scoped_array to avoid a local array when the
size
> looks like it’s a constexpr looks wrong. kMemorySize ought to be renamed.

Done.

[Mark Mentovai]

http://breakpad.appspot.com/399002/diff/10018/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/10018/src/client/linux/minidump_write...
src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc:123:
ASSERT_NE(-1, fd) << "Failed to open file: " << helper_path << std::endl
It is uncommon to see endl in the middle of an ASSERT, and you never see it at
the end, because ASSERT takes care of the final newline for you.

http://breakpad.appspot.com/399002/diff/10018/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/10018/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer_unittest.cc:305: << "Failed to
execute: " << cmdline << std::endl;
No endl.

http://breakpad.appspot.com/399002/diff/10018/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer_unittest_utils.h (right):

http://breakpad.appspot.com/399002/diff/10018/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer_unittest_utils.h:42: string
GetHelperBinary();
This needs a comment explaining what it does.

http://breakpad.appspot.com/399002/diff/10018/src/common/language.h
File src/common/language.h (right):

http://breakpad.appspot.com/399002/diff/10018/src/common/language.h#newcode83
src/common/language.h:83: virtual ~Language() {}
Destructors shouldn’t follow functions or variables.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

http://breakpad.appspot.com/399002/diff/10018/src/common/linux/elf_core_dump_...
File src/common/linux/elf_core_dump_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/10018/src/common/linux/elf_core_dump_...
src/common/linux/elf_core_dump_unittest.cc:186: #if defined(__i386)
__i386__ is the modern name for this macro. Not all architectures define a macro
with just the underscores on the front side, but all define a macro with the
underscores on both sides.

http://breakpad.appspot.com/399002/diff/10018/src/common/linux/file_id_unitte...
File src/common/linux/file_id_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/10018/src/common/linux/file_id_unitte...
src/common/linux/file_id_unittest.cc:77: ASSERT_EQ(system(cmdline.get()), 0) <<
"Failed to execute: "
ASSERT_EQ takes arguments in the order expected, actual. These are reversed.

It prints out the expected and actual values and names them as such, so you
should fix this.

http://breakpad.appspot.com/399002/diff/10018/src/common/linux/file_id_unitte...
src/common/linux/file_id_unittest.cc:78: << cmdline.get() << std::endl;
endl, throughout this section.

http://breakpad.appspot.com/399002/diff/10018/src/common/test_assembler.h
File src/common/test_assembler.h (right):

http://breakpad.appspot.com/399002/diff/10018/src/common/test_assembler.h#new...
src/common/test_assembler.h:274: // A base class destructor should be either
public and virtual,
Blank line before this one.

http://breakpad.appspot.com/399002/diff/10018/src/tools/linux/md2core/minidum...
File src/tools/linux/md2core/minidump-2-core.cc (right):

http://breakpad.appspot.com/399002/diff/10018/src/tools/linux/md2core/minidum...
src/tools/linux/md2core/minidump-2-core.cc:54: #include "processor/scoped_ptr.h"
Sort. p < t.

[ivanpe.google]

Everything is fixed.

Thanks,
-Ivan

http://breakpad.appspot.com/399002/diff/10018/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/10018/src/client/linux/minidump_write...
src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc:123:
ASSERT_NE(-1, fd) << "Failed to open file: " << helper_path << std::endl
On 2012/06/30 12:55:41, Mark Mentovai wrote:
> It is uncommon to see endl in the middle of an ASSERT, and you never see it at
> the end, because ASSERT takes care of the final newline for you.

Done.

http://breakpad.appspot.com/399002/diff/10018/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/10018/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer_unittest.cc:305: << "Failed to
execute: " << cmdline << std::endl;
On 2012/06/30 12:55:41, Mark Mentovai wrote:
> No endl.

Done.

http://breakpad.appspot.com/399002/diff/10018/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer_unittest_utils.h (right):

http://breakpad.appspot.com/399002/diff/10018/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer_unittest_utils.h:42: string
GetHelperBinary();
On 2012/06/30 12:55:41, Mark Mentovai wrote:
> This needs a comment explaining what it does.

Done.

http://breakpad.appspot.com/399002/diff/10018/src/common/language.h
File src/common/language.h (right):

http://breakpad.appspot.com/399002/diff/10018/src/common/language.h#newcode83
src/common/language.h:83: virtual ~Language() {}
On 2012/06/30 12:55:41, Mark Mentovai wrote:
> Destructors shouldn’t follow functions or variables.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.

http://breakpad.appspot.com/399002/diff/10018/src/common/linux/elf_core_dump_...
File src/common/linux/elf_core_dump_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/10018/src/common/linux/elf_core_dump_...
src/common/linux/elf_core_dump_unittest.cc:186: #if defined(__i386)
On 2012/06/30 12:55:41, Mark Mentovai wrote:
> __i386__ is the modern name for this macro. Not all architectures define a
macro
> with just the underscores on the front side, but all define a macro with the
> underscores on both sides.

Fixed all references of __i386 and __x86_64 in this file only.  There a couple
of usages in other files, but it will be better to fix all of them in a separate
change.

http://breakpad.appspot.com/399002/diff/10018/src/common/linux/file_id_unitte...
File src/common/linux/file_id_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/10018/src/common/linux/file_id_unitte...
src/common/linux/file_id_unittest.cc:77: ASSERT_EQ(system(cmdline.get()), 0) <<
"Failed to execute: "
On 2012/06/30 12:55:41, Mark Mentovai wrote:
> ASSERT_EQ takes arguments in the order expected, actual. These are reversed.
> 
> It prints out the expected and actual values and names them as such, so you
> should fix this.

Done.

http://breakpad.appspot.com/399002/diff/10018/src/common/linux/file_id_unitte...
src/common/linux/file_id_unittest.cc:78: << cmdline.get() << std::endl;
On 2012/06/30 12:55:41, Mark Mentovai wrote:
> endl, throughout this section.

Done.

http://breakpad.appspot.com/399002/diff/10018/src/common/test_assembler.h
File src/common/test_assembler.h (right):

http://breakpad.appspot.com/399002/diff/10018/src/common/test_assembler.h#new...
src/common/test_assembler.h:274: // A base class destructor should be either
public and virtual,
On 2012/06/30 12:55:41, Mark Mentovai wrote:
> Blank line before this one.

Done.

http://breakpad.appspot.com/399002/diff/10018/src/tools/linux/md2core/minidum...
File src/tools/linux/md2core/minidump-2-core.cc (right):

http://breakpad.appspot.com/399002/diff/10018/src/tools/linux/md2core/minidum...
src/tools/linux/md2core/minidump-2-core.cc:54: #include "processor/scoped_ptr.h"
On 2012/06/30 12:55:41, Mark Mentovai wrote:
> Sort. p < t.

Done.

[Mark Mentovai]

http://breakpad.appspot.com/399002/diff/24002/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer_unittest_utils.cc (right):

http://breakpad.appspot.com/399002/diff/24002/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer_unittest_utils.cc:42: //
Returns the full path to linux_dumper_unittest_helper.  The full path is
You only need to write this in the .h file. Duplicating it here
is…duplication…and duplication is bad.

http://breakpad.appspot.com/399002/diff/24002/src/common/linux/file_id_unitte...
File src/common/linux/file_id_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/24002/src/common/linux/file_id_unitte...
src/common/linux/file_id_unittest.cc:75: google_breakpad::scoped_array<char>
cmdline(new char[4096]);
Why are you switching this to a scoped_array? What was wrong with this being on
the stack?

[ivanpe.google]

Fixed.

Thanks,
-Ivan

http://breakpad.appspot.com/399002/diff/24002/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer_unittest_utils.cc (right):

http://breakpad.appspot.com/399002/diff/24002/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer_unittest_utils.cc:42: //
Returns the full path to linux_dumper_unittest_helper.  The full path is
On 2012/07/02 18:54:14, Mark Mentovai wrote:
> You only need to write this in the .h file. Duplicating it here
> is…duplication…and duplication is bad.

Done.

http://breakpad.appspot.com/399002/diff/24002/src/common/linux/file_id_unitte...
File src/common/linux/file_id_unittest.cc (right):

http://breakpad.appspot.com/399002/diff/24002/src/common/linux/file_id_unitte...
src/common/linux/file_id_unittest.cc:75: google_breakpad::scoped_array<char>
cmdline(new char[4096]);
On 2012/07/02 18:54:14, Mark Mentovai wrote:
> Why are you switching this to a scoped_array? What was wrong with this being
on
> the stack?

I missed to clean this up.  Sorry.  The reason I changed it in the first place
was that it results in function frames which are too large (> 16K) and the
compilation breaks in Google.  Unfortunately, I was unable to enforce the same
warning using the "regular gcc" (probably there is a bug in gcc) so I decided to
revert all "large frame" related changes and to simply suppress this warning on
our side.  Apparently, I missed to revert this one.

[Mark Mentovai]

LGTM