[PATCH] D86539: [Debuginfo][llvm-dwarfutil] llvm-dwarfutil dsymutil-like tool for ELF.

Tue Mar 29 02:01:05 PDT 2022

jhenderson added a comment.

I've made some basic comments, but I don't really have the time at the moment to review the code in depth (plus a good chunk of it is in areas I'm not particularly familiar with).

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:1-2
+llvm-dwarfutil - A tool to copy and manipulate debug info.
+==============================================
+
----------------

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:9
+
+:program:`llvm-dwarfutil` [*options*] *input* --out=*output*
+
----------------
Assuming you only intend to support a single input file per invocation, I'd follow llvm-objcopy's syntax of `llvm-dwarfutil [options] input [output]`, and if output is missing, modify in-place (this latter behaviour is not necessary, but I think makes some sense).

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:15
+:program:`llvm-dwarfutil` is a tool to copy and manipulate debug info.
+
+
----------------
Delete extra blank line.

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:17-24
+In basic usage, it makes a semantic copy of the input to the output. If any options are
+specified, the output may be modified along the way, e.g. by removing unused debug info.
+
+If "-" is specified for the input file, the input is read from the program's standard
+input stream. If "-" is specified for the output file, the output is written to
+the standard output stream of the program.
+
----------------
Let's reflow this and later content to an 80 character width.

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:27
+COMMAND-LINE OPTIONS
+----------------------------------
+
----------------

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:29
+
+.. option:: --do-garbage-collection, --gc
+
----------------
Unless we're trying to be compatible with some specific utility's command-line, let's not have two option names for the same option, unless one is a true short option (i.e. single letter, groupable with other short options).

Same applies throughout.

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:31
+
+            Removes pieces of debug information related to the discarded sections.
+            When the linker does garbage collection the abandoned debug info is left
----------------
jhenderson wrote:
> I don't think you need the deleted sentence. Also, if it is enabled by default, how is it disabled, and why does the option actually need to exist?
There's no need for such massive indentation. A couple of spaces is sufficient. Same applies throughout.

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:31-38
+            Removes pieces of debug information related to the discarded sections.
+            When the linker does garbage collection the abandoned debug info is left
+            behind. That abandoned debug info references address ranges using
+            tombstone value. Thus, when this option is specified, the tool removes
+            debug info which marked with the tombstone value. That cures
+            "overlapping address ranges" errors reported by :program:`llvm-dwarfdump`.
+
----------------
I don't think you need the deleted sentence. Also, if it is enabled by default, how is it disabled, and why does the option actually need to exist?

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:42-46
+            Remove duplicated types(if "One Definition Rule" is supported
+            by source language). Keeps first type definition and removes other
+            definitions. That significantly reduces the size of output debug info.
+
+            That option is set ON by default.
----------------
Same question as above: if it is enabled by default, how do you turn it off and why does the option need to exist?

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:48
+
+.. option:: --help, --h
+
----------------

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:52
+
+.. option:: --num-threads=<n>, --j
+
----------------

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:56
+
+.. option:: --out=<string>, -o
+
----------------
Nobody uses a long-form option for output file names (but also see my earlier comment about potentially not needing the option at all).

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:66-68
+            :program:`objcopy` --only-keep-debug in-file out-file.debug
+            :program:`objcopy` --strip-debug in-file out-file
+            :program:`objcopy` --add-gnu-debuglink=out-file.debug out-file
----------------
Use `llvm-objcopy` rather than `objcopy` throughout here.

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:72-80
+            <value> might have one of the following values:
+
+            - bfd: zero for all addresses and [1,1] for dwarf4(or less) address ranges.
+
+            - maxpc: -1 for all addresses and -2 for dwarf4(or less) address ranges.
+
+            - universal: both "bfd" and "maxpc".
----------------
"might have" implies there are other possible options.

================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:111
+
+To report bugs, please visit <https://github.com/llvm/llvm-project/labels/tools:llvm-dwarfutil/>.
----------------
Does this label exist yet?

================
Comment at: llvm/tools/llvm-dwarfutil/DebugInfoLinker.h:1
+//===-- DebugInfoLinker.h -------------------------------------------------===//
+//
----------------
Missing C++ indicator in the first line, as per the style guide for header file headers.

================
Comment at: llvm/tools/llvm-dwarfutil/DebugInfoLinker.h:2-6
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
----------------
This doesn't look like the current license header... Same elsewhere in the new code.

================
Comment at: llvm/tools/llvm-dwarfutil/DebugInfoLinker.h:18-19
+
+using namespace llvm;
+using namespace object;
+
----------------
Don't put `using namepsace xxx` in header files. It's too easy to end up causing everything to have that directive. Wrap the `dwarfutil` namespace inside the `llvm` namespace and explicitly qualify things in the `object` namespace.

================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:38
+
+static opt<bool>
+    BuildSeparateDebugFile("separate-debug-file",
----------------
I think it would be cleaner to use Tablegen-style options for these options. That also will produce more typical option parsing behaviour (cl::opt has weird syntax for some things).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86539