Page MenuHomePhabricator

[obj2yaml] [yaml2obj] Add yaml support for SHT_LLVM_BB_ADDR_MAP section.
ClosedPublic

Authored by rahmanl on Oct 1 2020, 10:27 PM.

Details

Summary

YAML support allows us to better test the feature in the subsequent patches. The implementation is quite similar to the .stack_sizes section.

Diff Detail

Event Timeline

rahmanl created this revision.Oct 1 2020, 10:27 PM
rahmanl updated this revision to Diff 295734.Oct 1 2020, 10:35 PM

Remove the ELFDumper.cpp change.

rahmanl updated this revision to Diff 298247.Oct 14 2020, 3:05 PM
  • Add three more test cases to bb-addr-map.yaml.
rahmanl edited reviewers, added: grimar, MaskRay; removed: espindola.Oct 14 2020, 3:06 PM
rahmanl published this revision for review.Oct 14 2020, 3:07 PM
rahmanl retitled this revision from Add yaml support for llvm_bb_addr_map section. to [obj2yaml] [yaml2obj] Add yaml support for SHT_LLVM_BB_ADDR_MAP section..Oct 14 2020, 3:10 PM
rahmanl edited the summary of this revision. (Show Details)
rahmanl added subscribers: snehasish, tmsriram, shenhan.
grimar added a comment.EditedOct 15 2020, 1:44 AM

Generally looks fine to me, I've put a few comments inline and also:

  1. This needs a rebasing (D89039 was landed, it added support of "Size"/"Content" for all sections).
  2. This needs yaml2obj tests. I think following are reasonable for initial support:
    • a) Check values of section fields produced by default (sh_link, sh_info, etc).
    • b) Check you can specify none of "Content", "Size" or "Entries" to produce an empty section.
    • c) Check you can specify only "Size", only "Content" or only "Entries" (see tests in D89039 and others).
    • d) Check you can't specify "Entries" with "Content"/"Size" (you need to add the code to validate(), see D89391).
llvm/lib/ObjectYAML/ELFEmitter.cpp
1336
1342

You don't need to have curly braces here.

llvm/lib/ObjectYAML/ELFYAML.cpp
1544

This needs a yaml2obj test to show that we can omit "Address" and it will accept it and produce "0" as address.

1546

I think you don't need NumBlock YAML key. It matches the number of items in "BBEntries", right?
E.g. see how we describe Notes (and other things):

- Name:  .note.foo
  Type:  SHT_NOTE
  Flags: [ SHF_ALLOC ]
  Notes:
    - Name: AB
      Desc: ''
      Type: 0xFF
    - Name: ABC
      Desc: '123456'
      Type: 254
llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml
148

Missing empty line before "--- !ELF"

157

I'd change 0x0000000000000010 to 0x0000000000000000 and demonstrate that
obj2yaml does not emit the "Address" key when it is 0x0. (I haven't check, but this is how I expect the code works now).

llvm/tools/obj2yaml/elf2yaml.cpp
510

Excessive empty line.

jhenderson added inline comments.Oct 19 2020, 2:13 AM
llvm/lib/ObjectYAML/ELFYAML.cpp
1546

Presumably if the NumBlock field corresponds to part of the data structure, we want it to be an Optional member, so that users can override it independently of the number of entries - that way tests can be crafted that show what happens if the value is invalid.

rahmanl updated this revision to Diff 300816.Oct 26 2020, 3:51 PM
rahmanl marked 6 inline comments as done.
  • Remove NumBlocks from the YAML key.
  • Add yaml2obj test.
rahmanl marked an inline comment as done.Oct 26 2020, 3:53 PM

Thanks for your helpful comments @grimar.

llvm/lib/ObjectYAML/ELFEmitter.cpp
1336

Adopted in the line above.

llvm/lib/ObjectYAML/ELFYAML.cpp
1544

Please look at the added test.

1546

Great suggestion. Thanks. Removed the NumBlocks YAML key, even though the field still shows up in the section data.

llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml
157

Thanks. It works as expected.

rahmanl updated this revision to Diff 300821.Oct 26 2020, 3:55 PM
rahmanl marked an inline comment as done.
  • Remove excessive empty line.
rahmanl marked an inline comment as done.Oct 26 2020, 3:55 PM
grimar added inline comments.Oct 26 2020, 11:41 PM
llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml
19

You don't need this line, since you are always using -NEXT and dump all output.

llvm/test/tools/yaml2obj/ELF/bb-addr-map.yaml
4

I'd move these 1-5 comments to YAML description though:

Sections:
## 1) We can produce a .llvm_bb_addr_map section from a description with section
##    content.
  - Name:    '.llvm_bb_addr_map (1)'
    Type:    SHT_LLVM_BB_ADDR_MAP
    Content: "000000000000000001010203"
## 2) We can produce an empty .llvm_bb_addr_map section from a description
##    swith empty section content.
  - Name:    '.llvm_bb_addr_map (2)'
    Type:    SHT_LLVM_BB_ADDR_MAP
...
113

You don't need (1) for this test.

115

I think you should be able to write just Entries: []. No need to provide more input data than needed.
The same for Context: a single byte should be enough for this test.

140

I'd merge last two YAMLs into one with the use of macros. I think something like the
following should work:

--- !ELF
FileHeader:
  Class: ELFCLASS64
  Data:  ELFDATA2LSB
  Type:  ET_EXEC
Sections:
##  Specify Content and Entries
  - Name:    '.llvm_bb_addr_map'
    Type:    SHT_LLVM_BB_ADDR_MAP
    Entries: []
    Content: [[CONTENT=<none>]]
    Size:    [[SIZE=<none>]]

And then you can:

not yaml2obj --docnum=2 -DCONTENT="00"...
not yaml2obj --docnum=2 -DSIZE=0...
grimar added inline comments.Oct 26 2020, 11:43 PM
llvm/test/tools/yaml2obj/ELF/bb-addr-map.yaml
115

Or even: Content: ""

jhenderson added inline comments.Wed, Oct 28, 2:18 AM
llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml
129

Same as above - the -NOT check is unnecessary due to the use of the -NEXT pattern throughout.

llvm/test/tools/yaml2obj/ELF/bb-addr-map.yaml
8
41

Watch out - this will also match Size: 10. You need extra regex patterns to make sure there are no preceding characters, e.g:

# CHECK:      Size:{{ }}
# CHECK-SAME: {{^}}0

(in this context the ^ character represents the end of the previous match).

llvm/tools/obj2yaml/elf2yaml.cpp
799

In the empty() case, I think it would be great to not specify Content or Entries, so that you end up with a section like this:

Name: .llvm_bb_addr_map
Type: SHT_LLVM_BB_ADDR_MAP
rahmanl updated this revision to Diff 302670.Tue, Nov 3, 1:00 PM
rahmanl marked 9 inline comments as done.
  • Refine tests.
llvm/test/tools/yaml2obj/ELF/bb-addr-map.yaml
41

Thanks for the catch.
Applied your suggestion with some minor changes.

115

Used -DCONTENT="00"

140

Looks neat. Thanks.

rahmanl marked an inline comment as done.Tue, Nov 3, 1:02 PM
rahmanl added inline comments.
llvm/tools/obj2yaml/elf2yaml.cpp
799

Checked for Content.empty() before jumping into the while loop.

jhenderson accepted this revision.Wed, Nov 4, 12:24 AM

LGTM, with a couple of nits.

llvm/test/tools/yaml2obj/ELF/bb-addr-map.yaml
60

Trivial nit: to me, the '.' is silent when I say this out loud, so it sounds like "an .llvm_bb_addr_map" would be better than "a .llvm_bb_addr_map". Same applies for point 4). Feel free to ignore.

100

Reads more cleanly to me.

This revision is now accepted and ready to land.Wed, Nov 4, 12:24 AM
grimar accepted this revision.Wed, Nov 4, 11:37 PM

LGTM too.

rahmanl updated this revision to Diff 303525.Fri, Nov 6, 12:38 PM
rahmanl marked an inline comment as done.
  • Typos/Style fix.
rahmanl marked 2 inline comments as done.Fri, Nov 6, 12:42 PM

Thank you @grimar and @jhenderson for the thorough feedback. I will start on llvm-readobj support once I submit this patch.

llvm/test/tools/yaml2obj/ELF/bb-addr-map.yaml
60

Fixed here and elsewhere.

This revision was landed with ongoing or failed builds.Fri, Nov 6, 12:46 PM
This revision was automatically updated to reflect the committed changes.
rahmanl marked an inline comment as done.
grimar added inline comments.Mon, Nov 9, 12:20 AM
llvm/lib/ObjectYAML/ELFYAML.cpp
1504

Oh. This piece is actually not needed anymore, we have a general mechanism for validating fields.
I've committed rGc9d036ad4a2962059c595c77abb51154e2f5ec27

rahmanl added inline comments.Mon, Nov 9, 10:00 AM
llvm/lib/ObjectYAML/ELFYAML.cpp
1504

Thanks for submitting the fix.