[llvm-dva] 02 - Driver and documentation

[CarlosAlbertoEnciso]

Summary

llvm-dva (debug information visual analyzer)

For more information, see the RFC https://discourse.llvm.org/t/llvm-dev-rfc-llvm-dva-debug-information-visual-analyzer/62570

Patches:
01 - Interval Tree
02 - Driver and Documentation
03 - Logical Elements
04 - Locations and Ranges
05 - Select Elements
06 - Warning and Internal Options
07 - Compare Elements
08 - ELF Reader
09 - CodeView Reader

This is a high level summary of the changes in this patch.

Driver and documentation

• Command line options.
• Full documentation.
• String Pool table.

[psamolysov]

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.

[psamolysov]

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

details?

510–511

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

527

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

563

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

1467

Two formats generated by 3 compilers.

1918

1937

To use the same order for the words.

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

27

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

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

17

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

53

The virtual destructor is required, isn't it?

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

23

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

193

the closed bracket is missed.

220

236

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

300–303

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

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

19

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

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

29

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

43

48

51

71

llvm/lib/DebugInfo/LogicalView/LLVMBuild.txt

22

An empty line should be added at EOF

llvm/tools/llvm-dva/Options.cpp

83

249

257

394–395

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

465

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

468

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

476

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

Should the class be marked as final?

llvm/tools/llvm-dva/llvm-dva.cpp

9

71

[jryans]

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

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

147–148

Nit: These sentences read a bit strangely

[CarlosAlbertoEnciso]

In D125777#3522100, @psamolysov wrote:

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.

[CarlosAlbertoEnciso]

In D125777#3525139, @psamolysov wrote:

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.

[psamolysov]

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

[CarlosAlbertoEnciso]

llvm/docs/CommandGuide/llvm-dva.rst

19

You are correct. Replaced with Mach-O.

147–148

Replaced with your suggestion.

239

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

510–511

It should be lowercase: class and compile unit.

527

You are correct. It should be A union.

563

It should be pointer type and Import module.

1467

Fixed incorrect sentence.

1918

Fixed the typo.

1937

Goot catch. Changed.

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

193

Added closed bracket.

220

Fixed typo.

236

It should be kinds.

[CarlosAlbertoEnciso]

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

43

Added const.

48

Added const.

51

Added const.

71

Added const.

llvm/lib/DebugInfo/LogicalView/LLVMBuild.txt

22

Added empty line.

llvm/tools/llvm-dva/Options.cpp

83

Fixed typo.

249

Changed to lowercase.

257

Changed to lowercase.

llvm/tools/llvm-dva/llvm-dva.cpp

9

Fixed typo.

[CarlosAlbertoEnciso]

llvm/docs/CommandGuide/llvm-dva.rst

239

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

[CarlosAlbertoEnciso]

llvm/docs/CommandGuide/llvm-dva.rst

239

Replaced with: used when lower level detail is required.

[psamolysov]

llvm/docs/CommandGuide/llvm-dva.rst

239

The proposed wording looks good for me. Thank you.

[probinson]

llvm/docs/CommandGuide/llvm-dva.rst

239

when a lower level of detail is required.

[CarlosAlbertoEnciso]

llvm/docs/CommandGuide/llvm-dva.rst

239

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

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

27

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

17

Including <string>.

53

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

23

<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?

300–303

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

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

19

<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

29

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

llvm/tools/llvm-dva/Options.cpp

394–395

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

465

Not a particular reason. Passing them by value.

468

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

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

llvm/tools/llvm-dva/Options.h

27

Marked as final.

llvm/tools/llvm-dva/llvm-dva.cpp

71

Replaced with empty().

[psamolysov]

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

23

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.

300–303

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

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

19

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.

[CarlosAlbertoEnciso]

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

23

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

300–303

Thanks.

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

19

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

[CarlosAlbertoEnciso]

In D125777#3591664, @psamolysov wrote:

@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.