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

Wed Apr 6 02:09:14 PDT 2022

avl added inline comments.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:504-505
+  if (Unit.overlapsWithFunctionsRanges(*LowPc, *HighPc)) {
+    reportWarning(formatv("Overlapped address range [{0:X}, {1:X}]. Range will "
+                          "be discarded.\n",
+                          *LowPc, *HighPc),
----------------
jhenderson wrote:
> Noting that this warning message (and indeed others in this file) don't conform to the current LLVM style guide. It also seems like `\n` should be part of the `reportWarning` function itself. I'd certainly expect it to be.
right. I used the same style like in the rest of that file. It does not look good to have a mix of styles. I think it should be separate commit for DWARFLinker library changing style of error messages.


================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy-itself.test:7
+# RUN: llvm-dwarfutil %t1 %t1
+# RUN: llvm-dwarfdump -a %t1 | FileCheck -check-prefix=CHECK-DEBUG %s
+# RUN: llvm-dwarfutil %t1 %t2
----------------
jhenderson wrote:
> Nit: here and throughout, I'd suggest using `--check-prefix` (two dashes), since it is the more common format for long options. It's also odd having some test cases using double dash for one command and then single dash for the FileCheck command.
Ok.


================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy-itself.test:8-10
+# RUN: llvm-dwarfutil %t1 %t2
+# RUN: llvm-dwarfdump -a %t2 | FileCheck -check-prefix=CHECK-DEBUG %s
+# RUN: diff %t1 %t2
----------------
jhenderson wrote:
> When I suggested adding this test case, I meant you should have it somewhere, but it probably doesn't belong in this test file. It might be more appropriate in the "standard" copy test file for this tool.
i.e. I need to move this check into the "standard" copy test file. OK.


================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy-itself.test:12-27
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-dwarfutil %t1 --separate-debug-file %t1
+# RUN: llvm-objdump --headers %t1 | FileCheck -check-prefix=CHECK-NON-DEBUG %s
+# RUN: llvm-dwarfdump -a %t1.debug | FileCheck -check-prefix=CHECK-DEBUG %s
+
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-dwarfutil --no-garbage-collection %t1 %t1
----------------
jhenderson wrote:
> As stated in my previous comment, you don't need these test cases: they duplicate coverage provided elsewhere. There's no need to test that the copy-self behaviour works with every option, because the behaviour is unrelated to those options.
It actually does not duplicate coverage. Using combination of these options(--garbage-collection, --no-garbage-collection, --separate-debug-file) leads to crossing different code paths:

llvm-dwwarfutil.cpp:219
llvm-dwwarfutil.cpp:244
llvm-dwwarfutil.cpp:331
llvm-dwwarfutil.cpp:359
llvm-dwwarfutil.cpp:395

Thus it looks good if all of them would be covered.


================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy.test:1-2
+## This test checks that debug info contained in the source file
+## is fully copied to the destination file.
+
----------------
jhenderson wrote:
> It seems like `gc-default.test` covers much of this test's behaviour? Do we need both?
> 
> `--separate-debug-file` should probably be tested in its own test file.
>It seems like gc-default.test covers much of this test's behaviour? Do we need both?

Probably, part for garbage-collection case is doubled and then might be removed. But part for no-garbage-collection looks correct and is not covered by gc-default.test.


================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/copy.test:17-18
+
+# RUN: llvm-dwarfutil %t.o --separate-debug-file --no-separate-debug-file %t1
+# RUN: llvm-dwarfdump -a %t1 | FileCheck -check-prefix=CHECK-DEBUG %s
+
----------------
jhenderson wrote:
> This doesn't show that `--no-separate-debug-file` is the option being processed... If you removed that option the test would still pass!
It looks like it does not pass:


```

diff --git a/llvm/test/tools/llvm-dwarfutil/ELF/copy.test b/llvm/test/tools/llvm-dwarfutil/ELF/copy.test
index 9ca78308f5da..9e9eb8b84750 100644
--- a/llvm/test/tools/llvm-dwarfutil/ELF/copy.test
+++ b/llvm/test/tools/llvm-dwarfutil/ELF/copy.test
@@ -14,7 +14,7 @@
 # RUN: llvm-objdump --headers %t1 | FileCheck -check-prefix=CHECK-NON-DEBUG %s
 # RUN: llvm-dwarfdump -a %t1.debug | FileCheck -check-prefix=CHECK-DEBUG %s
 
-# RUN: llvm-dwarfutil %t.o --separate-debug-file --no-separate-debug-file %t1
+# RUN: llvm-dwarfutil %t.o --separate-debug-file %t1
 # RUN: llvm-dwarfdump -a %t1 | FileCheck -check-prefix=CHECK-DEBUG %s
 
 # RUN: llvm-dwarfutil --no-garbage-collection %t.o %t1
```


```
llvm-lit  copy.test 
-- Testing: 1 tests, 1 workers --
FAIL: LLVM :: tools/llvm-dwarfutil/ELF/copy.test (1 of 1)
```




================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/odr-class.test:11
+
+# RUN: llvm-dwarfutil --odr %t.o - | llvm-dwarfdump -a - | FileCheck %s -check-prefixes=CHECK,CHECK-ODR
+
----------------
jhenderson wrote:
> Here and in similar tests, it would be useful for a short comment explaining what each individual case is testing, e.g. here it is "show that --odr is the default behaviour".
> 
> Additionally, where you are checking an alias behaviour/default behaviour that you've already tested in another way, i.e. where the output should be identical, use `cmp` to compare the previous output against the new output. This removes the additional llvm-dwarfdump and FileCheck runs, and also more rigidly ensures identity.
OK


================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:332
+  if (Error Err = writeToOutput(
+          Config.Common.OutputFilename, [&](raw_ostream &OutFile) -> Error {
+            raw_crc_ostream CRCBuffer(OutFile);
----------------
jhenderson wrote:
> Is the trailing return type needed?
I think it is always useful to type it explicitly. It clearly shows return type. Otherwise you need to compile the lambda by itself.  If it is against of guideline - then would remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86539

