[PATCH] D125777: [llvm-dva] 02 - Driver and documentation

Thu May 19 08:18:12 PDT 2022

psamolysov added a comment.

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.

================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:239
+ extended knowledge about debug information. They are intended to be
+ used when a more low level detail is required.
+
----------------
details?

================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:510-511
+    =CatchBlock: An exception block.
+    =Class: A Class.
+    =CompileUnit: A Compile unit.
+    =EntryPoint: A subroutine entry point.
----------------
Why `Class` and `Compile unit` are started from a capital letter while all the other entities (`array`, `call site`, etc.) aren't?

================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:527
+    =TryBlock: An exception try block.
+    =Union: An union.
+
----------------
I'm not a language expert, but I believe there should be "A union". 

================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:563
+    =ImportModule: Import Module.
+    =Pointer: Pointer Type.
+    =PointerMember: Pointer to member function.
----------------
`Pointer Type` is here but `Reference type` (not `Type`) is below. Also `Import Module` (not `Module`) is on the previous line.

================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:1467
+references to the enumerators **RED** and **BLUE**. The DWARF
+generated by GCC, CodeView generated by Clang and MSVC, it does
+include such references.
----------------
Two formats generated by 3 compilers.

================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:1918
+information by each scope, which can be used to determine unexpected
+sizes changes in the DWARF sections between different versions of the
+same toolchain.
----------------

================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:1937
+The **{Coverage}** and **{Location}** attributes describe the debug
+location and coverage for logical symbols. For optimized code, the
+coverage value decreases and it affects the program debuggability. 
----------------
To use the same order for the words.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVElement.h:27
+
+class LVElement : public LVObject {
+  // Indexes in the String Pool.
----------------
Does it make sense to mark the `LVElement` class as `final`?

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:17
+
+#include "llvm/DebugInfo/LogicalView/Core/LVSupport.h"
+
----------------
Let's include `<string>` to make the file self contained.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:53
+  std::string lineNumberAsStringStripped(bool ShowZero = false) const;
+};
+
----------------
The virtual destructor is required, isn't it?

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:23
+#include "llvm/DebugInfo/LogicalView/Core/LVType.h"
+
+namespace llvm {
----------------
Let's include `<set>` and `<string>` to make the file self contained.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:193
+// members that in very few cases point to a command option (see associated
+// comment. Other cases for 'bool' refers to internal values derivated from
+// the command options.
----------------
the closed bracket is missed.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:220
+    bool Formatting = true;    // Disable formatting during printing.
+    bool Offset = false;       // Print offsets while formatting is disable.
+    bool SizesSummary = false; // Print 'sizes' or 'summary'.
----------------

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:236
+    bool Execute = false;        // Select requested.
+    bool GenericKind = false;    // We have collected generic kind.
+    bool GenericPattern = false; // We have collected generic patterns.
----------------
kinds or there will be a single kind only? There are a bunch of kind sets below, though.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:300-303
+  LVOptions() = default;
+  LVOptions(const LVOptions &) = default;
+  LVOptions &operator=(LVOptions const &) = default;
+  ~LVOptions() = default;
----------------
Just for interest, why do you explicitly define these default members?

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:19
+#include "llvm/DebugInfo/LogicalView/Core/LVSort.h"
+
+namespace llvm {
----------------
Let's include <set> to make the file self contained.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h:29
+class LVStringPool {
+  const size_t BadIndex = std::numeric_limits<size_t>::max();
+  using TableType = StringMap<size_t, BumpPtrAllocator>;
----------------
Can we use `constexpr` here (I'm not sure is it available in C++14)?

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h:43
+public:
+  bool isValidIndex(size_t Index) { return Index != BadIndex; }
+
----------------

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h:48
+  // strings.
+  size_t getSize() { return Entries.size() - 1; }
+
----------------

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h:51
+  // Return the index for the specified key, otherwise 'BadIndex'.
+  size_t findIndex(StringRef Key) {
+    TableType::const_iterator Iter = StringTable.find(Key);
----------------

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h:71
+  // Given the index, return its corresponding string.
+  StringRef getString(size_t Index) {
+    assert(Index < Entries.size() && "Invalid string pool index.");
----------------

================
Comment at: llvm/lib/DebugInfo/LogicalView/LLVMBuild.txt:22
+required_libraries = Object Support DebugInfoDWARF DebugInfoCodeView DebugInfoPDB
\ No newline at end of file

----------------
An empty line should be added at EOF

================
Comment at: llvm/tools/llvm-dva/Options.cpp:83
+           clEnumValN(LVAttributeKind::Format, "format",
+                      "Objet file format name."),
+           clEnumValN(LVAttributeKind::Gaps, "gaps",
----------------

================
Comment at: llvm/tools/llvm-dva/Options.cpp:249
+           clEnumValN(LVReportKind::Children, "children",
+                      "Selected elements are displayed in a tree View "
+                      "(Include children)"),
----------------

================
Comment at: llvm/tools/llvm-dva/Options.cpp:257
+           clEnumValN(LVReportKind::View, "view",
+                      "Selected elements are displayed in a tree View "
+                      "(Include parents and children.")));
----------------

================
Comment at: llvm/tools/llvm-dva/Options.cpp:394-395
+        clEnumValN(LVTypeKind::IsEnumerator, "Enumerator", "Enumerator."),
+        clEnumValN(LVTypeKind::IsImport, "Import", "Import declaration."),
+        clEnumValN(LVTypeKind::IsImportDeclaration, "ImportDeclaration",
+                   "Import declaration."),
----------------
Both options are described as `Import declaration`. Is this actual?

================
Comment at: llvm/tools/llvm-dva/Options.cpp:465
+  // Traverse list of options and update the given set (Using case and Regex).
+  auto updatePattern = [&](auto &List, auto &Set, bool &Case, bool &Regex) {
+    if (!List.empty())
----------------
`Case` and `Regex` parameters are `bool`s and can be passed by value, why are they passed by reference?

================
Comment at: llvm/tools/llvm-dva/Options.cpp:468
+      for (std::string &Pattern : List)
+        Set.insert((Case && !Regex) ? StringRef(Pattern).lower() : Pattern);
+  };
----------------
`StringRef(Pattern).lower()` changes the string in the `List` or I'm missing something. Is it expected?

================
Comment at: llvm/tools/llvm-dva/Options.cpp:476
+  // Traverse list of options and update the given set.
+  auto updateSet = [&](auto &List, auto &Set) {
+    if (!List.empty())
----------------
I believe it can be replaced with `std::copy` or the corresponding overload of the `insert` method of the `Set` could be used.

================
Comment at: llvm/tools/llvm-dva/Options.h:27
+
+class OffsetParser : public llvm::cl::parser<unsigned long long> {
+public:
----------------
Should the class be marked as `final`?

================
Comment at: llvm/tools/llvm-dva/llvm-dva.cpp:9
+//
+// This program is a utility that display the logical view for the debug
+// information.
----------------

================
Comment at: llvm/tools/llvm-dva/llvm-dva.cpp:71
+  }
+  if (!BundlePaths.size())
+    BundlePaths.push_back(InputPath);
----------------

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125777/new/

https://reviews.llvm.org/D125777