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

[Alexey Lapshin via Phabricator via llvm-commits]

avl 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"
----------------
jhenderson wrote:
> 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?
It could not be empty. It also should be not less then referenced:


```
          Values:
            - CStr: foo1
            - Value:  0x1000
            - Value:  0x10  <<<<<<<<<
```


================
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,
----------------
jhenderson wrote:
> 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.
I understand. This way current patch will depend on the patch for restoreStatOnFile. It looks non practical for me because it would delay review. This restoreStatOnFile is not an important interface thing which will make a lot of problems later. That is why I think it is better to not create patch dependencies and that it might be OK to do both things independently.

But if you prefer to refactor restoreStatOnFile first - will rebase this patch on requested refactoring.


================
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;
----------------
jhenderson wrote:
> 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.
that is already done:


```
inline void error(Error Err, StringRef Prefix = "") {
  handleAllErrors(std::move(Err), [&](ErrorInfoBase &Info) {
    WithColor::error(errs(), Prefix) << Info.message() << '\n';
  });
  std::exit(EXIT_FAILURE);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86539