[PATCH] D85094: [obj2yaml] Add support for dumping the .debug_aranges section.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/obj2yaml/ELF/DWARF/debug-aranges.yaml:4
+## a) Test dumping the DWARF32/64 address range table from 32/64-bit, little/big endian object files.
+## The .debug_aranges should be parsed into the 'DWARF' entry and the 'Sections' entry should keep empty.
+
----------------



================
Comment at: llvm/test/tools/obj2yaml/ELF/DWARF/debug-aranges.yaml:65
+
+## b) Test dumping an .debug_aranges section whose section header properties are overridden.
+
----------------
As the relevant `if` is an "or" mechanism, we should probably test each field individually, since any one of them should cause the section header to be emitted. You might want to use the -D option combined with the new `=<none>` default value.


================
Comment at: llvm/test/tools/obj2yaml/ELF/DWARF/debug-aranges.yaml:154-167
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+DWARF:
----------------
Seems like with appropriate -D options, you should be able to share this YAML (and maybe the CHECK directives too)?


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:408-409
+
+      // If the DWARF section can be successfully parsed, the content will be
+      // dropped.
+      if (Err)
----------------
I find this comment a little confusing as is. I have made a suggestion.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:411
+      if (Err)
+        consumeError(std::move(Err));
+      else
----------------
grimar wrote:
> Higuoxing wrote:
> > I want to hear suggestions on handling the parsing error here. I think it would be good if we can 
> > emit a warning or add a comment about the details of parsing failure in the generated YAML description. e.g.,
> > 
> > 1) Emit a warning
> > ```
> > $ obj2yaml a.out
> > ...
> > obj2yaml warning: obj2yaml doesn't support parsing the address range table whose segment selector size is not 0.
> > ```
> > 
> > 2) Add a comment field.
> > ```
> > Sections:
> >   - Name:    '.debug_aranges'
> >     Type:    SHT_PROGBITS
> >     Content: '01234567890abcdef'
> >     Comment: 'obj2yaml doesn't support parsing the address range table whose segment selector size is not 0.'
> > ```
> > 1. Emit a warning
> 
> I do not think we are emiting warnings when unable to parse a section, but this
> sounds reasonable to me. Perhaps I`d mention in the warning message that we falled
> back to dumping section data as raw bytes. Also I'd mention the section name.
> 
> > 2. Add a comment field.
> 
> I am not sure this is a useful thing to have. At least if it is going to be a comment, it can probably be a real
> comment, e.g:
> 
> ```
> ## 'obj2yaml doesn't support parsing the address range table whose segment selector size is not 
>     Content: '01234567890abcdef'
> ```
> 
> but I think it is probably too much and emiting a warning is enough.
> 
> 
> 
> 
So for regular ELF sections, we don't emit a warning or print a comment, I believe, so this as-is is probably fine. I could be persuaded that a warning is okay, but I think that's the only thing I'd suggest. @grimar - thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85094