[PATCH] D140478: [RISCV] For Dwarf v5, emit indices into .debug_addr for range list entries
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 9 12:26:10 PST 2023
dblaikie added a comment.
In D140478#4023844 <https://reviews.llvm.org/D140478#4023844>, @tmsriram wrote:
> In D140478#4015568 <https://reviews.llvm.org/D140478#4015568>, @dblaikie wrote:
>
>> In D140478#4012779 <https://reviews.llvm.org/D140478#4012779>, @RamNalamothu wrote:
>>
>>> In D140478#4011085 <https://reviews.llvm.org/D140478#4011085>, @dblaikie wrote:
>>>
>>>> Is there any mechanism that tests whether a given range bounded by two labels may be subject to linker relaxation? It'd be nice to have that abstraction and use it here (& in some other places in DWARF emission) rather than hardcoding one particular architecture.
>>>
>>> Unless the relaxations are performed during the assembling, this may not be possible before linking because we will not know which set of labels have instruction sequence(s) that the linker actually might be able to relax after finishing the section layout.
>>> So, depending on whether linker relaxations are enabled, I think we can only either emit relocatable symbols for all the range labels or never.
>>
>> Yeah, such a query wouldn't have to be precise, only conservatively correct - so if you can't be sure a given range won't be relaxed, then the query would say that it might be & DWARF would be emitted that'd be resilient to such relaxation.
>>
>>>> The other place this would be needed is in the low/high_pc computation, since currently, even in DWARFv4, the high_pc is encoded as a delta from low_pc, not as a separate relocatable label, so that'd need the same checking.
>>>
>>> I have introduced a new MCAsmInfo API to check if we need relocatable symbol references in dwarf entries based on whether the linker relaxations are enabled.
>>
>>
>>
>> In D140478#4011149 <https://reviews.llvm.org/D140478#4011149>, @tmsriram wrote:
>>
>>> In D140478#4011085 <https://reviews.llvm.org/D140478#4011085>, @dblaikie wrote:
>>>
>>>> Is there any mechanism that tests whether a given range bounded by two labels may be subject to linker relaxation? It'd be nice to have that abstraction and use it here (& in some other places in DWARF emission) rather than hardcoding one particular architecture.
>>>>
>>>> The other place this would be needed is in the low/high_pc computation, since currently, even in DWARFv4, the high_pc is encoded as a delta from low_pc, not as a separate relocatable label, so that'd need the same checking.
>>>>
>>>> And I was thinking that PROPELLER needed/was considering doing some linker relaxation too, so could benefit from such an abstraction/hook/check - but maybe they didn't end up needing/using linker relaxation? (@tmsriram - did PROPELLER ever end up needing/using linker relaxation? is that still something under consideration?)
>>>
>>> Propeller needs linker relaxation for branch instructions. Making it a shorter branch when the offset can fit in one byte and flipping branches, from like je to jne to convert and remove a subsequent fallthrough.
>>>
>>> I remember dealing with issues regarding low_pc, high_pc. I think you reviewed these patches, I forget the details now and can take another look to see how this can help.
>>
>> Hmm, yeah, I'm confused - because I'd expect the code for ranges to have to handle this but judging by the code being added in this patch, the code isn't already there that I'd expect to be there for linker relaxation in PROPELLER.
>>
>> Could you point me to the reviews and/or a simple-ish example that would reproduce the relaxation behavior for PROPELLER so I can see how the DWARF is emitted/works uot?
>
> The one I was remembering: https://reviews.llvm.org/D85085
Ah, yeah, that handles a correct source variable location range descirptions when a function is split over multiple sections (so we can't use length offsets relative to the start of the variable's range - because some parts of the variable's range aren't relative to the start, they're independent due to being in another section)
But that doesn't handle the relaxation issue of shrinking code.
> Basically, the relaxation is part of basic block sections. Here is a simple example:
>
> int glob;
> int main() {
> if (glob > 5) {
> __builtin_printf("Large");
> }
> if (glob > 3) {
> __builtin_printf("Medium");
> }
> return 0;
> }
>
> # Without relaxation
> $ clang -fbasic-block-sections=all foo.cc -O2 -fuse-ld=lld
>
> # With relaxation
> $ clang -fbasic-block-sections=all foo.cc -O2 -fuse-ld=lld -Wl,--optimize-bb-jumps
>
> My guess is relaxation will expose some debug info issues unaddressed in the original debug info patch. Currently, Propeller isn't using this relaxation as it only does intra-procedural reordering via clusters. We are looking at inter-procedural reordering which will need relaxation. Thanks for taking a look!
Yeah, looking at the DWARF it doesn't account for potential relaxation:
0x00000014: [DW_RLE_startx_length]: 0x0000000000000003, 0x0000000000000019 => [0x0000000000000000, 0x0000000000000019)
0x00000017: [DW_RLE_startx_length]: 0x0000000000000004, 0x000000000000000e => [0x0000000000000000, 0x000000000000000e)
0x0000001a: [DW_RLE_startx_length]: 0x0000000000000005, 0x0000000000000013 => [0x0000000000000000, 0x0000000000000013)
0x0000001d: [DW_RLE_startx_length]: 0x0000000000000006, 0x0000000000000004 => [0x0000000000000000, 0x0000000000000004)
0x00000020: [DW_RLE_startx_length]: 0x0000000000000007, 0x0000000000000015 => [0x0000000000000000, 0x0000000000000015)
0x00000023: [DW_RLE_end_of_list ]
This is the range of addresses for the `main` function (& for the overall compilation unit, since they're identical) - notice that it uses the `startx_length` encoding which uses a relocatable address for the start of the range, then a fixed length.
Looking at what this translates to - we can see the overlap between sections due to that relaxation when compiling with `--optimize-bb-jumps`:
DW_AT_ranges [DW_FORM_rnglistx] (indexed (0x1) rangelist = 0x00000024
[0x00000000000017d0, 0x00000000000017e9)
[0x00000000000017e4, 0x00000000000017f2)
[0x00000000000017ed, 0x0000000000001800)
[0x00000000000017fb, 0x00000000000017ff)
[0x00000000000017c0, 0x00000000000017d5))
We can see the ranges have become overlapping.
so, yeah, PROPELLER will need some way to specify that certain ranges may be subject to relaxation, and those would need to use the startx_endx encoding - which is bigger.
So that explains why there's no logic here already to handle this case and so we're forging new ground in this patch.
================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2773
+ const MCSubtargetInfo *STI =
+ Asm->MF ? &(Asm->getSubtargetInfo()) : Asm->TM.getMCSubtargetInfo();
+ const bool NeedRelocSymbols =
----------------
Is this a subtarget feature? (can it vary per function?)
I guess/hope eventually it's a per address range property - though I don't know which target/subtarget/etc should be making the choice, given an address range, about whether it can be relaxed.
I guess it is a subtarget feature - based on the implementation of `doDwarfAddrRangesNeedRelocSymbols` - so maybe a test case with a single CU with one function that doesn't relax, and one that does?
I'm guessing this'll be more complicated to implement that correctly - probably require being able to go from the start symbol to the function, to query its subtarget - and doing that inside the `SectionRanges` search below, so that we use start_end when necessary, even if it isn't the default/current subtarget?
================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2775
+ const bool NeedRelocSymbols =
+ Asm->MAI->doDwarfAddrRangesNeedRelocSymbols(STI);
const MCSymbol *CUBase = CU.getBaseAddress();
----------------
Rather than passing the subtarget to the asm info, could this be a virtual function on the subtarget directly? (I don't know enough about all these abstractions, maybe there's a good reason to keep the choice in the AsmInfo)
================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2780-2784
+ if (NeedRelocSymbols) {
+ Base = nullptr;
+ BaseIsSet = false;
+ ShouldUseBaseAddress = false;
+ }
----------------
Could we avoid the need for doing this in each iteration of the loop by doing it once before the loop (if `CUBase` is set to null, and `ShouldUseBaseAddress` is set to false, then everything else should work out OK, I think? The base will never be set and base-relative forms will never be used)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140478/new/
https://reviews.llvm.org/D140478
More information about the llvm-commits
mailing list