558319 - library files without corresponding sources in google-breakpad

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Mike Hommey [:glandium]]

I spotted these new files in the 3.6.3plugin1 source code:
./toolkit/crashreporter/google-breakpad/src/third_party/linux/lib/gflags/libgflags.a
./toolkit/crashreporter/google-breakpad/src/third_party/linux/lib/glog/libglog.a
./toolkit/crashreporter/google-breakpad/src/client/mac/gcov/libgcov.a

while at it, there was already a binary file before:
./toolkit/crashreporter/google-breakpad/src/tools/solaris/dump_syms/testdata/dump_syms_regtest.o

Comment 1

[(not currently active) Ted Mielczarek]

We're not actually linking with any of these in Mozilla, FWIW. I filed an upstream bug on these a while ago:
http://code.google.com/p/google-breakpad/issues/detail?id=360

I think they must be unused now, since the code builds just fine on x86-64 and ARM, but maybe they never got removed from the repo.

Comment 2

[(not currently active) Ted Mielczarek]

(In reply to comment #0)
> while at it, there was already a binary file before:
> ./toolkit/crashreporter/google-breakpad/src/tools/solaris/dump_syms/testdata/dump_syms_regtest.o

AFAIK this is just testdata, and the source is right next to it:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/tools/solaris/dump_syms/testdata/dump_syms_regtest.cc

so I don't see why it's a problem.

Comment 3

[Mike Hommey [:glandium]]

(In reply to comment #2)
> AFAIK this is just testdata, and the source is right next to it:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/tools/solaris/dump_syms/testdata/dump_syms_regtest.cc
> 
> so I don't see why it's a problem.

Usually, you don't want object files to be checked in and distributed alongside the source code. Though this one is less problematic as there is the corresponding source.

Comment 4

[Jim Blandy :jimb]

It would be nice if an interested party were interested in converting the Solaris dumper to use the src/common code now.  If you substitute <stdio.h>'s FILE for the file descriptor, I think src/common/linux/dump_symbols.h might just drop right in.

Comment 5

[Jim Blandy :jimb]

This would get Solaris DWARF support, which is the default since Sun Studio 11 (and perhaps 10, the compiler manual was confusing).

Comment 6

[Jim Blandy :jimb]

(In reply to comment #3)
> Usually, you don't want object files to be checked in and distributed alongside
> the source code. Though this one is less problematic as there is the
> corresponding source.

Actually, in this case the test depends on the exact series of STABS entries in the .o file.  You don't want to regenerate that, because new compilers might generate slightly different stabs. So if one is going to test that way (which I *don't* think is the best way to test), then having that exact binary checked into the tree is the right thing.

Comment 7

[Gervase Markham [:gerv]]

Jim: can you file a new bug on the issue you've raised in comments 4, 5 and 6?

Mike: are you OK to chase the upstream bug instead of us? :-)

Gerv

Comment 8

[Mike Hommey [:glandium]]

(In reply to comment #7)
> Mike: are you OK to chase the upstream bug instead of us? :-)

I'm probably not the best choice for that, I don't even build the crashreporter...

Comment 9

[Jim Blandy :jimb]

I think this bug is fine to represent the work.  It's been filed upstream here:
http://code.google.com/p/google-breakpad/issues/detail?id=360

The patch (awaiting revision by Google) is here:
http://breakpad.appspot.com/63002

The point of comment 6 is that there's no work to be done here: that binary file is appropriate to be checked into the tree, because it is test data that should not be regenerated.

Comment 10

[Jim Blandy :jimb]

I've put pings in the upstream patch and bug.

Comment 11

[Gabriele Svelto [:gsvelto]]

This was fixed upstream.