Page MenuHomePhabricator

Breakpad: auto-detect path style of file entries
ClosedPublic

Authored by labath on Thu, Feb 7, 7:39 AM.

Details

Summary

This adds support for auto-detection of path style to SymbolFileBreakpad
(similar to how r351328 did the same for DWARF). We guess each file
entry separately, as we have no idea which file came from which compile
units (and different compile units can have different path styles). The
breakpad generates should have already converted the paths to absolute
ones, so this guess should be reasonable accurate, but as always with
these kinds of things, it is hard to give guarantees about anything.

In an attempt to bring some unity to the path guessing logic, I move the
guessing logic from inside SymbolFileDWARF into the FileSpec class and
have both symbol files use it to implent their desired behavior.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

labath created this revision.Thu, Feb 7, 7:39 AM
lemo accepted this revision.Thu, Feb 7, 9:51 AM
This revision is now accepted and ready to land.Thu, Feb 7, 9:51 AM
clayborg accepted this revision.Thu, Feb 7, 10:12 AM
amccarth added inline comments.
include/lldb/Utility/FileSpec.h
250

Given that this implementation is limited to absolute paths ... should it be in such a general purpose class?

If so, maybe this restriction be made more obvious by naming the parameter absolute_path. Then people who just look at the signature without reading the comments, or who get a prompt in their IDE are more likely to note the limitation.

source/Utility/FileSpec.cpp
375

The code this replaces was returning Style::native rather than Style::None. I'm not sure if that will have any implications.

labath marked 2 inline comments as done.Fri, Feb 8, 1:52 AM
labath added inline comments.
include/lldb/Utility/FileSpec.h
250

Do you have an idea where to place it? Theoretically I could put it into the base SymbolFile class, since all existing users are SymbolFiles, but I am not sure if that makes things any better. Renaming the arg to absolute_path sounds good.

source/Utility/FileSpec.cpp
375

It terms of functionality, it doesn't, since all users then do getValueOr(Style::native). But I am not sure what's the impact on readability -- it seemed better to have a more explicit value for "I don't know" in such a general API, but since all users just revert to native when they can't guess (and I'm not sure if there's anything better they could do), maybe going through Optional is overkill. I'd be happy to change it if you think that's better.

labath updated this revision to Diff 185937.Fri, Feb 8, 3:06 AM

Rename arg to absolute_path.

amccarth accepted this revision.Fri, Feb 8, 8:06 AM

I think absolute_path is great. Thanks for checking on the native/None. I didn't want there to be any latent surprises.

I think absolute_path is great. Thanks for checking on the native/None. I didn't want there to be any latent surprises.

Thanks. BTW, I've added the auto-detection code to dwarf only a couple of weeks ago, so I think we're more likely to get latent surprises from that then from this move. :)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Feb 11, 6:10 AM
Herald added a subscriber: abidh. · View Herald Transcript