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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-dwarfutil.rst:9
+
+:program:`llvm-dwarfutil` [*options*] *input* --out=*output*
+
----------------
jhenderson wrote:
> 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.
Ok.


================
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
----------------
jhenderson wrote:
> Is this test passing at the moment? If so, why is the `-o` option not throwing an error?
Currently unknown options lead to warning. So yes the test passing. Will change to error and remove "-o" here.


================
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
----------------
jhenderson wrote:
> These two should produce identical output, right? Why do you need both?
To be sure default behavior matches with explicit. i.e. To check that "noopt" case is equal to "garbage-collection" case.


================
Comment at: llvm/tools/llvm-dwarfutil/Options.td:11
+def help : Flag<["--"], "help">,
+  HelpText<"Prints this help output.">;
+
----------------
jhenderson wrote:
> You're inconsistent in whether help messages end in '.'. Please always or never, whichever is more common in the other LLVM tools.
yep. My intention was to delete all of them.


================
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>,
----------------
jhenderson wrote:
> Do you need these aliases? If you prefer the shorter name, why not make it the canonical form?
The idea is to have all options in long form - "--no-odr-deduplication", "--garbage-collection". That short version - "no-odr" is to be compatible with dsymutil options. 

We could not have all options compatible with dsymutil, thus this tool have own options and they are assumed to be in a long form. But some options have aliases matching with dsymutil options. I think it would be good to have aliases matching with dsymutil.


================
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");
----------------
jhenderson wrote:
> It's pretty common to just print usage and exit in this case, I believe.
If there is no any option the tool would exactly print usage and exit, This message displayed if some options were specified but file name is not.


================
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());
----------------
jhenderson wrote:
> No . at end of error messages. Really though, should we not just report an error when failing to open the file later?
Ok, will remove.


================
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;
----------------
jhenderson wrote:
> Make this an error, since there's no reason for a warning?
Ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86539