This is an archive of the discontinued LLVM Phabricator instance.

[DWARFYAML] Implement the .debug_str_offsets section.
ClosedPublic

Authored by Higuoxing on Jul 15 2020, 12:21 AM.

Details

Summary

This patch helps add support for emitting the .debug_str_offsets section
to yaml2elf.

Diff Detail

Event Timeline

Higuoxing created this revision.Jul 15 2020, 12:21 AM
jhenderson added inline comments.Jul 15 2020, 12:49 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
434

A thought I've just had: what do you think should happen if a user specifies a 64-bit-using value for DWARF32? This may apply in one or two other places too.

I don't have any particular opinions here. Just wondering if you've considered it at all.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-str-offsets.yaml
46–47

I'd explicitly use the full 4 bytes here (and 8 bytes in the DWARF64 version).

98

Perhaps omit Offsets entirely here.

120

Perhaps it would be worth a test case for there being no tables at all, i.e:

DWARF:
  debug_str_offsets: []

I'd expect an empty section to be emitted in this case.

This might also be a missing test case in some (most?) of the other sections.

Higuoxing updated this revision to Diff 278104.Jul 15 2020, 1:35 AM
Higuoxing marked 5 inline comments as done.

Address comments.

llvm/lib/ObjectYAML/DWARFEmitter.cpp
434

Just wondering if you've considered it at all.

Yes, I've considered it before.

At first, It seemed to me that the format should be inferred from the content. When the length is greater than UINT32_MAX or the offset is greater than UINT32_MAX, the DWARF64 should be specified automatically. However, this approach makes our implementation complicated. Besides, DWARF32 is the most commonly used format. I think it's not a good approach.

Another approach is to check whether the integer is able to be encoded using the given size and warn the user about it.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-str-offsets.yaml
120

Yes, this test is missing in other tests. I'll add them. Thanks!

jhenderson added inline comments.Jul 15 2020, 1:42 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
434

My instinct is to do nothing at all for now. If people find it causing problems, it's functionality that could be added later. I wouldn't bother with the warning - in most cases, people will never see the warning, due to yaml2obj being run as part of lit tests.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-str-offsets.yaml
47–48

I meant something like 0x12345678 and 0x87654321, i.e. so that all the bytes contain non-zero.

122

Hmmm... I would expect the section header to be emitted, but for the section to be empty in this context.

Higuoxing updated this revision to Diff 278111.Jul 15 2020, 2:08 AM
Higuoxing marked 3 inline comments as done.

Address comments.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-str-offsets.yaml
122

Oh, I misunderstood it. We can wrap std::vector<StringOffsetsTable> with an additional Optional layer (Optional<std::vector<StringOffsetsTable>>). We can add it for other DWARF sections as well.

jhenderson added inline comments.Jul 15 2020, 3:01 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-str-offsets.yaml
125–133

I don't think you need to check the whole section header table in this case. Probably sufficient is just checking the .debug_str_offsets line. It might make more sense to use llvm-readobj style in that case.

Higuoxing updated this revision to Diff 278123.Jul 15 2020, 3:15 AM
Higuoxing marked an inline comment as done.

Address comments.

Thanks for reviewing!

jhenderson accepted this revision.Jul 15 2020, 3:19 AM

LGTM.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-str-offsets.yaml
125–129

Nice solution!

This revision is now accepted and ready to land.Jul 15 2020, 3:19 AM
MaskRay accepted this revision.Jul 15 2020, 9:53 PM
This revision was automatically updated to reflect the committed changes.