Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(679)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 11 months ago by Ryan Sleevi
Modified:
11 years, 2 months ago
CC:
google-breakpad-dev_googlegroups.com
Base URL:
http://google-breakpad.googlecode.com/svn/trunk/src
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Python helper #

Total comments: 9

Patch Set 3 : #

Total comments: 10

Patch Set 4 : Rebased #

Patch Set 5 : Review feedback #

Patch Set 6 : Test fixes #

Patch Set 7 : Remove macpath #

Total comments: 1

Patch Set 8 : Slight cleanup #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M common/dwarf_cu_to_module.cc View 1 2 3 3 chunks +13 lines, -5 lines 0 comments Download
M common/dwarf_cu_to_module.h View 1 2 3 3 chunks +15 lines, -9 lines 0 comments Download
M common/dwarf_cu_to_module_unittest.cc View 1 2 3 4 5 10 chunks +34 lines, -17 lines 0 comments Download
M common/dwarf_line_to_module.cc View 1 2 3 4 5 6 7 3 chunks +16 lines, -11 lines 0 comments Download
M common/dwarf_line_to_module.h View 1 2 3 2 chunks +7 lines, -1 line 0 comments Download
M common/dwarf_line_to_module_unittest.cc View 1 2 3 4 5 19 chunks +73 lines, -25 lines 0 comments Download
M common/linux/dump_symbols.cc View 1 2 3 4 1 chunk +8 lines, -4 lines 0 comments Download
M common/mac/dump_syms.mm View 1 2 3 1 chunk +10 lines, -4 lines 0 comments Download
A tools/python/filter_syms.py View 1 2 3 4 5 6 1 chunk +204 lines, -0 lines 0 comments Download
A tools/python/tests/filter_syms_unittest.py View 1 2 3 1 chunk +138 lines, -0 lines 0 comments Download

Messages

Total messages: 32
Ryan Sleevi
Mark, Ted: I was hoping to get your thoughts on this patch. This changes the ...
11 years, 11 months ago #1
Ryan Sleevi
(Just republishing so google-breakpad-dev can be aware of the change)
11 years, 11 months ago #2
Mark Mentovai
I don’t think this is a good idea.
11 years, 11 months ago #3
Mark Mentovai
Absolute paths are meaningless garbage. What we want to do is come up with a ...
11 years, 11 months ago #4
Ryan Sleevi
On 2012/04/27 04:13:41, Mark Mentovai wrote: > Absolute paths are meaningless garbage. What we want ...
11 years, 11 months ago #5
Mark Mentovai
I mean… Do something so that all paths are relative to the directory where dump_syms ...
11 years, 11 months ago #6
Ryan Sleevi
On 2012/04/27 04:47:41, Mark Mentovai wrote: > I mean… > > Do something so that ...
11 years, 11 months ago #7
eroman
I asked Ryan for absolute paths here. My motivation is crash reports in Google Chrome. ...
11 years, 11 months ago #8
Mark Mentovai
The symbol files are huge and are slow to parse a character at a time. ...
11 years, 11 months ago #9
Mark Mentovai
(resolving the ..s in paths that have been made absolute like what Ryan’s showing here)
11 years, 11 months ago #10
Ryan Sleevi
On Apr 27, 2012 8:10 AM, <mark@chromium.org> wrote: > > The symbol files are huge ...
11 years, 11 months ago #11
Mark Mentovai
Piping to filter_syms would be fine. Let’s see it!
11 years, 11 months ago #12
Ted Mielczarek
On Fri, Apr 27, 2012 at 11:43 AM, Ryan Sleevi <rsleevi@chromium.org> wrote: > > On ...
11 years, 11 months ago #13
Ted Mielczarek
Upon further review, this is important and we should do this. Why? Because when paths ...
11 years, 10 months ago #14
Ted Mielczarek
11 years, 10 months ago #15
Mark Mentovai
OK, that’s fine, we can do this. I’d still like to see a filter_syms thing ...
11 years, 10 months ago #16
Ryan Sleevi
On 2012/05/24 16:45:35, Mark Mentovai wrote: > OK, that’s fine, we can do this. I’d ...
11 years, 10 months ago #17
Ryan Sleevi
On 2012/05/24 16:45:35, Mark Mentovai wrote: > OK, that’s fine, we can do this. I’d ...
11 years, 10 months ago #18
Ryan Sleevi
On 2012/04/27 15:45:18, Mark Mentovai wrote: > Piping to filter_syms would be fine. Let’s see ...
11 years, 6 months ago #19
Mark Mentovai
One comment on the python, otherwise this seems good. Are you also going to wire ...
11 years, 6 months ago #20
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 ...
11 years, 6 months ago #21
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 ...
11 years, 6 months ago #22
Ryan Sleevi
Mark & Marc-Antoine: Mind taking another look? In addition to addressing the comments and adding ...
11 years, 6 months ago #23
Marc-Antoine Ruel
Note breakpad does not use a PRESUBMIT.py file. I do not trust in unit tests ...
11 years, 6 months ago #24
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#newcode189 tools/python/filter_syms.py:189: 'mac': macpath }.get(options.path_handler, os.path) I don’t think macpath is ...
11 years, 6 months ago #25
Ted Mielczarek
Sorry for letting this sit for so long. http://breakpad.appspot.com/385001/diff/25004/common/dwarf_cu_to_module_unittest.cc File common/dwarf_cu_to_module_unittest.cc (right): http://breakpad.appspot.com/385001/diff/25004/common/dwarf_cu_to_module_unittest.cc#newcode1 common/dwarf_cu_to_module_unittest.cc:1: // ...
11 years, 6 months ago #26
Ryan Sleevi
Ted: Thanks for poking me about this. I added and updated the unit tests, as ...
11 years, 2 months ago #27
Mark Mentovai
macpath implements the pre-Mac OS X old-style pathname functions. You know, like… mark@cougar bash$ python ...
11 years, 2 months ago #28
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#newcode192 tools/python/filter_syms.py:192: 'posix': posixpath }.get(options.path_handler, os.path) Updated. I still ...
11 years, 2 months ago #29
Mark Mentovai
I’m not re-reviewing the entire thing, but LGTM based on my previous comments which indicate ...
11 years, 2 months ago #30
Ryan Sleevi
On 2013/01/16 23:17:20, Mark Mentovai wrote: > I’m not re-reviewing the entire thing, but LGTM ...
11 years, 2 months ago #31
Ted Mielczarek
11 years, 2 months ago #32
LGTM. I'll land this for you shortly. Thanks for finishing this up!
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1004:630ec63f810e-tainted