Page MenuHomePhabricator

dsymutil: Follow references to clang modules and recursively clone the debug info
ClosedPublic

Authored by aprantl on Sep 21 2015, 5:15 PM.

Details

Reviewers
friss
Summary

This patch teaches llvm-dsymutil to follow references to clang modules and recursively clone the debug info therein.
This does not yet resolve external type references.

Diff Detail

Event Timeline

aprantl updated this revision to Diff 35323.Sep 21 2015, 5:15 PM
aprantl retitled this revision from to dsymutil: Follow references to clang modules and recursively clone the debug info.
aprantl updated this object.
aprantl added a reviewer: friss.
aprantl set the repository for this revision to rL LLVM.
aprantl added a subscriber: llvm-commits.
aprantl updated this revision to Diff 35327.Sep 21 2015, 5:23 PM
aprantl removed rL LLVM as the repository for this revision.

Got rid of ModuleDIEAlloc.

friss added inline comments.Sep 22 2015, 10:07 AM
test/tools/dsymutil/X86/modules.map
1–8 ↗(On Diff #35327)

The name of that file is a bit confusing because of the context it addresses. I'd like to see the actual module map file committed, and if you could rework that test to look like the other newer ones (and thus get rid of this map in favor of dummy-debug-map.map), this would be great.

test/tools/dsymutil/X86/modules.test
3 ↗(On Diff #35327)

Is that copying really necessary?

4–5 ↗(On Diff #35327)

I usually ditch the tmp file in favor of -o - | llvm-dwarfdump

tools/dsymutil/DwarfLinker.cpp
395

I think that for your work, this line would be sufficient in this loop. The rest will be needed for --update support, but I need to first figure out accelerator tables.

1114–1115

Can you do that refactoring in a preparatory NFC commit?

1452

StringSet?

1471–1473

Making that a static helper could be a separate NFC commit too, right?

2625–2631

Took me some time to remember why we need that (or rather why we didn't need it before). Usually, the last DIE is always a NULL. It can happen that the input file is just a single compile_unit DIE though and that would crash here because we don't have a terminating NULL. A comment explaining that we should do that optimization would be nice.

3093–3095

It might help the reader if there was a comment describing the layout of debug info containing a module reference so that it's trivial to understand how this function works.

3148

this should be sys::TimeValue::PosixZeroTime().

3162

That condition seems unlikely to trigger.

tools/dsymutil/dsymutil.h
31 ↗(On Diff #35327)

Separate NFC patch?

aprantl added inline comments.Sep 22 2015, 11:53 AM
tools/dsymutil/DwarfLinker.cpp
1114–1115

248310

1471–1473

248311

tools/dsymutil/dsymutil.h
31 ↗(On Diff #35327)

248312

aprantl updated this revision to Diff 35407.Sep 22 2015, 1:29 PM

Addressed remaining review feedback. Thanks!

friss edited edge metadata.Sep 22 2015, 1:52 PM

LGTM once you address one nit:

tools/dsymutil/DwarfLinker.cpp
2625

The next DIE isn't usually NULL, but it is usually valid because there's nearly always at least a NULL after it.

friss accepted this revision.Sep 22 2015, 1:52 PM
friss edited edge metadata.
This revision is now accepted and ready to land.Sep 22 2015, 1:52 PM
aprantl closed this revision.Sep 22 2015, 3:23 PM

Thanks, r248331.