This is an archive of the discontinued LLVM Phabricator instance.

[ObjectYAML][DWARF] Support emitting the .debug_aranges section in ELFYAML.
ClosedPublic

Authored by Higuoxing on Jun 1 2020, 8:25 PM.

Details

Summary

This patch enables yaml2obj to emit the .debug_aranges section in ELFYAML.

Known issues:

  • The current implementation of debug_aranges doesn't support emitting segment in the (segment, address, length) tuple. I will fix it in a follow-up patch.

Diff Detail

Event Timeline

Higuoxing created this revision.Jun 1 2020, 8:25 PM
Higuoxing edited the summary of this revision. (Show Details)Jun 1 2020, 8:27 PM
Higuoxing edited the summary of this revision. (Show Details)Jun 2 2020, 12:14 AM
jhenderson added inline comments.Jun 2 2020, 1:55 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
99–100 ↗(On Diff #267779)

Perhaps you should move this fix into a follow-up change, and wait to add DWARF64 testing to that same change. It looks good, but I think we want to avoid confusing the two issues.

108 ↗(On Diff #267779)

Similar to above, you should probably delay segment size support for a follow-up change.

llvm/lib/ObjectYAML/ELFYAML.cpp
1657–1659 ↗(On Diff #267779)

This seems somewhat unrelated too? Perhaps defer to another patch, rather than trying to adopt it all at once.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml
6

the -> a

8

Nit: -S -> --sections (since you are using a long option already).

9

Can you get away with just one check-prefix here? It looks like they're both used together always.

76

I'd make one of these 4 byte size and the other 8 (perhaps deliberately do 8 bytes for the DWARF32 case, to deliberately show the address size and DWARF32/64 bits are unrelated). Since I'm suggesting you wait for DWARF64 support to a separate change, make them both DWARF32 temporarily, and update the test case when you add DWARF64 support.

82

Not part of this change, but I think this should be changed in the future to be an explicit Format: DWARF64 field, and just leave TotalLength to be the only length field (i.e. don't have a separate Length and TotalLength fields).

92

the -> and

93

In your follow-up change to add support for big endian, you can avoid the need for two separate YAML blobs, by using yaml2obj's -D option. It works similary to FileCheck's:

# RUN: yaml2obj %s -D ENDIAN=ELFDATA2LSB -o %tle.o
...
# RUN: yaml2obj %s -D ENDIAN=ELFDATA2MSB -o %tbe.o
...

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    [[ENDIAN]]
...
179

Delete "the" from "the raw section"

217

debug_arange -> debug_aranges

Higuoxing updated this revision to Diff 268042.Jun 2 2020, 7:40 PM

Address comments

  • Remove test cases that relate to endianness or DWARF64
    • These test cases will be added in a follow-up patch.
  • Correct some grammar mistakes.
Higuoxing edited the summary of this revision. (Show Details)Jun 2 2020, 7:42 PM
Higuoxing updated this revision to Diff 268043.Jun 2 2020, 7:51 PM

Correct docnum.

jhenderson added inline comments.Jun 3 2020, 2:09 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml
10

As you only have one check prefix, you can use --check-prefix like before, if you want.

I'm slightly surprised the code naturally produces big endian output!

49

Nit: I'd get rid of the space in the dashes on this line.

Same sort of comment on other lines below.

77

Looks like you haven't addressed my earlier comment about making the tables different address sizes. I don't think the address size should have to match the target's address size, if not desired.

80–81

It's probably a good idea for at least one table to have multiple Descriptor entries.

grimar added inline comments.Jun 3 2020, 2:37 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml
107

I wonder if we need to test such things?

The Content is a key that is often used to emit some broken output.
So why not just to test that we can emit an arbitrary content, e.g.: Content: "112233"?

Higuoxing marked an inline comment as done.Jun 3 2020, 2:55 AM
Higuoxing added inline comments.
llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml
107

I want to test the default values of section header, e.g., sh_info/link/... and reuse the check tag: DWARF-BE-DEFAULT. What about having 2 check tags: DWARF-BE-HEADER, DWARF-BE-CONTENT

# RUN: yaml2obj --docnum=1 %s -o %t1.o
# RUN: llvm-readobj --sections --section-data %t1.o | FileCheck --check-prefixes=DWARF-BE-HEADER,DWARF-BE-CONTENT

#      DWARF-BE-HEADER: Index: 1
# DWARF-BE-HEADER-NEXT: Name:  .debug_aranges
...
#      DWARF-BE-CONTENT: SectionData (
# DWARF-BE-CONTENT-NEXT:   ...

# RUN: yaml2obj --docnum=2 %s -o %t2.o
# RUN: llvm-readobj --sections --section-data %t2.o | FileCheck --check-prefixes=DWARF-BE-HEADER,ARBITRARY-CONTENT

#      ARBITRARY-CONTENT: SectionData (
# ARBITRARY-CONTENT-NEXT:   ...
grimar added inline comments.Jun 3 2020, 3:21 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml
107

Having 2 tags sounds fine to me.

Higuoxing updated this revision to Diff 268125.Jun 3 2020, 3:42 AM

Addressed comments:

  • Let the section in test case (b) be generated from an arbitraty "Content".
  • Let the "AddrSize" in first entry be 0x04.
  • Add one more (Address, Length) pair in the second entry.
Higuoxing updated this revision to Diff 268137.Jun 3 2020, 4:19 AM

Correct one comment in test case.

jhenderson accepted this revision.Jun 3 2020, 4:30 AM

LGTM, with one nit.

Don't forget to mark comments as "Done" before posting your latest update. The "Done" marks will be submitted when you post the diff, and it's then easy to see any comments that might have been missed/not addressed etc.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml
12–25

Nit: there's an extra two spaces of indentation here that aren't really needed as far as I can tell, so you can remove them.

This revision is now accepted and ready to land.Jun 3 2020, 4:30 AM
Higuoxing marked an inline comment as done.Jun 3 2020, 6:00 AM
Higuoxing updated this revision to Diff 268159.Jun 3 2020, 6:01 AM

Remove unneeded spaces.

Thanks for reviewing!

jhenderson accepted this revision.Jun 3 2020, 6:34 AM
This revision was automatically updated to reflect the committed changes.