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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Just to be clear: I don't have the time, or really the knowledge, to review the bulk of the core behaviour of this change. Someone else will need to review that.



================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:506
+  //       ranges. The better fix would be to combine such ranges. Following
+  //       is a workaround should be removed when a good fix is done.
+  if (Unit.overlapsWithFunctionRanges(*LowPc, *HighPc)) {
----------------
Apologies, missed one bit in my previous suggestion.


================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/Inputs/common.yaml:8-13
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x0000000000000010
+    Content:        "FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"
----------------
Assuming you need a .text section, do you actually need it to have anything other than something like the below?
```
Name:    .text
Type:    SHT_PROGBITS
Address: 0x1000
Size:    0x10
```


================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:191-194
+  // The file "FileName" was created by this utility in the previous steps.
+  // (i.e. it is already known that it should pass isObject check).
+  // If the createBinary() function does not return an error then check for
+  // isObject should also be successful.
----------------
Slightly more natural sounding phrasing suggested. Please reflow the comment if needed.


================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:452-453
+
+// TODO: This implementation is copied from llvm-objcopy.
+//       It is nessessary to refactor it out and remove this duplication.
+static Error restoreStatOnFile(StringRef Filename,
----------------
Could we refactor it out first please? Alternatively, does the behaviour need to be in the initial landing of this patch, or can it be added later?


================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:528
+  if (Error Err = validateAndSetOptions(Args, Opts)) {
+    error(std::move(Err), dwarfutil::ToolName);
+    return EXIT_FAILURE;
----------------
Many tools have the `error()` function call `std::exit(1)` to end the program without needing an explicit `return EXIT_FAILURE`. I'm okay either way with it, but wanted to raise common practice with you here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86539