Page MenuHomePhabricator

[DWARFYAML][debug_aranges] Replace InitialLength with Format and Length.
ClosedPublic

Authored by Higuoxing on Jun 3 2020, 1:23 AM.

Details

Summary

This patch addresses the comment in D80972.

Before this patch, the initial length field of .debug_aranges section should be declared as:

## 32-bit DWARF
debug_aranges:
  - Length:
      TotalLength: 0x20
    Version: 2
    ...

## 64-bit DWARF
debug_aranges:
  - Length:
      TotalLength:   0xffffffff
      TotalLength64: 0x20
    Version: 2
    ...

After this patch:

## 32-bit DWARF
debug_aranges:
  - [[Format:  DWARF32]] ## Optional
    Length:  0x20
    Version: 2
    ...

## 64-bit DWARF
debug_aranges:
  - Format:  DWARF64
    Length:  0x20
    Version: 2

Current implementation of generating DWARF64 .debug_aranges section is buggy. A follow-up patch will improve it and add test cases for DWARF64.

Diff Detail

Unit TestsFailed

TimeTest
590 mslinux > libFuzzer.libFuzzer::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/premerge-debian-74f8f877f7-rl9hz-1/llvm-project/premerge-checks/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/mnt/disks/ssd0/agent/premerge-debian-74f8f877f7-rl9hz-1/llvm-project/premerge-checks/compiler-rt/lib/fuzzer -m64 /mnt/disks/ssd0/agent/premerge-debian-74f8f877f7-rl9hz-1/llvm-project/premerge-checks/compiler-rt/test/fuzzer/AccumulateAllocationsTest.cpp -o /mnt/disks/ssd0/agent/premerge-debian-74f8f877f7-rl9hz-1/llvm-project/premerge-checks/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/max-number-of-runs.test.tmp-AccumulateAllocationsTest
110 mswindows > LLVM-Unit.Support/_/SupportTests_exe::Unknown Unit Message ("")
Note: Google Test filter = YAMLVFSWriterTest.HandleRootAsVPath [==========] Running 1 test from 1 test case.

Event Timeline

Higuoxing created this revision.Jun 3 2020, 1:23 AM
jhenderson added inline comments.Jun 3 2020, 2:17 AM
llvm/include/llvm/ObjectYAML/DWARFYAML.h
68

I wouldn't bother with a length prefix. I'm not sure it's important for testing, because either the parser will read the DWARF64 marker (i.e. 0xffffffff) and treat the next 8 bytes as the rest of the length, or it won't in which case, it will treat those 4 bytes as the actual length, regardless of their value (with possible exceptions for the rest of the reserved range, but those values can be entered as the length in the YAML document anyway).

llvm/lib/ObjectYAML/DWARFYAML.cpp
78

I'd make this optional, with DWARF32 as the default. That's going to be the common case.

Higuoxing marked an inline comment as done.Jun 3 2020, 2:45 AM
Higuoxing added inline comments.
llvm/include/llvm/ObjectYAML/DWARFYAML.h
68

Sorry, I'm a little bit lost here. What do you mean by saying

"but those values can be entered as the length in the YAML document anyway"?

I'm not sure if I understand it correctly. The 12-bit initial length is valid only when the format is DWARF64 (the prefix is 0xffffffff). When the prefix is in 0xfffffff0-0xfffffffe, the DWARF parser will not interpret the last 8-byte as length?

So we don't need LengthPrefix here, right? We can test the prefix in the preserved range by crafting something like

Format: DWARF32
Length: 0xfffffff0
jhenderson added inline comments.Jun 3 2020, 3:29 AM
llvm/include/llvm/ObjectYAML/DWARFYAML.h
68

Yes, exactly. The LengthPrefix isn't needed at all, I believe. The users options are:

Format: DWARF32
Length: <some value, including special values, that will just be written in the 4 bytes range, interpreted as 32-bits>

(the parser should bail on values in the reserved range other than 0xffffffff, and not try to carry on reading)

or

Format: DWARF64 (implies 0xffffffff for first 4 bytes)
Length: <64-bit length value>

I guess there might be an edge case where people want DWARF32 bit 8-byte offsets or DWARF64 with 4-byte offsets, but let's not worry about that. If people actually need that behaviour, it can be added later.

Higuoxing updated this revision to Diff 268388.Jun 4 2020, 1:47 AM
Higuoxing marked 2 inline comments as done.
  • Make "Format" optional. (DWARF32 by default).
  • Remove LengthPrefix.

Test case for 64-bit will be added in a follow-up patch.

This comment was removed by Higuoxing.
Higuoxing edited the summary of this revision. (Show Details)Jun 4 2020, 1:55 AM
jhenderson accepted this revision.Jun 4 2020, 2:16 AM

LGTM, thanks.

Nit: in the title: replaced ... by -> replaced ... with

(It should be either "X is replaced by Y", or "Y replaces X", along with appropriate tense modifications etc)

This revision is now accepted and ready to land.Jun 4 2020, 2:16 AM
Higuoxing retitled this revision from [DWARFYAML][debug_aranges] Replace InitialLength by Format and Length. to [DWARFYAML][debug_aranges] Replace InitialLength with Format and Length..Jun 4 2020, 6:04 AM
This revision was automatically updated to reflect the committed changes.