[PATCH] D125777: [llvm-debuginfo-analyzer] 02 - Driver and documentation

Wed Sep 21 04:33:11 PDT 2022

CarlosAlbertoEnciso added inline comments.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:2
+llvm-debuginfo-analyzer - Print a logical representation of low-level debug information.
+========================================================================================
+
----------------
probinson wrote:
> Is the line of `=` supposed to be exactly as long as the preceding line? It looks like there's one extra character.
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.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:126
+
+ With **value** being one the options in the following lists.
+
----------------
probinson wrote:
> 
Added `of`.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:276
+
+ With **value** being one the options in the following lists.
+
----------------
probinson wrote:
> 
Added `of`.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:340
+
+ With **value** being one the options in the following lists.
+
----------------
probinson wrote:
> 
Added `of`.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:378
+
+ With **value** being one the options in the following list.
+
----------------
probinson wrote:
> 
Added `of`.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:449
+ Print all elements that satisfy the given <condition>. With **condition**
+ being one the options in the following list.
+
----------------
probinson wrote:
> 
Added `of`.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:471
+symbols (:option:`--select-symbols`) and types (:option:`--select-types`).
+These options, require knowledge of the debug information format (DWARF,
+CodeView, COFF), as the given **kind** describes a very specific type
----------------
probinson wrote:
> 
Removed `,`.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:482
+
+ With **kind** being one the options in the following list.
+
----------------
probinson wrote:
> 
Added `of`.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:503
+
+ With **kind** being one the options in the following list.
+
----------------
probinson wrote:
> 
Added `of``.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:537
+
+ With **kind** being one the options in the following list.
+
----------------
probinson wrote:
> 
Added `of`.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:555
+
+ With **kind** being one the options in the following list.
+
----------------
probinson wrote:
> 
Added `of`.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:635
+When reading the input object files, :program:`llvm-debuginfo-analyzer`
+can detected issues in the raw debug information. These may not be
+considered fatal to the purpose of printing a logical view but they can
----------------
probinson wrote:
> 
Changed to `detect`.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:645
+
+ With **value** being one the options in the following list.
+
----------------
probinson wrote:
> 
Added `of`.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:667
+ For a better understanding of the logical view, access to more detailed
+ internal information is needed. Such data would help to identify debug
+ information processed or incorrect logical element management. Typically
----------------
probinson wrote:
> is -> could be (?)
Taking your suggestion.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:677
+
+ With **value** being one the options in the following list.
+
----------------
probinson wrote:
> 
Added `of`.

================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:711
+:program:`llvm-debuginfo-analyzer`. We compiled the example for an X86
+ELF target with a recent version of Clang (-O0 -g):
+
----------------
probinson wrote:
> (two years from now it won't be "recent" anymore)
Good point. Removed `a recent version of`.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:43
+public:
+  // True if the scope has not been named or typed or no line number.
+  virtual bool isNamed() const { return false; }
----------------
probinson wrote:
> True if it has _not_ been named?  that's inconsistent with the function name.
Changed to `// True if the scope has been named.`
Later patches will update the comment to include `type` and `line number`.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:49
+#define STDSET_FUNCTION_4(FAMILY, FIELD, TYPE, SET)                            \
+  bool get##FAMILY##FIELD() const {                                            \
+    return FAMILY.SET.find(TYPE::FIELD) != FAMILY.SET.end();                   \
----------------
probinson wrote:
> 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.
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.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h:92
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+  virtual void dump() const { print(dbgs()); }
+#endif
----------------
probinson wrote:
> Does this really need to be virtual?  If so, the destructor should also be virtual?
It does not need to be virtual. Removed `virtual`.

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

https://reviews.llvm.org/D125777