Include the compilation directory for FILE entries, making them absolute instead of relative

Include the compilation directory for FILE entries, making them absolute instead
of relative

When parsing the Line Info program, track the current compilation unit's
compilation directory, and ensure that is appended when reading
relative include_directories (6.2.4.10 of DWARF 2.0 STD) or when reading
file names (6.2.4.11) that indicate a directory of LEB128(0).

This effectively absolutizes all paths in the resultant Module*, and thus the
Breakpad symbol file produced by dump_syms.

BUG=477
TEST=tools/python/tests/filter_syms_unittest.py

[Ryan Sleevi]

Mark, Ted:

I was hoping to get your thoughts on this patch. This changes the FILE line of
(Mac/Linux) to be absolute (with maybe some host-specific details, depending on
gcc version & platform).

This brings the Mac/Linux dumpers into consistency with Win, where PDBs are
always absolute paths.

The goal is to make it easier to potentially replace the FILE paths with source
repository info. Ted, I believe you're already doing this for Socorro with
http://mxr.mozilla.org/mozilla/source/toolkit/crashreporter/tools/symbolstore.py
.

Tests are currently broken, but I wanted to validate the approach first, as it
definitely fixes the problem.

[Ryan Sleevi]

(Just republishing so google-breakpad-dev can be aware of the change)

[Mark Mentovai]

I don’t think this is a good idea.

[Mark Mentovai]

Absolute paths are meaningless garbage. What we want to do is come up with a way
to get relative paths, but make them relative to some consistent root.

[Ryan Sleevi]

On 2012/04/27 04:13:41, Mark Mentovai wrote:
> Absolute paths are meaningless garbage. What we want to do is come up with a
way
> to get relative paths, but make them relative to some consistent root.

Could you explain a bit more why you don't think it's valuable?

There's no guarantee that two compilation units may have some consistent root.
This is especially true when statically linked in third-party libraries with
debug symbols. Their common root may be, at best, /. 

Including the compilation directory at least provides a way for a client to do
meaningful filtering and interpretation. As it stands today, the FILE
information is today a complete and utter crapshoot dependent on build tool,
environment, and what you're building.

[Mark Mentovai]

I mean…

Do something so that all paths are relative to the directory where dump_syms is
run from. Then all of the paths are meaningful, but you don’t have all of the
meaningless garbage about the cwd, because who cares about that anyway? For
Chrome, we could just change into src, and all of the paths would wind up
completely meaningful.

[Ryan Sleevi]

On 2012/04/27 04:47:41, Mark Mentovai wrote:
> I mean…
> 
> Do something so that all paths are relative to the directory where dump_syms
is
> run from. Then all of the paths are meaningful, but you don’t have all of the
> meaningless garbage about the cwd, because who cares about that anyway? For
> Chrome, we could just change into src, and all of the paths would wind up
> completely meaningful.

That presumes that you're running dump_syms from the same machine in which you
built.

Having worked with a Breakpad+Socorro based system quite a bit prior to joining
Chrome, I can certainly say for some cases, this isn't true. Today, you can
reliably dump the symbols (on any platform) without having to worry about local
directories or checkouts.

I think any relativizing would not be the responsibility of dump_syms, but on
any post-processing steps (if any exist). Have dump_syms reliably reflect what
ever is in the DWARF/PDB debug info, and then fix that up in post-processing.
For example, some older GDB versions  have the paths as host:/path (I fail to
recall exactly what version), while newer ones just use /path. That sort of
logic I would think is best encapsulated in post-processing build tools, not in
dump_syms itself.

An example of post-processing steps that I would see could be done are:
 * Have a script fixup all the paths to be relative to some CWD (to save on sym
size - presumably your motivation?)
 * Have a script replace all of the file paths with source server information
(eg: /path/to/foo -> svn:svnserver:repo:path/to/foo:123 for
"svn://svnserver/repo/path/to/foo@123")
 * ???

These are things I wouldn't want to see in dump_syms, as I don't think it
reflects all the (non-Chromium) use cases.

[eroman]

I asked Ryan for absolute paths here.

My motivation is crash reports in Google Chrome.

Right now the path field being exported by breakpad for Mac Chrome is utterly
useless. These paths then show up as the filename in the crash report. Since the
paths are not relative to a known parent, it is impossible to programatically
tell where the file actually came from.

To illustrate this, here are some paths I took straight out of Chrome crash
reports.

../base/tuple.h    (src/base/tuple.h)
../base/callback.h    (src/base/callback.h)
../base/timer.h    (src/base/timer.h)
../../src/spaces.h    (src/v8/src/spaces.h)
../../src/objects-inl.h    (src/v8/src/objects-inl.h)
../platform/graphics/IntSize.h   
(src/third_party/WebKit/Source/WebCore/platform/graphics/IntPoint.h)
../base/memory/ref_counted.h    (src/base/memory/ref_counted.h)
../../WTF/wtf/text/StringImpl.h   
(src/third_party/WebKit/Source/WTF/wtf/text/StringImpl.h)
../../WTF/wtf/MessageQueue.h   
(src/third_party/WebKit/Source/WTF/wtf/text/MessageQueue.h)

You can see how the ancestry information was lost.

By contrast, other paths in the mac breakpad symbols show up as (more or less)
absolute, such as:

/b/build/slave/chrome-official-mac/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../platform/graphics/mac/GlyphPageTreeNodeMac.cpp

While not pretty, the path above does not have any loss of information, and it
is possible for tools to unambiguously infer the relative path from "src".

Having all the paths be relative to some specific root would certainly be great,
and I am in favor of it (in the case of Chrome, we would want "src/"). However
for generality would be simpler to just stick with absolute paths and not worry
about issues like what Ryan brought up.

I am cool with either approach, since they both solve my problem :-)

Thanks guys for working on this!

[Mark Mentovai]

The symbol files are huge and are slow to parse a character at a time. It’s not
really that well-designed a format. I’m trying to reduce the processor-time
burden by moving as much of the “get rid of extraneous garbage” step up front so
that it happens once before the symbol files go to the symbol server.

How ’bout this…

It sounds like the problem only occurs in practice for headers (at least for the
Mac build). What if we tack a deduping step onto dump_syms. We can resolve the
..s in paths and then fold identical FILE references into a single output FILE.
Ultimately, that should be a net win for symbol file size and parse time.

[Mark Mentovai]

(resolving the ..s in paths that have been made absolute like what Ryan’s
showing here)

[Ryan Sleevi]

On Apr 27, 2012 8:10 AM, <mark@chromium.org> wrote:
>
> The symbol files are huge and are slow to parse a character at a time.
> It’s not really that well-designed a format. I’m trying to reduce the
> processor-time burden by moving as much of the “get rid of extraneous
> garbage” step up front so that it happens once before the symbol files
> go to the symbol server.

To be clear, I am absolutely in favor of some sort of cleanup step prior to
storing the symbols. I just don't believe that step reasonably belongs in
dump_syms.

For the naive case, and for the way Chromium's symbol dumping works, I
would see:

dump_syms foo.dSYM | filter_syms.py > foo.break pad

Following the POSIXy model of many tools working together.

Having dump_syms do it means you need to dump from the compiler machine -
which not all projects do, and which is not a requirement today. There's
also, I believe, a fair bit of user-specific handling that may need to
occur when filtering, and I think the filter_syms[.py] approach can more
readily meet those needs.

Just look at Ted's script that I referenced to get an idea of some of the
post-processing that occurs.

>
> How ’bout this…
>
> It sounds like the problem only occurs in practice for headers (at least
> for the Mac build). What if we tack a deduping step onto dump_syms. We
> can resolve the ..s in paths and then fold identical FILE references
> into a single output FILE. Ultimately, that should be a net win for
> symbol file size and parse time.
>
> http://breakpad.appspot.com/385001/

[Mark Mentovai]

Piping to filter_syms would be fine. Let’s see it!

[Ted Mielczarek]

On Fri, Apr 27, 2012 at 11:43 AM, Ryan Sleevi <rsleevi@chromium.org> wrote:
>
> On Apr 27, 2012 8:10 AM, <mark@chromium.org> wrote:
>>
>> The symbol files are huge and are slow to parse a character at a time.
>> It’s not really that well-designed a format. I’m trying to reduce the
>> processor-time burden by moving as much of the “get rid of extraneous
>> garbage” step up front so that it happens once before the symbol files
>> go to the symbol server.
>
> To be clear, I am absolutely in favor of some sort of cleanup step prior to
> storing the symbols. I just don't believe that step reasonably belongs in
> dump_syms.
>
> For the naive case, and for the way Chromium's symbol dumping works, I would
> see:
>
> dump_syms foo.dSYM | filter_syms.py > foo.break pad
>
> Following the POSIXy model of many tools working together.
>
> Having dump_syms do it means you need to dump from the compiler machine -
> which not all projects do, and which is not a requirement today. There's
> also, I believe, a fair bit of user-specific handling that may need to occur
> when filtering, and I think the filter_syms[.py] approach can more readily
> meet those needs.
>
> Just look at Ted's script that I referenced to get an idea of some of the
> post-processing that occurs.

Just a couple of points of info:
a) The Windows dumping code de-duplicates file records, which reduced
the count of FILE lines in our main DLL from several hundred thousand
to a few thousand:
http://code.google.com/p/google-breakpad/source/browse/trunk/src/common/windo...
b) We (Mozilla) do our symbol dumping on our build machines
immediately after the build process (using the script you've pointed
out).

-Ted

[Ted Mielczarek]

Upon further review, this is important and we should do this. Why? Because when
paths are relative they're relative *to the directory where the compilation took
place*, which means that if you don't use the extra directory info the DWARF
gives you, you cannot reconstruct the path to the file later. We've had a bug
open on this for a while: https://bugzilla.mozilla.org/show_bug.cgi?id=631645
and jimb proposed basically this exact patch.

[Mark Mentovai]

OK, that’s fine, we can do this. I’d still like to see a filter_syms thing to
get rid of /b/garbage in the absolute path.

[Ryan Sleevi]

On 2012/05/24 16:45:35, Mark Mentovai wrote:
> OK, that’s fine, we can do this. I’d still like to see a filter_syms thing to
> get rid of /b/garbage in the absolute path.

Especially for Chromium, I agree. I haven't had a chance to work on this patch
yet in light of other things, but I'm hoping to get back to it RSN. I haven't
abandoned it :)

[Ryan Sleevi]

On 2012/05/24 16:45:35, Mark Mentovai wrote:
> OK, that’s fine, we can do this. I’d still like to see a filter_syms thing to
> get rid of /b/garbage in the absolute path.

Especially for Chromium, I agree. I haven't had a chance to work on this patch
yet in light of other things, but I'm hoping to get back to it RSN. I haven't
abandoned it :)

[Ryan Sleevi]

On 2012/04/27 15:45:18, Mark Mentovai wrote:
> Piping to filter_syms would be fine. Let’s see it!

New patchset uploaded. Python code, both for its trivial nature and the ability
to get all of the OS-specific path goodness via (posixpath, ntpath, macpath) if
needed. It should also be suitable for Ted's (and other's) SVN/SCM
post-processing steps.

[Mark Mentovai]

One comment on the python, otherwise this seems good. Are you also going to wire
this up for the Chrome bots to use?

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py
File tools/symfix.py (right):

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py#newcode105
tools/symfix.py:105: }.get(record_type, self._ParseLineRecord)
This would need to be updated if we ever added another line type to the format.
I think it’d be better to look for FILE and direct those to _ParseFileRecord,
look for numbers and give those to _ParseLineRecord, and give everything else to
_PassthruRecord.

[Mark Mentovai]

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py
File tools/symfix.py (right):

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py#newcode1
tools/symfix.py:1: #!/usr/bin/env python
Another comment on the python: “symfix” is too generic a name. I think you
should be more descriptive.

[Marc-Antoine Ruel]

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py
File tools/symfix.py (right):

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py#newcode57
tools/symfix.py:57: def __init__(self, input_stream, output_stream):
If __init__ accepted path_prefix as a parameter, you'd save a few lines of code.

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py#newcode62
tools/symfix.py:62: self.input_stream = input_stream
I'd group immutables first, then mutable members.

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py#newcode122
tools/symfix.py:122: return os.path.normpath(path)
Note that this doesn't get the native path case on case-insensitive but
case-preserving file systems.

http://src.chromium.org/viewvc/chrome/trunk/src/tools/isolate/trace_inputs.py...

You could get the OSX implementation of get_native_path_case() but that could be
overkill in your case and just use a tolower() when comparing, as you prefer.

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py#newcode165
tools/symfix.py:165: default = [],
default=[],

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py#newcode170
tools/symfix.py:170: 
Personally, I always:
if args:
  parser.error('Unknown argument: %s' % args)

This helps on typo errors.

[Ryan Sleevi]

Mark & Marc-Antoine: Mind taking another look?

In addition to addressing the comments and adding tests, I added the option for
the SymbolFileParser (and filter_syms.py) to take an argument indicating the
path format of the symbol file. Primarily, this was to ensure that the unit
tests were deterministic, and not relying on os.path behaviours.

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py
File tools/symfix.py (right):

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py#newcode105
tools/symfix.py:105: }.get(record_type, self._ParseLineRecord)
On 2012/09/04 15:58:06, Mark Mentovai wrote:
> This would need to be updated if we ever added another line type to the
format.
> I think it’d be better to look for FILE and direct those to _ParseFileRecord,
> look for numbers and give those to _ParseLineRecord, and give everything else
to
> _PassthruRecord.

Correct. The intent and ordering was to match the conditional of
BasicSourceLineResolver::LoadMapFromMemory from
src/processor/basic_source_line_resolver.cc, which handles the C++ side.

http://breakpad.appspot.com/385001/diff/16002/tools/symfix.py#newcode122
tools/symfix.py:122: return os.path.normpath(path)
On 2012/09/05 16:21:02, Marc-Antoine Ruel wrote:
> Note that this doesn't get the native path case on case-insensitive but
> case-preserving file systems.
> 
>
http://src.chromium.org/viewvc/chrome/trunk/src/tools/isolate/trace_inputs.py...
> 
> You could get the OSX implementation of get_native_path_case() but that could
be
> overkill in your case and just use a tolower() when comparing, as you prefer.

I think it may be overkill, and comparing case-insensitively for case-sensitive
filesystems seems like it will yield the wrong result, since distinct files may
be collapsed. Of course, anyone who has multiple files/directories
differentiated by case is, rightfully, mad, but it seems like the current
implementation of comparing as-is best preserves the existing Breakpad behaviour
(across all systems)

[Marc-Antoine Ruel]

Note breakpad does not use a PRESUBMIT.py file. I do not trust in unit tests
that are not run automatically. Maybe run it during the build process?

anyhow, lgtm for the python part.

http://breakpad.appspot.com/385001/diff/25004/tools/python/filter_syms.py
File tools/python/filter_syms.py (right):

http://breakpad.appspot.com/385001/diff/25004/tools/python/filter_syms.py#new...
tools/python/filter_syms.py:201: main()
sys.exit(main()) otherwise the return code is ignored.

[Mark Mentovai]

http://breakpad.appspot.com/385001/diff/25004/tools/python/filter_syms.py
File tools/python/filter_syms.py (right):

http://breakpad.appspot.com/385001/diff/25004/tools/python/filter_syms.py#new...
tools/python/filter_syms.py:189: 'mac': macpath }.get(options.path_handler,
os.path)
I don’t think macpath is correct or a good idea. Use posixpath. Or really, at
that point, path_handler is just os.path, right?

[Ted Mielczarek]

Sorry for letting this sit for so long.

http://breakpad.appspot.com/385001/diff/25004/common/dwarf_cu_to_module_unitt...
File common/dwarf_cu_to_module_unittest.cc (right):

http://breakpad.appspot.com/385001/diff/25004/common/dwarf_cu_to_module_unitt...
common/dwarf_cu_to_module_unittest.cc:1: // Copyright (c) 2010 Google Inc.
Feels like you ought to be adding a unit test here for your new
LineToModuleHandler method.

http://breakpad.appspot.com/385001/diff/25004/common/dwarf_line_to_module.cc
File common/dwarf_line_to_module.cc (right):

http://breakpad.appspot.com/385001/diff/25004/common/dwarf_line_to_module.cc#...
common/dwarf_line_to_module.cc:96: full_name = ExpandPath(name, full_name);
Using full_name for two different purposes seems confusing here. I'd use a
dir_name variable for the directory up above, and full_name down here.

http://breakpad.appspot.com/385001/diff/25004/common/dwarf_line_to_module.h
File common/dwarf_line_to_module.h (right):

http://breakpad.appspot.com/385001/diff/25004/common/dwarf_line_to_module.h#n...
common/dwarf_line_to_module.h:124: vector<Module::Line> *lines)
I don't see any changes to dwarf_line_to_module_unittest.cc in this patch. If
you change this you need to change that file to match.

http://breakpad.appspot.com/385001/diff/25004/common/linux/dump_symbols.cc
File common/linux/dump_symbols.cc (right):

http://breakpad.appspot.com/385001/diff/25004/common/linux/dump_symbols.cc#ne...
common/linux/dump_symbols.cc:203: std::string compilation_dir_;
I think you want to just use "string" here.

http://breakpad.appspot.com/385001/diff/25004/tools/python/filter_syms.py
File tools/python/filter_syms.py (right):

http://breakpad.appspot.com/385001/diff/25004/tools/python/filter_syms.py#new...
tools/python/filter_syms.py:144: file_index = long(file_info[0])
I believe it's more usual to use "int" in Python, since int and long do the same
thing, and long was removed in Python 3.

http://breakpad.appspot.com/385001/diff/25004/tools/python/filter_syms.py#new...
tools/python/filter_syms.py:156: """Determines if the current record type is a
Line record"""
FWIW, the parser in BasicSourceLineResolver simply checks this by noting when it
sees a FUNC line, and then trying to parse any line that follows a FUNC (but
doesn't start with a known keyword) as a line record.

http://breakpad.appspot.com/385001/diff/25004/tools/python/filter_syms.py#new...
tools/python/filter_syms.py:168: file_index = long(line_info[3])
int()

[Ryan Sleevi]

Ted: Thanks for poking me about this. I added and updated the unit tests, as
requested, along with the Python changes.

http://breakpad.appspot.com/385001/diff/25004/tools/python/filter_syms.py
File tools/python/filter_syms.py (right):

http://breakpad.appspot.com/385001/diff/25004/tools/python/filter_syms.py#new...
tools/python/filter_syms.py:189: 'mac': macpath }.get(options.path_handler,
os.path)
On 2012/09/10 13:42:34, Mark Mentovai wrote:
> I don’t think macpath is correct or a good idea. Use posixpath. Or really, at
> that point, path_handler is just os.path, right?

I'm fine having mac use posixpath, but part of the motivation was to explicitly
allow the path to be specified. The example being having running this script on
a Linux machine, de-duping Windows file paths (or vice versa). The pathname
handling is dependent upon the source where dump_syms was run, which may not be
the same.

Additionally, it makes for deterministic output from the unit tests

[Mark Mentovai]

macpath implements the pre-Mac OS X old-style pathname functions. You know,
like…

mark@cougar bash$ python
Python 2.7.2 (default, Jun 20 2012, 16:23:33) 
[GCC 4.2.1 Compatible Apple Clang 4.0 (tags/Apple/clang-418.0.60)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import macpath
>>> macpath.sep
':'

Ref. http://docs.python.org/2/library/macpath.html

That’s why I don’t think it’s correct or a good idea. To get native Mac paths on
any platform, just use posixpath, the same as Linux. Or did you have something
else in mind by specifying macpath?

If Windows is the only outlier and you want to avoid os.path to have portable
deterministic operation, then you should probably special-case Windows and
default to posixpath.

[Ryan Sleevi]

Ah. Updated.

http://breakpad.appspot.com/385001/diff/47004/tools/python/filter_syms.py
File tools/python/filter_syms.py (right):

http://breakpad.appspot.com/385001/diff/47004/tools/python/filter_syms.py#new...
tools/python/filter_syms.py:192: 'posix': posixpath }.get(options.path_handler,
os.path)
Updated. I still wanted to keep the os.path handling so as to avoid having to
have Windows users always having to "-t win32" under non-Cyg pythons.

[Mark Mentovai]

I’m not re-reviewing the entire thing, but LGTM based on my previous comments
which indicate that I reviewed it then and liked it except for the comments
which have now been addressed.

[Ryan Sleevi]

On 2013/01/16 23:17:20, Mark Mentovai wrote:
> I’m not re-reviewing the entire thing, but LGTM based on my previous comments
> which indicate that I reviewed it then and liked it except for the comments
> which have now been addressed.

Ted: Ping on this, since you were reporting it from the Firefox side as well?

And I don't have commit access to the Breakpad repository, so if it LGTY (or
Mark's LGTM works for you), could someone land?

[Ted Mielczarek]

LGTM. I'll land this for you shortly. Thanks for finishing this up!