Page MenuHomePhabricator

[dwarfdump] Make recursion affect the parent chain
ClosedPublic

Authored by JDevlieghere on May 23 2019, 5:16 PM.

Details

Summary

Currently, -r (recursion depth) doesn't affect -p (show parents). This patch fixes that and adds a test case.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.May 23 2019, 5:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 5:16 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
aprantl accepted this revision.May 23 2019, 7:15 PM

Thanks!

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
559 ↗(On Diff #201102)

if (DumpOpts.RecurseDepth && Depth >= DumpOpts.RecurseDepth)

562 ↗(On Diff #201102)

Depth + 1?

This revision is now accepted and ready to land.May 23 2019, 7:15 PM

Is this the desired behavior?

Currently, I assume, searching for a particular DIE offset dumps that DIE, its parent, and all its children. (& the recursion limit is entirely ignored?)

This patch, if I understand it, dumps from the CU to the target DIE until the recursion limit is reached. So you get some parent DIE of the one you searched for.

I would've probably thought that the entire parent chain would be dumped, and then the recursion limit would limit how deep into the children of the target DIE would be dumped.

Thoughts?

JDevlieghere marked 2 inline comments as done.May 23 2019, 8:11 PM

Is this the desired behavior?

Currently, I assume, searching for a particular DIE offset dumps that DIE, its parent, and all its children. (& the recursion limit is entirely ignored?)

Yup.

This patch, if I understand it, dumps from the CU to the target DIE until the recursion limit is reached. So you get some parent DIE of the one you searched for.

I would've probably thought that the entire parent chain would be dumped, and then the recursion limit would limit how deep into the children of the target DIE would be dumped.

Thoughts?

This is what happens currently (without the patch). The recursion depth only applies to children, and passing -p shows the whole parent chain.

I think there's a case to be made for both. With the patch, if you pass -p -c -r 2, you'll get two parents and two children, which is a little weird.

@aprantl since you requested this, WDYT?

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
559 ↗(On Diff #201102)

The recursion depth can be negative, so that wouldn't work.

562 ↗(On Diff #201102)

Makes sense!

I perceived the current behavior as a bug in a situation where I was doing dwarfdump --name=foo in a deeply nested DIE tree with inlined functions and I was annoyed at all the repetitive parent context making it impossible to see more than one result on a page. It's true that there may be situation where you'd want to limit the parent and children recursion depth individually.

If we take grep as an analogy, this patch implements -C, which is useful in it's own right, but in addition there is also -B and -A.

I'd have no problem with adding even more fine-grained --limit-parents and --limit-children options, if someone finds them useful, but I would also like to have the option that does both.

I'd have no problem with adding even more fine-grained --limit-parents and --limit-children options, if someone finds them useful, but I would also like to have the option that does both.

I take that back. I want some form of limiting both directions. I'd be fine with only adding a --limit-parents (or some other name) option. The combined option is redundant and probably not nearly as popular.

  • Add new flag -parent-recurse-depth.
  • Add new aliases -limit-children and -limit-parents.

I'll leave it to you two to - I just wanted to bring up the thought at least - thanks!

aprantl added inline comments.May 24 2019, 11:06 AM
llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
202 ↗(On Diff #201282)

To avoid an overproliferation of options, I think I'd be fine with just adding parent-recurse-depth and dropping the limit-* aliases.

209 ↗(On Diff #201282)

why do we use -1U here and check for 0 in the code? I suppose this will also work fine, but it's inconsistent.

This revision was automatically updated to reflect the committed changes.