[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/tools/obj2yaml/elf2yaml.cpp:411
+      if (Err)
+        consumeError(std::move(Err));
+      else
----------------
jhenderson wrote:
> 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?
> @grimar - thoughts?
(Never mind, I wrote this before seeing you'd posted...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85094