This is an archive of the discontinued LLVM Phabricator instance.

[llvm-symbolizer] Introduce the -dsym-hint option.
ClosedPublic

Authored by glider on Oct 9 2014, 9:59 AM.

Details

Reviewers
samsonov
Summary

llvm-symbolizer will consult one of the .dSYM paths passed via -dsym-hint if it fails to find the .dSYM bundle at the default location.

Diff Detail

Event Timeline

glider retitled this revision from to [llvm-symbolizer] Introduce the -dsym-hint option. llvm-symbolizer will consult one of the .dSYM paths passed via -dsym-hint if it fails to find the .dSYM bundle at the default location..Oct 9 2014, 9:59 AM
glider updated this object.
glider edited the test plan for this revision. (Show Details)
glider added a reviewer: samsonov.
glider added a subscriber: Unknown Object (MLST).
glider updated this revision to Diff 14660.Oct 9 2014, 9:59 AM
glider retitled this revision from [llvm-symbolizer] Introduce the -dsym-hint option. llvm-symbolizer will consult one of the .dSYM paths passed via -dsym-hint if it fails to find the .dSYM bundle at the default location. to [llvm-symbolizer] Introduce the -dsym-hint option..
glider updated this object.
samsonov edited edge metadata.Oct 9 2014, 7:11 PM

Sorry, wasn't able to carefully review it today. Some early feedback.

Note that you will need tests in test/DebugInfo/ (add actual .dSYM bundles and macho files you're trying to symbolize under test/DebugInfo/, preferrably with sources and comments describing how to build them).

LLVMSymbolize.cpp
215 ↗(On Diff #14660)

Why all this complexity? Can you just change this function to take .dSYM bundle, and call it either on file from .dSYM hints argument, or from PathToExe + ".dSYM"?

329 ↗(On Diff #14660)

This can be a static helper, pass Opts.DynamicHints here.
Output parameters ("Paths") should better go last in argument list.

llvm-symbolizer.cpp
34 ↗(On Diff #14660)

Please update docs/CommandGuide/llvm-symbolizer.rst

130 ↗(On Diff #14660)

Additional method looks like an overkill. Opts is a struct, you can just call

Opts.DsymHints.assign(ClDsymHint.begin(), ClDsymHint.end());

Don't you think the tests for commandline args should go into tets/tools/llvm-symbolizer instead?

LLVMSymbolize.cpp
215 ↗(On Diff #14660)

We probably don't want to try those paths from -dsym-hints that don't end with ".dSYM"

329 ↗(On Diff #14660)

Agreed.

llvm-symbolizer.cpp
34 ↗(On Diff #14660)

Done

130 ↗(On Diff #14660)

Done

glider updated this revision to Diff 14727.Oct 10 2014, 7:10 AM
glider edited edge metadata.

Updated the diff. Haven't finished the tests yet though.

glider added inline comments.Oct 10 2014, 7:31 AM
LLVMSymbolize.cpp
215 ↗(On Diff #14660)

Hmm, the following workflow still doesn't work:

$ clang foo.c -g -c && clang foo.o -g -o foo
$ dsymutil foo -o bar.dSYM

Turns out the DWARF binary for foo is at bar.dSYM/Contents/Resources/DWARF/foo.

329 ↗(On Diff #14660)

Actually, no. This function needs Opts.DsymHints.

Sorry the last comment has been lost in time. The updated diff fixes the problem mentioned in http://reviews.llvm.org/D5705?id=14660#inline-47195

I'm not sure if adding user-facing Verbosity option is a good idea, in this patch it looks merely like a debugging aid.

If we decide to introduce it, I'd prefer a separate patch, with a careful audit of all existing code and reasonable decisions of what and when to print (for instance, it makes sense to print our steps in attempt to resolve .gnu_debuglink).

docs/CommandGuide/llvm-symbolizer.rst
18

Existing options are already listed below (after EXAMPLE)

31

Please specify that this flag may be provided multiple times and that it's relevant only for Mac OS.

tools/llvm-symbolizer/LLVMSymbolize.cpp
217

Looks like unfinished sentence.

303–346

Looks like this routine is generally usable and should reside somewhere in libLLVMObject. At the very least, add a FIXME about it.

304

dyn_cast may return null. Either handle this, or take MachOObjectFile as an input argument.

319

static

328

I still think this should be a static helper, which should take Opts.DsymHints as an argument. Also, consider checking --dsym_hint values in the main binary (probably, even reporting an error is incorrect value is passed there), so that "Opts.DsymHints" are always reasonable.

330

Variable names should start with capital letter.

334

const auto &path

345

DsymPaths

347–348

Please get rid of Bin/DbgBin here, use local variables with small scope where appropriate instead.

351–352

So, you have this sequence scattered throughout the code (at least three times):

OwningBinary<Binary> B = std::move(BinaryOrErr.get());
Bin = B.getBinary().get();
Obj = getObjectFileFromBinary(Bin, ArchName);

Can it be simplified or moved to a function?

361

just use "else if" here.

363

if (!DbgObj) ?

367

You can use EC.message() - this can be a different kind of error.

tools/llvm-symbolizer/LLVMSymbolize.h
66

BinaryPair is unused now.

91

typedef is not really needed now in C++11 world. ObjectPairForPathArchTy::iterator can be replaced with auto.

Addressed the comments.
WDYT is better: a) commit the binaries and .dSYM files for testing to the repository, or b) generate them on the fly?

docs/CommandGuide/llvm-symbolizer.rst
18

Indeed. Sorry, I've missed those.

31

Done.

tools/llvm-symbolizer/LLVMSymbolize.cpp
217

Good catch, fixed.

303–346
304

Done

319

Done

328

I've moved the check for ".dSYM" extension to the main binary, so this function has become really small now.
I think it's ok to make it a private member, or otherwise I can move it into lookUpDsymFile.

330

Done (hope I've fixed them all)

334

done

345

done

347–348

Moved them 1-2 scopes down.

351–352

I've tried to compactify these a bit.
But moving just these three lines into a separate function doesn't work because B is passed to addOwningBinary() later.
However it's not always correct to include addOwningBinary() into that function, because the former isn't called after every sequence you're talking about.
We won't get much from moving two of these three lines into a function OTOH.

361

Removed Opts.Verbose instead.

363

Fixed

367

Ok, but I've removed Opts.Verbose.

tools/llvm-symbolizer/LLVMSymbolize.h
66

Removed.

91

Removed typedefs except for ObjectPair which is used in several class members' declarations.

glider updated this revision to Diff 14804.Oct 13 2014, 9:47 AM

Addressed Alexey's comments

I think we should check in .dSYM binaries - IIUC we can't generate these files unless we have dsymutil installed in the system (and we certainly don't have it on Linux).

test/tools/llvm-symbolizer/Inputs/dsym-test.c
2

Please provide a comment with sequence of compiler / dsymutil invocations yielding the all the binaries you check in to Inputs/

samsonov added inline comments.Oct 13 2014, 12:14 PM
test/tools/llvm-symbolizer/Inputs/dsym-test.c
3

Do you need to add lit.local.cfg to this directory to exclude .c files from testsuite?

tools/llvm-symbolizer/LLVMSymbolize.cpp
100–101

SymbolIterator

326

Is it OK for a binary to not have UUID command? I think that if the object is well-formed, but doesn't have UUID command, you shouldn't use error() wrapper, as it prints error messages to stderr.

333

Probably it's ok to even sink this function down to lookUpDsymFile (I don't see how it can be generally useful).

336

ResourcePath (or just get rid of local variable).

346

This variable is not used.

352–357
if (auto MachObj = dyn_cast<const MachOObjectFile>(Obj))
  DbgObj = ...;
353

Please check that the code works if Obj is nullptr

tools/llvm-symbolizer/LLVMSymbolize.h
93

Thanks for the typedef cleanup. You may want to commit it separately as a cleanup change before the main patch.

128–129

Please define each member on a separate line.

tools/llvm-symbolizer/llvm-symbolizer.cpp
132

You may want to print the invalid flag value here.

glider added inline comments.Oct 14 2014, 8:12 AM
test/tools/llvm-symbolizer/Inputs/dsym-test.c
2

Done

3

I think no, lit doesn't attempt to run this file.

tools/llvm-symbolizer/LLVMSymbolize.cpp
100–101

Fixed in r219682.

326

No, it's not ok. However this code has changed in the latest version of the patch.

333

Done

336

Done

346

Removed.

352–357

Done

353

It should. I've inserted a return statement just for the case.

tools/llvm-symbolizer/LLVMSymbolize.h
93

Done in r219682.

128–129

Fixed in r219682.

tools/llvm-symbolizer/llvm-symbolizer.cpp
132

Done.

glider updated this revision to Diff 14868.Oct 14 2014, 8:16 AM

Addressed the comments.

samsonov accepted this revision.Oct 14 2014, 11:25 AM
samsonov edited edge metadata.

LGTM, once the following minor comments are addressed.

Please run the patch through clang-format-diff before submitting. Thanks for working on this!

tools/llvm-symbolizer/LLVMSymbolize.cpp
320

s/path/Path

351–352

Do you need this check now? Looks like it's enough to check that Obj is a MachOObjectFile (and you won't need "Bin" variable in this case).

355

We still need to cache (nullptr, nullptr) pair in ObjectPairForPathArch. For example, if the path is invalid and provided 50 times, we don't want to print diagnostics 50 times in error() function. So, at least in the way the code is structured now, we'll have to increase the indentation.

This revision is now accepted and ready to land.Oct 14 2014, 11:25 AM
glider added inline comments.Oct 16 2014, 5:12 PM
tools/llvm-symbolizer/LLVMSymbolize.cpp
320

Done

351–352

Removed the check.

355

Added code that puts (nullptr, nullptr) into the map. Don't think increasing the indentation is better here.

glider updated this revision to Diff 15055.Oct 16 2014, 5:15 PM
glider edited edge metadata.

Fixed the comments.

glider closed this revision.Oct 16 2014, 6:00 PM

r220004, thank you!