This is an archive of the discontinued LLVM Phabricator instance.

[llvm-debuginfo-analyzer] 02 - Driver and documentation
ClosedPublic

Authored by CarlosAlbertoEnciso on May 17 2022, 6:40 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
CarlosAlbertoEnciso requested review of this revision.May 17 2022, 6:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 6:40 AM
CarlosAlbertoEnciso edited projects, added debug-info; removed Restricted Project.May 17 2022, 6:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 6:41 AM
CarlosAlbertoEnciso edited the summary of this revision. (Show Details)May 17 2022, 7:04 AM
CarlosAlbertoEnciso set the repository for this revision to rG LLVM Github Monorepo.
CarlosAlbertoEnciso edited the summary of this revision. (Show Details)May 18 2022, 1:32 AM

Implementation for an Interval Tree (light tree data structure to hold intervals).

(in the patch description), I believe there should be a description for the second patch in the series, not the first.

Good job! I believe the tool will be very usable. Thank you very much.

I'm not a subject matter expert in the debug tools and LLVM's debug info area and therefore cannot be an official reviewer but I've read the code and my findings are below. They are just some nits and typos mostly.

llvm/docs/CommandGuide/llvm-dva.rst
239 ↗(On Diff #430030)

details?

510–511 ↗(On Diff #430030)

Why Class and Compile unit are started from a capital letter while all the other entities (array, call site, etc.) aren't?

527 ↗(On Diff #430030)

I'm not a language expert, but I believe there should be "A union".

563 ↗(On Diff #430030)

Pointer Type is here but Reference type (not Type) is below. Also Import Module (not Module) is on the previous line.

1467 ↗(On Diff #430030)

Two formats generated by 3 compilers.

1918 ↗(On Diff #430030)
1937 ↗(On Diff #430030)

To use the same order for the words.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVElement.h
28

Does it make sense to mark the LVElement class as final?

llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
18

Let's include <string> to make the file self contained.

54

The virtual destructor is required, isn't it?

llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
24

Let's include <set> and <string> to make the file self contained.

194

the closed bracket is missed.

221
237

kinds or there will be a single kind only? There are a bunch of kind sets below, though.

301–304

Just for interest, why do you explicitly define these default members?

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
20

Let's include <set> to make the file self contained.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h
30

Can we use constexpr here (I'm not sure is it available in C++14)?

44
49
52
72
llvm/lib/DebugInfo/LogicalView/LLVMBuild.txt
22

An empty line should be added at EOF

llvm/tools/llvm-dva/Options.cpp
83 ↗(On Diff #430030)
249 ↗(On Diff #430030)
257 ↗(On Diff #430030)
394–395 ↗(On Diff #430030)

Both options are described as Import declaration. Is this actual?

465 ↗(On Diff #430030)

Case and Regex parameters are bools and can be passed by value, why are they passed by reference?

468 ↗(On Diff #430030)

StringRef(Pattern).lower() changes the string in the List or I'm missing something. Is it expected?

476 ↗(On Diff #430030)

I believe it can be replaced with std::copy or the corresponding overload of the insert method of the Set could be used.

llvm/tools/llvm-dva/Options.h
27 ↗(On Diff #430030)

Should the class be marked as final?

llvm/tools/llvm-dva/llvm-dva.cpp
9 ↗(On Diff #430030)
71 ↗(On Diff #430030)
jryans added a subscriber: jryans.May 22 2022, 11:09 AM

I've only read through the docs so far. This is looking great overall, and I'm excited for it to land. 😄

llvm/docs/CommandGuide/llvm-dva.rst
19 ↗(On Diff #430030)

"MacOS" is not an object file format, so probably you mean "Mach-O" here?

147–148 ↗(On Diff #430030)

Nit: These sentences read a bit strangely

CarlosAlbertoEnciso edited the summary of this revision. (Show Details)Jun 17 2022, 4:41 AM

Implementation for an Interval Tree (light tree data structure to hold intervals).

(in the patch description), I believe there should be a description for the second patch in the series, not the first.

Thanks. Very good catch.

Good job! I believe the tool will be very usable. Thank you very much.

I'm not a subject matter expert in the debug tools and LLVM's debug info area and therefore cannot be an official reviewer but I've read the code and my findings are below. They are just some nits and typos mostly.

First of all, thanks for all the time you are spending in the reviews. Very much appreciated.

@CarlosAlbertoEnciso You are welcome! I'll be glad if my modest efforts help you to land this amazing tool.

llvm/docs/CommandGuide/llvm-dva.rst
19 ↗(On Diff #430030)

You are correct. Replaced with Mach-O.

147–148 ↗(On Diff #430030)

Replaced with your suggestion.

239 ↗(On Diff #430030)

May be used when a more low level details are required.

510–511 ↗(On Diff #430030)

It should be lowercase: class and compile unit.

527 ↗(On Diff #430030)

You are correct. It should be A union.

563 ↗(On Diff #430030)

It should be pointer type and Import module.

1467 ↗(On Diff #430030)

Fixed incorrect sentence.

1918 ↗(On Diff #430030)

Fixed the typo.

1937 ↗(On Diff #430030)

Goot catch. Changed.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
194

Added closed bracket.

221

Fixed typo.

237

It should be kinds.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h
44

Added const.

49

Added const.

52

Added const.

72

Added const.

llvm/lib/DebugInfo/LogicalView/LLVMBuild.txt
22

Added empty line.

llvm/tools/llvm-dva/Options.cpp
83 ↗(On Diff #430030)

Fixed typo.

249 ↗(On Diff #430030)

Changed to lowercase.

257 ↗(On Diff #430030)

Changed to lowercase.

llvm/tools/llvm-dva/llvm-dva.cpp
9 ↗(On Diff #430030)

Fixed typo.

llvm/docs/CommandGuide/llvm-dva.rst
239 ↗(On Diff #430030)

Even better: used when lower level of detail is required.

llvm/docs/CommandGuide/llvm-dva.rst
239 ↗(On Diff #430030)

Replaced with: used when lower level detail is required.

psamolysov added inline comments.Jun 17 2022, 8:07 AM
llvm/docs/CommandGuide/llvm-dva.rst
239 ↗(On Diff #430030)

The proposed wording looks good for me. Thank you.

probinson added inline comments.Jun 17 2022, 8:43 AM
llvm/docs/CommandGuide/llvm-dva.rst
239 ↗(On Diff #430030)

when a lower level of detail is required.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVElement.h
28

Marking LVElement as final it would be OK for the current patch. But then we need to remove it in the next patch. Another point is that the current patch does not create any instances of LVElement.

It is just the driver and documentation.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
18

Including <string>.

llvm/tools/llvm-dva/Options.cpp
465 ↗(On Diff #430030)

Not a particular reason. Passing them by value.

llvm/tools/llvm-dva/Options.h
27 ↗(On Diff #430030)

Marked as final.

llvm/docs/CommandGuide/llvm-dva.rst
239 ↗(On Diff #430030)

Final sentence:
They are intended when a lower level of detail is required.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
54

I see your point. What this patch adds is just the driver and the documentation. No logical elements are created; basically no instances of LVObject, LVElement, LVLine, `LVScope or LVSymbol are created.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
24

<string> already included by LVObject.h. See previous change.
<set> already included by LVElement.

May be I am not understanding your point. Do you prefer those includes to be here instead of being include indirectly?

301–304

Just to explicitly state that these members use the default implementation.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
20

<set> already included by LVElement.

May be I am not understanding your point. Do you prefer this include to be here instead of being include indirectly?

llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h
30

But then BadIndex should be declared as static. Changed to static constexpr size_t BadIndex.

llvm/tools/llvm-dva/Options.cpp
394–395 ↗(On Diff #430030)

clEnumValN(LVTypeKind::IsImport, "Import", "Import declaration."),
should be
clEnumValN(LVTypeKind::IsImport, "Import", "Import."),

468 ↗(On Diff #430030)

The behaviour is correct. The string is converted to lowercase. May be the issue is the parameter name Case that controls the conversion.
Changed the parameters to IgnoreCase and UseRegex.

476 ↗(On Diff #430030)

Replaced with std::copy(List.begin(), List.end(), std::inserter(Set, Set.begin()));.

llvm/tools/llvm-dva/llvm-dva.cpp
71 ↗(On Diff #430030)

Replaced with empty().

psamolysov added inline comments.Jun 23 2022, 4:32 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
24

Yes, the idea was to make the header more independent from all the other ones including the LVElement.h and LVObject ones but if this makes no sense (the header defined things that depends on the defined in LVElement.h things and those things cannot be just forward declared) and the <set>, <string> and the other headers are already included there, there is no reason to include them directly.

I've added numerous similar comments for the other headers in next patches, you can just ignore them if they also make no sense. Sorry.

301–304

Thank you for the answer. I see no problem with such declarations.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
20

Yes, the idea was to make the header more independent from all the other ones including the LVElement.h one but if this makes no sense (the header defined things that depends on the defined in LVElement.h things and those things cannot be just forward declared) and the <set> and the other headers are already included there, there is no reason to include them directly.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
24

Thanks for the explanation. Based on it, I am happy to include those header files.

301–304

Thanks.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
20

Thanks for the explanation. Based on it, I am happy to include those header files.

@CarlosAlbertoEnciso You are welcome! I'll be glad if my modest efforts help you to land this amazing tool.

@psamolysov Your reviews are very valuable and I appreciate your opinion about the tool. Thanks.

  • Addressed the reviewer’s feedback.
  • Tool renamed as: llvm-debuginfo-analyzer.
CarlosAlbertoEnciso retitled this revision from [llvm-dva] 02 - Driver and documentation to [llvm-debuginfo-analyzer] 02 - Driver and documentation.Jul 25 2022, 2:16 AM
CarlosAlbertoEnciso edited the summary of this revision. (Show Details)
probinson added inline comments.Sep 16 2022, 9:29 AM
llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst
3

Is the line of = supposed to be exactly as long as the preceding line? It looks like there's one extra character.

127
277
341
379
450
472
483
504
538
556
636
646
668

is -> could be (?)

678
712

(two years from now it won't be "recent" anymore)

llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
44

True if it has _not_ been named? that's inconsistent with the function name.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
50

It looks like you're using a std::set as if it were a BitVector? Seems inefficient... although functionally equivalent.
FYI, the current trend is to use TableGen for command-line options in tools.

I won't insist you change anything at this point, but in general the TableGen approach seems to reduce the amount of hand-coding required. You might want to investigate this later.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h
93

Does this really need to be virtual? If so, the destructor should also be virtual?

llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst
3

Yes. They should be the same.
If the line of =:

  1. Is longer: no warnings or errors.
  2. Is shorter: you get the warning: WARNING: Title underline too short.

Double check and both lines are the same length.

127

Added of.

277

Added of.

341

Added of.

379

Added of.

450

Added of.

472

Removed ,.

483

Added of.

504

Added of`.

538

Added of.

556

Added of.

636

Changed to detect.

646

Added of.

668

Taking your suggestion.

678

Added of.

712

Good point. Removed a recent version of.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
44

Changed to // True if the scope has been named.
Later patches will update the comment to include type and line number.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
50

Thanks for the information about TableGen being used for command line options.
Very interesting area to investigate. It seems that a quite few tools are using that approach:
llvm-objcopy, llvm-objdump, llvm-readobj, llvm-lipo, llvm-size, etc.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h
93

It does not need to be virtual. Removed virtual.

Address points raised by @probinson.

This revision is now accepted and ready to land.Sep 23 2022, 7:52 AM

Updated patch after '01 Interval Tree' was submitted.

Updated patch the removes some redundant public: modifiers.

@psamolysov do you have any additional comments on this patch? I would like to land it after @probinson accepted the patch. Thanks.

@CarlosAlbertoEnciso No, I haven't. Thank you very much for your amazing work!

This revision was landed with ongoing or failed builds.Oct 17 2022, 5:49 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Oct 17 2022, 6:29 AM

When you reland this, if you could cherry-pick https://github.com/nico/llvm-project/commit/5c9c504b48d3f0acbf01a8e28af9bc9766f66ec1 into the commit that relands this, that'd be cool. (But if it's too much trouble, not doing it is fine too of course.)

barannikov88 added inline comments.
llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
54

Is it going to be added in a separate patch? Currently, the absence of the virtual destructor causes compilation warnings.
The vtable should also be pinned to a single translation unit; the destructor could be used as an anchor.

When you reland this, if you could cherry-pick https://github.com/nico/llvm-project/commit/5c9c504b48d3f0acbf01a8e28af9bc9766f66ec1 into the commit that relands this, that'd be cool. (But if it's too much trouble, not doing it is fine too of course.)

@thakis I am happy to do the cherry pick as well as part of the reland.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h
54

The commit has been reverted and an updated patch will be commited later on, that fixes the 2 issues that caused buildbot errors.
Thanks

When you reland this, if you could cherry-pick https://github.com/nico/llvm-project/commit/5c9c504b48d3f0acbf01a8e28af9bc9766f66ec1 into the commit that relands this, that'd be cool. (But if it's too much trouble, not doing it is fine too of course.)

@thakis I am happy to do the cherry pick as well as part of the reland.

@thakis I am sorry but I missed the cherry-pick when I relanded.

Originally committed in fe7a3cedf77125a6309150d85cecbc20b1a31775

Reverted in 26dd64ba9cfabe5474bb207f3b7099965f81fed7

Buildbot failures:
https://lab.llvm.org/buildbot#builders/139/builds/29663

  • unittest trigger an invalid assertion.

https://lab.llvm.org/buildbot#builders/196/builds/19665

  • 'has virtual functions but non-virtual destructor' warning as error.

Recommitted with fix in c28a977b87defd2f37fd0808d7ba6173133744ce:

  • Removed the assertion.
  • Added virtual destructor.

@psamolysov , @probinson Thanks very much for all your valuable reviews.

The following patch fixed some linking issues when building shared libraries: https://reviews.llvm.org/D136159

CarlosAlbertoEnciso edited the summary of this revision. (Show Details)Nov 14 2022, 4:01 AM