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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:506
+  //       ranges. The better fix would be to combine such ranges. Following
+  //       workaround should be removed when good fix would be done.
+  if (Unit.overlapsWithFunctionRanges(*LowPc, *HighPc)) {
----------------



================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/error-unsupported-input-file.test:5-7
+# RUN: not llvm-dwarfutil --garbage-collection %t1 - 2>&1 | FileCheck %s
+
+# CHECK: error: '{{.*}}': unsupported input file
----------------
Optional to ensure you are checking the file name and not accidentally something else.


================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/gc-class.test:25
+
+## Check --no-odr-deduplication wins if last.
+# RUN: llvm-dwarfutil --no-odr-deduplication --odr-deduplication %t.o %t6
----------------



================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/reading-from-stdinput.test:4
+
+# RUN: yaml2obj %s -o - | llvm-dwarfutil --no-garbage-collection - %t1
+# RUN: llvm-dwarfdump -a %t1 | FileCheck %s
----------------
Up to you on this one, but I'd have this in the copy.test file, and then you can do `llvm-dwarfutil --no-garbage-collection - %t1 < [input-file]` where `[input-file]` is the same object used in the existing test cases in that file.


================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/reading-from-stdinput.test:24
+
+--- !ELF
+FileHeader:
----------------
Something to consider if you prefer to keep separate, or indeed more generally: move the YAML into a separate file under an Inputs subdirectory so that it can be referenced by multiple test cases. That saves duplicating identical YAML across different test files.


================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/separate-debug-file.test:1
+## This test checks --[no-]separate-debug-file option.
+
----------------



================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/separate-debug-file.test:22-23
+# RUN: llvm-dwarfdump -a %t3.debug | FileCheck --check-prefix=CHECK-DEBUG %s
+# RUN: cp %t3 %t4
+# RUN: cp %t3.debug %t4.debug
+
----------------
Did you mean `cp` here and not `cmp`? If so, I'd move it to where you need it rather than tagging it on the end of this test block, as it makes it look like it is important for the test case it is attached to.


================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/separate-debug-file.test:39
+# CHECK-NON-DEBUG-NOT: .debug_info
+
+
----------------
Nit: too many blank lines (1 is enough).


================
Comment at: llvm/tools/llvm-dwarfutil/Error.h:25
+
+inline void error(StringRef Prefix, Error Err) {
+  handleAllErrors(std::move(Err), [&](ErrorInfoBase &Info) {
----------------
If you swap the arguments and make `Prefix` default to empty, you can avoid the strange empty string at several call sites.


================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:190
+
+  llvm_unreachable(formatv("unsupported file: '{0}'", FileName).str().c_str());
+}
----------------
I think it might be more useful in the future to explain with a comment why we can't get here, in case something does manage to slip through and a developer needs to investigate. The error message could probably also do with explaining that it's an internal issue, e.g. "tool unexpectedly did not emit a supported object file".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86539