[PATCH] D86539: [Debuginfo][llvm-dwarfutil] llvm-dwarfutil dsymutil-like tool for ELF.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 4 00:28:14 PDT 2022
jhenderson added inline comments.
================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:9
+
+:program:`llvm-dwarfutil` [*options*] *input* --out=*output*
+
----------------
avl wrote:
> jhenderson wrote:
> > 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).
> "modify in-place" behavior has a risk of unintentional rewriting of original file. That is why I disabled it.
>
> If it is preferred behavior then I will change the syntax accordingly.
I'm okay either way, but I think people might want to be able to write the output to the input file, so a test for `llvm-dwarfutil foo foo` should be included.
================
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:
> avl wrote:
> > jhenderson wrote:
> > > avl wrote:
> > > > jhenderson wrote:
> > > > > 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.
> > > > It might be disabled in this way:
> > > >
> > > > ```
> > > > --do-garbage-collection=0
> > > >
> > > > ```
> > > >
> > > > So the option assumed to be used to disable the behavior or to enable it explicitly.
> > > This is one of those cl::opt oddities. It's basically undocumented too, so I don't think we should aim to support this approach if switching to tablegen style instead. I would instead add `--no-...` style options to explicitly disable things (there are some good examples of this.
> > If we have direct options then we might have following pattern(i will use cl::opt style):
> >
> > in general make file:
> >
> > ```
> > OPTIONS=--do-garbage-collection=0
> >
> > ```
> > in custom make file:
> >
> > ```
> > dwarfutil $OPTIONS --do-garbage-collection=1
> >
> > ```
> > i.e. it is possible to override general option by a custom one.
> >
> > For the --no-style options we cannot do this:
> >
> > ```
> > OPTIONS=--no-garbage-collection
> > dwarfutil $OPTIONS ??????
> >
> > ```
> >
> > Having possibility to override general set of options looks like a good thing?
> >
> >
> I think you misunderstand: you can have both the positive and negative option, with a last one wins approach. There are many examples of this, e.g. in LLD. See https://github.com/llvm/llvm-project/blob/f1d8e46258c6a08ca1a375dc9670dd5581d6cf65/llvm/lib/Option/ArgList.cpp#L73 and references to that.
Sorry, should have been clear: there needs to be 1-2 spaces of indentation, I believe, so that the text is associated with the option.
If you aren't already, make sure to inspect the built doc output.
================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:111
+
+To report bugs, please visit <https://github.com/llvm/llvm-project/labels/tools:llvm-dwarfutil/>.
----------------
avl wrote:
> jhenderson wrote:
> > Does this label exist yet?
> not yet. I think it should be created after review accepted and before integration. right?
Yeah, sounds good.
================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:53
+
+ Disable --garbage-collection.
+
----------------
This causes a link to be created to the referenced option.
================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:57
+
+ Disable --odr-deduplication.
+
----------------
================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:61
+
+ Disable --separate-debug-file.
+
----------------
================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:65
+
+ Specifies the maximum number (n) of simultaneous threads to use
+ for processing.
----------------
================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:74-76
+ :program:`llvm-objcopy` --only-keep-debug in-file out-file.debug
+ :program:`llvm-objcopy` --strip-debug in-file out-file
+ :program:`llvm-objcopy` --add-gnu-debuglink=out-file.debug out-file
----------------
Rather than the formatting used here, use code syntax formatting. You can see examples in the llvm-symbolizer doc.
================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:82
+
+ - bfd: zero for all addresses and [1,1] for DWARF v4 (or less) address
+ ranges.
----------------
================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:85
+
+ - maxpc: -1 for all addresses and -2 for DWARF v4 (or less) address ranges.
+
----------------
================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:87
+
+ - universal: both "bfd" and "maxpc".
+
----------------
================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:89
+
+ - exec: match with address ranges of executable sections.
+
----------------
================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy.test:10
+
+# RUN: llvm-dwarfutil --no-garbage-collection %t.o -o %t1
+# RUN: llvm-dwarfdump -a %t1 | FileCheck -check-prefix=CHECK-DEBUG %s
----------------
Is this test passing at the moment? If so, why is the `-o` option not throwing an error?
================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/gc-exec.test:3-4
+
+# RUN: llvm-dwarfutil --tombstone=exec %t.o - | llvm-dwarfdump -a - | FileCheck %s
+# RUN: llvm-dwarfutil --tombstone=exec --garbage-collection %t.o - | llvm-dwarfdump -a - | FileCheck %s
+# RUN: llvm-dwarfutil --tombstone=universal --garbage-collection %t.o - | llvm-dwarfdump -a - | FileCheck %s
----------------
These two should produce identical output, right? Why do you need both?
================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/gc-exec.test:7
+
+## This test checks that debug info related to deleted code is removed.
+
----------------
Put comments describing the whole test at the file start.
================
Comment at: llvm/tools/llvm-dwarfutil/CMakeLists.txt:1
+
+set(LLVM_TARGET_DEFINITIONS Options.td)
----------------
No leading blank line.
================
Comment at: llvm/tools/llvm-dwarfutil/Options.td:11
+def help : Flag<["--"], "help">,
+ HelpText<"Prints this help output.">;
+
----------------
You're inconsistent in whether help messages end in '.'. Please always or never, whichever is more common in the other LLVM tools.
================
Comment at: llvm/tools/llvm-dwarfutil/Options.td:22-30
+def odr : Flag<["--"], "odr">,
+ Alias<odr_deduplication>,
+ HelpText<"Alias for --odr-deduplication.">,
+ Group<grp_dwarfutil>;
+
+def no_odr : Flag<["--"], "no-odr">,
+ Alias<no_odr_deduplication>,
----------------
Do you need these aliases? If you prefer the shorter name, why not make it the canonical form?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:84
+ for (auto *Arg : Args.filtered(OPT_UNKNOWN))
+ warning("ignoring unknown option: " + Arg->getSpelling() + "\n");
+
----------------
Unless there's a good reason for this to be a warning, let's make it an error.
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:87-89
+ if (InputFiles.empty() || InputFiles.size() == 1)
+ return createStringError(std::errc::invalid_argument,
+ "please specify input and/or output file");
----------------
It's pretty common to just print usage and exit in this case, I believe.
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:93
+ return createStringError(std::errc::invalid_argument,
+ "too many input files is specified");
+
----------------
We can avoid the "is" (actually would be "are") here as it's okay to be a bit terse in such messages. However, not all positional arguments are input files, so we need different terminology.
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:102
+ Message += Options.InputFileName;
+ Message += "\" does not exist.";
+ return createStringError(std::errc::invalid_argument, Message.c_str());
----------------
No . at end of error messages. Really though, should we not just report an error when failing to open the file later?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:143-144
+ !Options.DoGarbageCollection) {
+ warning("switching on for --odr-deduplication without "
+ "--garbage-collection is not currently supported. switched off");
+ Options.DoODRDeduplication = false;
----------------
Make this an error, since there's no reason for a warning?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:455
+int main(int Argc, char const *Argv[]) {
+ InitLLVM X(Argc, Argv);
+ dwarfutil::ToolName = Argv[0];
----------------
Perhaps put a `using namespace dwarfutil` inside here?
================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:469
+ (dwarfutil::ToolName + " [options] <input file> <output file>").c_str(),
+ "llvm-dwarfutil is a tool to copy and manipulate debug info.", false);
+ return EXIT_SUCCESS;
----------------
Do these messages usually end in '.'?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86539/new/
https://reviews.llvm.org/D86539
More information about the llvm-commits
mailing list