This is an archive of the discontinued LLVM Phabricator instance.

[obj2yaml] Add support for dumping the .debug_aranges section.
ClosedPublic

Authored by Higuoxing on Aug 2 2020, 9:24 AM.

Details

Summary

This patch adds support for dumping DWARF sections to obj2yaml. The
.debug_aranges section is used to illustrate the basic idea.

Diff Detail

Event Timeline

Higuoxing created this revision.Aug 2 2020, 9:24 AM
Herald added a project: Restricted Project. · View Herald Transcript
Higuoxing requested review of this revision.Aug 2 2020, 9:24 AM
Higuoxing updated this revision to Diff 282456.Aug 2 2020, 9:29 AM

Remove a temporary variable.

Higuoxing added inline comments.Aug 2 2020, 9:20 PM
llvm/tools/obj2yaml/elf2yaml.cpp
411

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.
  1. 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.'
grimar added inline comments.Aug 3 2020, 1:56 AM
llvm/tools/obj2yaml/elf2yaml.cpp
411
  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.

  1. 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.

jhenderson added inline comments.Aug 3 2020, 2:05 AM
llvm/test/tools/obj2yaml/ELF/DWARF/debug-aranges.yaml
5
66

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.

155–168

Seems like with appropriate -D options, you should be able to share this YAML (and maybe the CHECK directives too)?

llvm/tools/obj2yaml/elf2yaml.cpp
408–409

I find this comment a little confusing as is. I have made a suggestion.

411

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?

411

@grimar - thoughts?

(Never mind, I wrote this before seeing you'd posted...)

Higuoxing updated this revision to Diff 283118.Aug 4 2020, 8:59 PM
Higuoxing marked 4 inline comments as done.

Address comments.

jhenderson accepted this revision.Aug 5 2020, 1:02 AM

LGTM. If you want to emit a warning for unparseable contents, you migth want to do it more widely for other sections too, but in a separate patch. Up to you.

llvm/tools/obj2yaml/elf2yaml.cpp
409

Always check what I wrote makes sense - I make typos too :)

This revision is now accepted and ready to land.Aug 5 2020, 1:02 AM
Higuoxing updated this revision to Diff 283188.Aug 5 2020, 3:39 AM
Higuoxing marked an inline comment as done.

Address comments.

llvm/tools/obj2yaml/elf2yaml.cpp
409

Oh, I didn't notice it 🤣. Thanks a lot!

This revision was landed with ongoing or failed builds.Aug 5 2020, 4:19 AM
This revision was automatically updated to reflect the committed changes.