Page MenuHomePhabricator

[llvm-dva] 02 - Driver and documentation
Needs ReviewPublic

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

Details

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.

Diff Detail

Unit TestsFailed

TimeTest
60,120 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libasan -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Output/scariness_score_test.cpp.tmp
60,120 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/scariness_score_test.cpp.tmp
30 msx64 debian > LLVM-Unit.DebugInfo/LogicalView/_/DebugInfoLogicalViewTests/8::9
Script(shard): -- GTEST_OUTPUT=json:/var/lib/buildkite-agent/builds/llvm-project/build/unittests/DebugInfo/LogicalView/./DebugInfoLogicalViewTests-LLVM-Unit-1649030-8-9.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=9 GTEST_SHARD_INDEX=8 /var/lib/buildkite-agent/builds/llvm-project/build/unittests/DebugInfo/LogicalView/./DebugInfoLogicalViewTests
60,030 msx64 debian > MLIR.Examples/standalone::test.toy
Script: -- : 'RUN: at line 1'; /usr/bin/cmake /var/lib/buildkite-agent/builds/llvm-project/mlir/examples/standalone -G "Ninja" -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/var/lib/buildkite-agent/builds/llvm-project/build/lib/cmake/mlir ; /usr/bin/cmake --build . --target check-standalone | tee /var/lib/buildkite-agent/builds/llvm-project/build/tools/mlir/test/Examples/standalone/Output/test.toy.tmp | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/mlir/test/Examples/standalone/test.toy

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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

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

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

147–148

Nit: These sentences read a bit strangely

CarlosAlbertoEnciso edited the summary of this revision. (Show Details)Fri, Jun 17, 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

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.

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.

llvm/docs/CommandGuide/llvm-dva.rst
239

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

llvm/docs/CommandGuide/llvm-dva.rst
239

Replaced with: used when lower level detail is required.

psamolysov added inline comments.Fri, Jun 17, 8:07 AM
llvm/docs/CommandGuide/llvm-dva.rst
239

The proposed wording looks good for me. Thank you.

probinson added inline comments.Fri, Jun 17, 8:43 AM
llvm/docs/CommandGuide/llvm-dva.rst
239

when a lower level of detail is required.

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 added inline comments.Thu, Jun 23, 4:32 AM
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.

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