This is an archive of the discontinued LLVM Phabricator instance.

[DWARFYAML] Implement the .debug_rnglists section.
ClosedPublic

Authored by Higuoxing on Jul 11 2020, 7:33 AM.

Details

Summary

This patch implements the .debug_rnglists section. We are able to
produce the .debug_rnglists section by the following syntax.

debug_rnglists:
  - Format:              DWARF32 ## Optional
    Length:              0x1234  ## Optional
    Version:             5       ## Optional
    AddressSize:         0x08    ## Optional
    SegmentSelectorSize: 0x00    ## Optional
    OffsetEntryCount:    2       ## Optional
    Offsets:             [1, 2]  ## Optional
    Lists:
      - Entries:
          - Operator: DW_RLE_base_address
            Values:   [ 0x1234 ]

The generated .debug_rnglists is verified by llvm-dwarfdump, except for
the operator DW_RLE_startx_endx, since llvm-dwarfdump doesn't support
it.

Diff Detail

Event Timeline

Higuoxing created this revision.Jul 11 2020, 7:33 AM
Higuoxing updated this revision to Diff 277283.Jul 12 2020, 4:41 AM

Simplify codes for emitting offsets.

Higuoxing updated this revision to Diff 277284.Jul 12 2020, 4:52 AM

Use reference in EmitOffsets.

jhenderson added inline comments.Jul 13 2020, 2:57 AM
llvm/include/llvm/ObjectYAML/DWARFYAML.h
189

Maybe call this RnglistTable to reduce the potential for confusion.

llvm/lib/ObjectYAML/DWARFEmitter.cpp
502

I keep finding the Is64Bit variable name confusing with the DWARF format. Could you rename it in a separate change to something like Is64BitAddrSize or something similar, please?

507–508

Maybe these could be named to indicate these contain just the lists. Perhaps ListBuffer and ListBufferOS?

515

Perhaps use List for the variable name to avoid confusion with the type.

517

Similarly, consider Entry for the name.

524–525

It would probably be better to be more explicit about writing DW_RLE_end_of_list, e.g. writeInteger(DW_RLE_end_of_list, ...).

It may make more sense to get users to explicitly specify the end-of-list marker. This would be in keeping with how DT_NULL works for SHT_DYNAMIC ELF sections. This means that users can deliberately write lists without trailing terminators.

530–534

A comment explaining this order of precedence might help make things clearer.

539–540

Maybe "... we use it instead of the actual length" might be clearer/more correct.

550

ArrayRef<uint64_t> here.

llvm/lib/ObjectYAML/DWARFYAML.cpp
242

Version should be optional, and have a value of 5 by default, since it's the only supported version of the format currently.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml
1

I'm out of time to review these now, but I'll try to look at them tomorrow, or later on.

Higuoxing updated this revision to Diff 277653.Jul 13 2020, 9:26 PM
Higuoxing marked 11 inline comments as done.

Address comments.

Thanks for reviewing.

llvm/lib/ObjectYAML/DWARFEmitter.cpp
524–525

The trailing terminator is removed here. Users should specify the DW_RLE_end_of_list manually.

llvm/lib/ObjectYAML/DWARFYAML.cpp
242

The version field of the .debug_addr section is required (https://reviews.llvm.org/D81541#inline-750289). Shall we make that one optional as well? Or is there any particular reason that makes these two sections different?

Couple of other test case suggestions:

  1. What is the value of the AddrSize field by default, if using a 32-bit object.
  2. An empty list is allowed for a table.
llvm/lib/ObjectYAML/DWARFYAML.cpp
242

I don't think there's anything special about .debug_addr (it's also new in v5), so let's make that optional too (the more optional stuff the better, as long as it isn't likely to cause confusion).

llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml
2

emits a .debug_rnglists section when requested.

206–217

Is there a motivation for having so many operators in the DWARF64 case? Could you simplify it down so that there's only end-of-list markers?

275

Rather than have the fields tested in independent inputs, it's probably safe to test these all in one input which overwrites each of the fields.

328

I'd specify a different number of offsets (perhaps fewer) than the actual number of lists.

418–419

You also need a test showing that an invalid size can be used when there are no operands with addresses.

You might also want multiple copies of case j), testing each of the different address-using operators individually.

442–443

You need to test this for every operator, I think.

The easiest way to do that would be to use -D for the operator kind and values.

Higuoxing updated this revision to Diff 277765.Jul 14 2020, 3:46 AM
Higuoxing marked 13 inline comments as done.

Address comments.

Couple of other test case suggestions:

  1. What is the value of the AddrSize field by default, if using a 32-bit object.

Added in test case (f).

  1. An empty list is allowed for a table.

Added in test case (k).

Thanks for reviewing.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml
206–217

It's not really a motivation. I want to test the offsets for multiple entries in the DWARF64 section. Seems that the operators are useless here, I will delete them.

275

They are merged into the test case (e), where the length, version, offset_entry_count and offsets are overwritten.

328

See test case (e).

418–419

You also need a test showing that an invalid size can be used when there are no operands with addresses.

See test case (i).

You might also want multiple copies of case j), testing each of the different address-using operators individually.

See test case (h).

442–443

You need to test this for every operator, I think.

See test case (j).

Are we deliberately not supporting non-zero segment selector size values again? There's no test coverage for tables with non-zero values.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml
2

emits -> emits a

227–229

These comments look wrong. Version appears to be occupying four bytes, according to the ^-------

242

I'd specify the AddrSize and segment selector size in this test too.

260

You should probably test each of the address-using operands here to show they all use the right size, like you do in g).

372

to operator -> for an operator

Are we deliberately not supporting non-zero segment selector size values again? There's no test coverage for tables with non-zero values.

The description of 'SegmentSelectorSize' isn't very clear to me in the spec. It doesn't mention how/where to insert a segment selector in the .debug_rnglists section.

"The segment size is given by the segment_selector_size field of the header, and the address size is given by the address_size field of the header. If the segment_selector_size field in the header is zero, the segment selector is omitted from the range list entries."

Any idea?

Are we deliberately not supporting non-zero segment selector size values again? There's no test coverage for tables with non-zero values.

The description of 'SegmentSelectorSize' isn't very clear to me in the spec. It doesn't mention how/where to insert a segment selector in the .debug_rnglists section.

"The segment size is given by the segment_selector_size field of the header, and the address size is given by the address_size field of the header. If the segment_selector_size field in the header is zero, the segment selector is omitted from the range list entries."

Any idea?

Huh, that's not great, and might be a spec bug again. @aprantl/@probinson, could either of you provide any enlightenment on how the segment is supposed to be encoded in the .debug_rnglists section, if at all? The spec seems to be silent on the matter. (For reference, for the .debug_aranges section the segment selector appears immediately before the address).

Higuoxing updated this revision to Diff 278653.Jul 16 2020, 8:03 PM
Higuoxing marked 5 inline comments as done.

Address comments & Make the debug_rnglists entry optional & Add test case (r).

I haven't got an exact answer about segment selctor in the range list entry from the Dwarf-Discuss list (http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2020-July/004646.html). But we're almost there, let's wait a little more time :-)

jhenderson accepted this revision.EditedJul 17 2020, 1:49 AM

Thanks, LGTM in its current form.

Address comments & Make the debug_rnglists entry optional & Add test case (r).

I haven't got an exact answer about segment selctor in the range list entry from the Dwarf-Discuss list (http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2020-July/004646.html). But we're almost there, let's wait a little more time :-)

That's quite an impressive discussion you managed to kick off there! I've read through it and there seem to be two possible outcomes: 1) A segment-selector-sized field should precede every address, representing the segment selection or 2) if the spec doesn't explicitly mention the segment selector, ignore the field (it will still be in the header, but have no effect). I'd be inclined to implement 2) (which is I think what this patch does), at least for now, but can always switch to 1) in the future, if the decision falls that way.

This revision is now accepted and ready to land.Jul 17 2020, 1:49 AM
Higuoxing edited the summary of this revision. (Show Details)Jul 19 2020, 6:37 PM
This revision was automatically updated to reflect the committed changes.