Code Review: Issue 207 - a Solaris version of symbol dumper

[Alfred Peng]

Hi guys,

There is a patch for the symbol dumper on Solaris. Could you please help review it?

The issue: http://code.google.com/p/google-breakpad/issues/detail?id=207
The patch: http://code.google.com/p/google-breakpad/issues/attachment?aid=6364599641305236471&name=dump_symbolv1.patch

Any specification for the dump string format that can be referred to? The Linux source code seems to be the only source for that.

The sample output after applying the patch: http://code.google.com/p/google-breakpad/issues/attachment?aid=-5531453677087767210&name=output.txt

Some references for stab format on Solaris:
The stab example of file_id.o on Solaris: http://code.google.com/p/google-breakpad/issues/attachment?aid=3409672743353733861&name=file_id.stabs
The stab document: http://developers.sun.com/tools/cc/documentation/ss10_docs/stabs.pdf

As for the tool "symupload", the Linux one works well on Solaris.

Thanks & Best Regards,
-Alfred

[Mark Mentovai]

+#include "common/linux/md5.h"

Since this makes sense on platforms other than Linux, we should move
it into common. You can either make that and include it in your patch
and let me know what needs to be "svn move"d, or I'll do the "svn
move" and make the adjustments myself when I check this in for you.

>+// TODO(Alfred): Computer the stack parameter size.
>+// Parameter variables are immediately following the N_FUN symbol.
>+// Will need to parse the type information to get a correct size.

We really only need the parameter size for FPO-optimized code, and
this is all tied up with the debugging data for Windows x86. It
doesn't make a difference elsewhere.

>+ // There maybe a lot of duplicated entry for a function in the symbol,
>+ // only one of them can met this.

I don't understand this comment.

>+ } // for each line.

Style: should have two blank spaces between the last thing on a line
('}') and the opening slash of your comment.

Otherwise, this looks good.

Mark

Mark

[Mark Mentovai]

Alfred Peng wrote:
> Any specification for the dump string format that can be referred to? The
> Linux source code seems to be the only source for that.

Only the reader and writer source at this point - but we've got a few
versions of the writer source.

> The sample output after applying the patch:
> http://code.google.com/p/google-breakpad/issues/attachment?aid=-5531453677087767210&name=output.txt
>
> Some references for stab format on Solaris:
> The stab example of file_id.o on Solaris:
> http://code.google.com/p/google-breakpad/issues/attachment?aid=3409672743353733861&name=file_id.stabs

Why don't we check these in and add a "make test" target?

> As for the tool "symupload", the Linux one works well on Solaris.

Maybe we should move that out of linux too, then.

Mark

[Mark Mentovai]

One more thing I've been meaning to correct in the Linux symbol
dumper: the OS should be reported in the MODULE line as "linux", not
"Linux". The same goes for the Solaris dumper: lowercase for
consistency with the other OSes we dump.

Mark

[Alfred Peng]

Hi Mento,

Thanks for the review. I've just updated the patch: http://code.google.com/p/google-breakpad/issues/attachment?aid=7053500767982773222&name=dump_symbolv2.patch .

On 9/27/07, Mark Mentovai <mmen...@gmail.com> wrote:

+#include "common/linux/md5.h"

Since this makes sense on platforms other than Linux, we should move
it into common.  You can either make that and include it in your patch
and let me know what needs to be "svn move"d, or I'll do the "svn
move" and make the adjustments myself when I check this in for you.

 The patch depends on the "svn move" of md5.c and md5.h to src/common directory. And the file include of md5.c need to be changed accordingly: #include "common/md5.h".

>+// TODO(Alfred): Computer the stack parameter size.
>+// Parameter variables are immediately following the N_FUN symbol.
>+// Will need to parse the type information to get a correct size.

We really only need the parameter size for FPO-optimized code, and
this is all tied up with the debugging data for Windows x86.  It
doesn't make a difference elsewhere.

So to set the parameter size to 0 doesn't break anything on Solaris. Remove the comment here.

>+      // There maybe a lot of duplicated entry for a function in the symbol,
>+      // only one of them can met this.

I don't understand this comment.

This is part of the Linux code. I don't see the duplicated entry for a function on Solaris till now. Remove the comment.

> The sample output after applying the patch:
> http://code.google.com/p/google-breakpad/issues/attachment?aid=-5531453677087767210&name=output.txt

>
> Some references for stab format on Solaris:
> The stab example of file_id.o on Solaris:
> http://code.google.com/p/google-breakpad/issues/attachment?aid=3409672743353733861&name=file_id.stabs

Why don't we check these in and add a "make test" target?

"gmake test" added  on Solaris.

> As for the tool "symupload", the Linux one works well on Solaris.

Maybe we should move that out of linux too, then.

I agree that we should move this to a common place that Linux and Solaris can share.

Best Regards,
-Alfred

[Mark Mentovai]

OK, Alfred, for the test data, we'll need some precompiled file.
There's no guarantee that file_id.o as compiled on an end user's
system will dump to the same thing as the file_id.o that you used to
generate the test data. Can you supply some precompiled test data
(and, if different, the source used to generate it?) and use that for
your test? See how in src/tools/windows/dump_syms, we use prebuild
debugging data (dump_syms_regtest.pdb)?

Also, the path that md5.c uses to include md5.h needs to change to get
rid of "linux".

Mark

[Alfred Peng]

Hi Mento,

Just updated the patch: http://code.google.com/p/google-breakpad/issues/attachment?aid=-2773045904090685911&name=dump_symbolv3.patch
Also the precompiled file attached: http://code.google.com/p/google-breakpad/issues/attachment?aid=-672419123056480748&name=dump_syms_regtest.o

The patch makes use of the Windows test data on Solaris.

Thanks & Best Regards,
-Alfred