[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/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"
----------------
avl wrote:
> jhenderson wrote:
> > 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
> > ```
> we need Flags and probably AddressAlign:, but Content might be replaced with Size.
Is `1b` significant as a size? Could it be empty? 1? 0x10?


================
Comment at: llvm/tools/llvm-dwarfutil/DebugInfoLinker.cpp:232
+    WithColor::note() << "    in DIE:\n";
+    Die->dump(errs(), 6 /* Indent */, DumpOpts);
+  };
----------------
Canonical style is as suggested.


================
Comment at: llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp:192
+  // 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, the isObject
----------------



================
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,
----------------
avl wrote:
> jhenderson wrote:
> > 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?
> I think it is better to land it in this form. It is useful functionality and it is better to have it from start. As to the refactoring restoreStatOnFile - I am going to create a patch for soon.
> As to the refactoring restoreStatOnFile - I am going to create a patch for soon.

Not sure if we're talking past each other hum. I'm simply requesting the `restoreStatOnFile` patch be created and then this patch rebased on top of it, rather than duplicating code that will be resolved by the other patch.


================
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;
----------------
avl wrote:
> jhenderson wrote:
> > 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.
> The reason to not calling std::exit(1) is that error() function  is used as a error reporting function for DWARFLinker. DWARFLinker in it`s current state does not stop if any error encountered. As an alternative I can create separate error reporting function for DWARFLinker.
I've got no preference, so do whatever you prefer. The one thing I would say: if you've printed `error: xxx` to stderr, people will expect the the tool to return with a non-zero exit code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86539