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

Mon Apr 4 23:33:48 PDT 2022

jhenderson added a comment.

Please add testing for all the various error cases at least. I haven't checked the wider test coverage though.

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy-itself.test:1-3
+## This test checks that debug info containing in source file
+## is fully copied in the destination file(source file matches
+## with destination.
----------------

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy-itself.test:6-7
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-dwarfutil %t1 %t1
+# RUN: llvm-dwarfdump -a %t1 | FileCheck -check-prefix=CHECK-DEBUG %s
+
----------------
You've got wider testing elsewhere. It's probably sufficient to just check one case here.

You may want an additional test case somewhere that shows performing the same operation on a file that was used to produce that file does nothing (e.g. something like `llvm-dwarfutil %t1 %t2` then `llvm-dwarfutil %t2 %t3` and show that `%t2` and `%t3` are the same. This assumes that such behaviour would be expected.

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy.test:1-2
+## This test checks that debug info containing in source file
+## is fully copied in the destination file.
+
----------------

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/gc-bfd.test:1
+## This test checks that debug info related to deleted code is removed.
+
----------------
This and the equivalent comments in other tests are identical. Probably they should point out the key difference?

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/gc-default.test:1
+## This test checks that debug info related to deleted code is removed.
+
----------------
I feel like this test should be in the same file as the explicit version that uses the equivalent option.

================
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
----------------
avl wrote:
> 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.
Once you've got a test that shows that --gardbage-collection is the default, I don't think you need to specify it explicitly in other tests. It doesn't add any additional coverage.

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/gc-full.test:1-2
+## This test checks that debug info removal optimization(--garbage-collection)
+## does not affect files which do not have dead debug info.
+
----------------
I don't get why "full" is in the test name?

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/odr-class.test:1-4
+## This test checks debug info for the case when one compilation unit
+## contains definition of the type and another compilation unit
+## contains definition of the same type. The result should contain original
+## definition in CU1 and removed definition from the CU2.
----------------

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/odr-fwd-declaration.test:1-4
+## This test checks debug info for the case when one compilation unit
+## contains forward declaration of the type and another compilation unit
+## contains definition for that type. The result should contain original
+## definition and declaration without modifications.
----------------
Same comments as above re. duplicate comment, and also grammar fixes.

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/verify.test:1
+## This test checks that debug info containing in source file is properly
+## verified by --verify after copying. If --no-garbage-collection is
----------------

================
Comment at: llvm/test/tools/llvm-dwarfutil/help.test:1
+## This test checks help message for llvm-dwarfutil utility.
+
----------------
"utility" doesn't add anything to this sentence, so get rid of it.

================
Comment at: llvm/tools/llvm-dwarfutil/Options.td:25
+  HelpText<"Alias for --odr-deduplication">,
+  Group<grp_dwarfutil>;
+
----------------
What does the grouping give us for these options? Unlike cl::opt based tools, there's no risk of unrelated command-line options appearing in the output, and as far as I can tell, there's no need to further categorise the options.

================
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>,
----------------
avl wrote:
> 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.
Okay. Please make sure there are test cases that show each alias applies appropriately.

Also, if you haven't already, please add the following test cases:
`--no-` options disable default on behaviour.
Competing `--no-` and explicit on options are resolved last-one-wins.

================
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");
----------------
avl wrote:
> 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.
Okay. I'd be more explicit about what's missing, i.e. have separate errors for both missing and just the output file missing. Alternatively, say something like "must specify exactly two positional arguments but N were specified" (where N is the number provided by the user). You can then remove the separate "too many positional arguments specified error", and simplify the check to `if (InputFiles.size() != 2)`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86539