[RISCV] For Dwarf v5, emit indices into .debug_addr for range list entries

[RamNalamothu]

Summary

Currently the dwarf emission generates offset pair DW_{R,L}LE_offset_pair
relative to the base address for range list entries and these offsets are
resolved during assembing. But this leads to overlapping addresses in the
range lists when the code size changes due to relaxations during linking.

Hence, for dwarf v5, generate indices DW_{R,L}LE_startx_endx into the
.debug_addr for the range list entries which would need relocations and
we would end up with the correct address range when the relocations are
resolved during linking.

For non dwarf v5, we already generate symbol references, which would need
relocations, for range list entries.

[RamNalamothu]

Fix the pre-merge checks test failure.

[RamNalamothu]

Another attempt to fix the pre-merge check failure.

[dblaikie]

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?)

[tmsriram]

In 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.

[RamNalamothu]

Introduce an MCAsmInfo API to check if linker relaxation is enabled for generating relocatable symbol references.

[RamNalamothu]

In 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.

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.

[dblaikie]

In D140478#4012779, @RamNalamothu wrote:

In 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, @tmsriram wrote:

In 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?

[tmsriram]

In D140478#4015568, @dblaikie wrote:

In D140478#4012779, @RamNalamothu wrote:

In 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, @tmsriram wrote:

In 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

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!

[dblaikie]

In D140478#4023844, @tmsriram wrote:

In D140478#4015568, @dblaikie wrote:

In D140478#4012779, @RamNalamothu wrote:

In 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, @tmsriram wrote:

In 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.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2802

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?

2804

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)

2805–2809

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)

[RamNalamothu]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2802

Is this a subtarget feature? (can it vary per function?)

Yes, at least for RISC-V and looks like it is the only one using this feature at the moment.

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?

Looks like the "target-features"="-relax" or "target-features"="+relax" Function attributes have no effect which means the relaxation is either enabled or disabled for the entire module. Hence, I may not be able to produce such a test case.

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?

It appears that there is not enough context available to go from the symbol to the function/subtarget and hence we can't do a per address range check on whether the relaxation is enabled or not.

2804

Yes, something on those lines is possible but may be after https://reviews.llvm.org/D143645.

2805–2809

Yes, change is coming in the next update.

[RamNalamothu]

Address review comments.

[RamNalamothu]

Few lit tests will fail for using MCSubtargetInfo::checkFeatures without D143645.

[craig.topper]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1299

This also feels like it should be a virtual function.

llvm/lib/CodeGen/MachineFunction.cpp

635

This feels like there should be a virtual function somewhere that a target can override and answer using its own knowledge. Rather than using feature strings that don't exist on most targets.

[RamNalamothu]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1299

That's what I started with in the previous versions of this revision https://reviews.llvm.org/D140478?vs=on&id=484785#toc but @dblaikie mentioned an interesting point that -mrelax is a subtarget feature, which is true at least for RISC-V. Hence I thought having these checks per machine function as in MachineFunction::hasRelaxationsEnabled would be right/better. Please see my previous replies to @dblaikie.

I can introduce a virtual function in MCSubtargetInfo but that will defeat the purpose of MCSubtargetInfo::checkFeatures.

And another challenge is we don't have enough context in DwarfDebug to know if relaxations are enabled or not. We have to go through AsmPrinter.

[craig.topper]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1299

Even with your changes to MCSubtargetInfo::checkFeatures isn't it still printing a message to errs() on targets that aren't RISC-V? A message that isn't actionable by the compiler.

[RamNalamothu]

Created a virtual method in AsmPrinter instead of using MCSubtargetInfo::checkFeatures.

[dblaikie]

llvm/include/llvm/CodeGen/AsmPrinter.h

456–461

Does it make sense to forward-compat this by providing the two labels to this function, even if they're ignored for now? Is it plausible that some backend could determine whether a given range is relaxable or not from those labels? Guess we can always add that in later...

Maybe the name could be more about relaxation? "is this address range a compile-time constant" or "is this address range not subject to linker relaxation"?

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

254 ↗

(On Diff #500671)

Is this possible, even when the MachineFunction's subtarget has the relax feature? (I'd have thought the subtarget would already reflect/account for/be derived from the function's attributes?)

[dblaikie]

llvm/include/llvm/CodeGen/AsmPrinter.h

456–461

At least I think this function should take the MachineFunction to operate on, rather than using the one that might be set on the AsmPrinter - I would guess if you test this with a module with some functions with relaxation and some without, the current implementation doesn't get it correct - because we emit all these ranges together, so the AsmPrinter's current MF, if it has one, won't be correct for all of the ranges being emitted, right?

456–461

Which is to say - please include some test coverage showing a mixed module, demonstrating that the appropriate encodings are used for the appropriate functions (relocatable for relaxable functions, and non-relocatable for non-relaxable functions)

[RamNalamothu]

Remove unnecessary checks for relaxation flags in "target-features".

[RamNalamothu]

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Also, at the moment, the -mrelax/-mno-relax affects the entire compilation unit for the RISC-V. If that changes in future, I guess it will need many other changes as well and this will also get looked at then.

@craig.topper @asb

The latest update has a target hook, which is specialized for RISC-V, and the changes are effective for only RISC-V target.
Does this look good to go now?

llvm/include/llvm/CodeGen/AsmPrinter.h

456–461

Does it make sense to forward-compat this by providing the two labels to this function, even if they're ignored for now? Is it plausible that some backend could determine whether a given range is relaxable or not from those labels? Guess we can always add that in later...

Yeah, it's a simple change that can be added when needed.

Maybe the name could be more about relaxation? "is this address range a compile-time constant" or "is this address range not subject to linker relaxation"?

I am not getting ideas here. Feel free to suggest a name.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

254 ↗

(On Diff #500671)

I don't know for sure. I think what you said is true.

Will change the checks accordingly.

[dblaikie]

In D140478#4164792, @RamNalamothu wrote:

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Then won't this make the wrong decisions if the value varies between functions/subtargets?

Also, at the moment, the -mrelax/-mno-relax affects the entire compilation unit for the RISC-V.

Then wouldn't these be target features, rather than subtarget features? My understanding is that subtarget features generally can vary by function, whereas target features cannot.

Even if the command line flags don't support it, we could probably make a simple IR example with different relaxation per function? & so we probably should be able to handle that case.

[jrtc27]

In D140478#4165988, @dblaikie wrote:

In D140478#4164792, @RamNalamothu wrote:

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Then won't this make the wrong decisions if the value varies between functions/subtargets?

Also, at the moment, the -mrelax/-mno-relax affects the entire compilation unit for the RISC-V.

Then wouldn't these be target features, rather than subtarget features? My understanding is that subtarget features generally can vary by function, whereas target features cannot.

Even if the command line flags don't support it, we could probably make a simple IR example with different relaxation per function? & so we probably should be able to handle that case.

It's a per-function feature that has implications for other functions in the same section (deleting code in one function at link time will shunt around the code in the rest of the section).

[RamNalamothu]

Thanks for the clarification @jrtc27.

Does this patch need any further changes for it to be acceptable?

[dblaikie]

In D140478#4165992, @jrtc27 wrote:

In D140478#4165988, @dblaikie wrote:

In D140478#4164792, @RamNalamothu wrote:

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Then won't this make the wrong decisions if the value varies between functions/subtargets?

Also, at the moment, the -mrelax/-mno-relax affects the entire compilation unit for the RISC-V.

Then wouldn't these be target features, rather than subtarget features? My understanding is that subtarget features generally can vary by function, whereas target features cannot.

Even if the command line flags don't support it, we could probably make a simple IR example with different relaxation per function? & so we probably should be able to handle that case.

It's a per-function feature that has implications for other functions in the same section (deleting code in one function at link time will shunt around the code in the rest of the section).

In D140478#4165992, @jrtc27 wrote:

In D140478#4165988, @dblaikie wrote:

In D140478#4164792, @RamNalamothu wrote:

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Then won't this make the wrong decisions if the value varies between functions/subtargets?

Also, at the moment, the -mrelax/-mno-relax affects the entire compilation unit for the RISC-V.

Then wouldn't these be target features, rather than subtarget features? My understanding is that subtarget features generally can vary by function, whereas target features cannot.

Even if the command line flags don't support it, we could probably make a simple IR example with different relaxation per function? & so we probably should be able to handle that case.

It's a per-function feature that has implications for other functions in the same section (deleting code in one function at link time will shunt around the code in the rest of the section).

What does it mean for it to be per-function? Can it vary at all between functions in the same module? (if not, why is it per-function?)

[jrtc27]

In D140478#4168115, @dblaikie wrote:

In D140478#4165992, @jrtc27 wrote:

In D140478#4165988, @dblaikie wrote:

In D140478#4164792, @RamNalamothu wrote:

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Then won't this make the wrong decisions if the value varies between functions/subtargets?

Also, at the moment, the -mrelax/-mno-relax affects the entire compilation unit for the RISC-V.

Then wouldn't these be target features, rather than subtarget features? My understanding is that subtarget features generally can vary by function, whereas target features cannot.

Even if the command line flags don't support it, we could probably make a simple IR example with different relaxation per function? & so we probably should be able to handle that case.

It's a per-function feature that has implications for other functions in the same section (deleting code in one function at link time will shunt around the code in the rest of the section).

In D140478#4165992, @jrtc27 wrote:

In D140478#4165988, @dblaikie wrote:

In D140478#4164792, @RamNalamothu wrote:

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Then won't this make the wrong decisions if the value varies between functions/subtargets?

Also, at the moment, the -mrelax/-mno-relax affects the entire compilation unit for the RISC-V.

Then wouldn't these be target features, rather than subtarget features? My understanding is that subtarget features generally can vary by function, whereas target features cannot.

Even if the command line flags don't support it, we could probably make a simple IR example with different relaxation per function? & so we probably should be able to handle that case.

It's a per-function feature that has implications for other functions in the same section (deleting code in one function at link time will shunt around the code in the rest of the section).

What does it mean for it to be per-function? Can it vary at all between functions in the same module? (if not, why is it per-function?)

It governs whether or not that function's text relocations are paired with R_RISCV_RELAX to tell the linker it can relax them.

[dblaikie]

What does it mean for it to be per-function? Can it vary at all between functions in the same module? (if not, why is it per-function?)

It governs whether or not that function's text relocations are paired with R_RISCV_RELAX to tell the linker it can relax them.

OK, then it seems reasonable that this be handled for DWARF on a per-function basis. We shouldn't use an addr+addr representation for a function that won't change size at link time (equally needRelocSymbolsForAddrRange couldn't provide the right answer if it's not implemented per-function, right? Which Function does it consider? If it's not looking at the right function it might conclude there's no linker relaxation for the whole module, when one function does have relaxation)

If a label pair doesn't have enough info (you can get the relaxation from the MCSection, retrievable from the MCSymbol? Or is it a function of using a different kind of instruction or relocation (so not a property of the section or symbol) - in which case, the dwarf emission code could keep the MachineFunction or similar for each address range, then that could be passed into the callback and queried to determine if relaxation is enabled)

llvm/include/llvm/CodeGen/AsmPrinter.h

public:
   bool needsSEHMoves();
 
   /// Since emitting CFI unwind information is entangled with supporting the
   /// exceptions, this returns true for platforms which use CFI unwind
   /// information for debugging purpose when
   /// `MCAsmInfo::ExceptionsType == ExceptionHandling::None`.
   bool needsCFIForDebug() const;
 
+  // The symbol's address range will only be known after the linking when using
+  // relaxations. Hence, generate address range offsets as relocatable symbols
+  // and not as absolute delta from label difference as they might get resolved
+  // before linking.
+  bool needRelocSymbolsForAddrRange() const;
+
   /// Print to the current output stream assembly representations of the
   /// constants in the constant pool MCP. This is used to print out constants
   /// which have been "spilled to memory" by the code generator.
   virtual void emitConstantPool();
 
   /// Print assembly representations of the jump tables used by the current
   /// function to the current output stream.
   virtual void emitJumpTableInfo();

llvm/include/llvm/CodeGen/MachineFunction.h

public:
 
   /// Returns true if this function has basic block sections enabled.
   bool hasBBSections() const {
     return (BBSectionsType == BasicBlockSection::All ||
             BBSectionsType == BasicBlockSection::List ||
             BBSectionsType == BasicBlockSection::Preset);
   }
 
+  /// Returns true if this function has linker relaxations enabled.
+  bool hasRelaxationsEnabled() const;
+
   /// Returns true if basic block labels are to be generated for this function.
   bool hasBBLabels() const {
     return BBSectionsType == BasicBlockSection::Labels;
   }
 
   void setBBSectionsType(BasicBlockSection V) { BBSectionsType = V; }
 
   /// Assign IsBeginSection IsEndSection fields for basic blocks in this

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

bool AsmPrinter::needsSEHMoves() {
   return MAI->usesWindowsCFI() && MF->getFunction().needsUnwindTableEntry();
 }
 
 bool AsmPrinter::needsCFIForDebug() const {
   return MAI->getExceptionHandlingType() == ExceptionHandling::None &&
          MAI->doesUseCFIForDebug() && ModuleCFISection == CFISection::Debug;
 }
 
+bool AsmPrinter::needRelocSymbolsForAddrRange() const {
+  if (MF)
+    return MF->hasRelaxationsEnabled();
+  return TM.getMCSubtargetInfo()->checkFeatures("+relax");
+}
+
 void AsmPrinter::emitCFIInstruction(const MachineInstr &MI) {
   ExceptionHandling ExceptionHandlingType = MAI->getExceptionHandlingType();
   if (!needsCFIForDebug() &&
       ExceptionHandlingType != ExceptionHandling::DwarfCFI &&
       ExceptionHandlingType != ExceptionHandling::ARM)
     return;
 
   if (getFunctionCFISectionType(*MF) == CFISection::None)

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

static MCSymbol *emitLoclistsTableHeader(AsmPrinter *Asm,
   for (const auto &List : DebugLocs.getLists())
     Asm->emitLabelDifference(List.Label, DebugLocs.getSym(),
                              Asm->getDwarfOffsetByteSize());
 
   return TableEnd;
 }
 
 template <typename Ranges, typename PayloadEmitter>
-static void emitRangeList(
-    DwarfDebug &DD, AsmPrinter *Asm, MCSymbol *Sym, const Ranges &R,
-    const DwarfCompileUnit &CU, unsigned BaseAddressx, unsigned OffsetPair,
-    unsigned StartxLength, unsigned EndOfList,
-    StringRef (*StringifyEnum)(unsigned),
-    bool ShouldUseBaseAddress,
-    PayloadEmitter EmitPayload) {
+static void
+emitRangeList(DwarfDebug &DD, AsmPrinter *Asm, MCSymbol *Sym, const Ranges &R,
+              const DwarfCompileUnit &CU, unsigned BaseAddressx,
+              unsigned OffsetPair, unsigned StartxLength, unsigned StartxEndx,
+              unsigned EndOfList, StringRef (*StringifyEnum)(unsigned),
+              bool ShouldUseBaseAddress, PayloadEmitter EmitPayload) {
 
   auto Size = Asm->MAI->getCodePointerSize();
   bool UseDwarf5 = DD.getDwarfVersion() >= 5;
 
   // Emit our symbol so we can find the beginning of the range.
   Asm->OutStreamer->emitLabel(Sym);
 
   // Gather all the ranges that apply to the same section so they can share
   // a base address entry.
   MapVector<const MCSection *, std::vector<decltype(&*R.begin())>> SectionRanges;
 
   for (const auto &Range : R)
     SectionRanges[&Range.Begin->getSection()].push_back(&Range);
 
+  bool NeedRelocSymbols = UseDwarf5 && Asm->needRelocSymbolsForAddrRange();
+  if (NeedRelocSymbols)
+    ShouldUseBaseAddress = false;
+
   const MCSymbol *CUBase = CU.getBaseAddress();
   bool BaseIsSet = false;
   for (const auto &P : SectionRanges) {
-    auto *Base = CUBase;
+    auto *Base = NeedRelocSymbols ? nullptr : CUBase;
     if (!Base && ShouldUseBaseAddress) {
       const MCSymbol *Begin = P.second.front()->Begin;
       const MCSymbol *NewBase = DD.getSectionLabel(&Begin->getSection());
       if (!UseDwarf5) {
         Base = NewBase;
         BaseIsSet = true;
         Asm->OutStreamer->emitIntValue(-1, Size);
         Asm->OutStreamer->AddComment("  base address");
         Asm->OutStreamer->emitSymbolValue(Base, Size);
       } else if (NewBase != Begin || P.second.size() > 1) {
         // Only use a base address if
         //  * the existing pool address doesn't match (NewBase != Begin)
         //  * or, there's more than one entry to share the base address
for (const auto *RS : P.second) {
           Asm->emitLabelDifferenceAsULEB128(Begin, Base);
           Asm->OutStreamer->AddComment("  ending offset");
           Asm->emitLabelDifferenceAsULEB128(End, Base);
         } else {
           Asm->emitLabelDifference(Begin, Base, Size);
           Asm->emitLabelDifference(End, Base, Size);
         }
       } else if (UseDwarf5) {
+        if (NeedRelocSymbols) {
+          // Generate indices into .debug_addr so they will need relocations
+          Asm->OutStreamer->AddComment(StringifyEnum(StartxEndx));
+          Asm->emitInt8(StartxEndx);
+          Asm->OutStreamer->AddComment("  start index");
+          Asm->emitULEB128(DD.getAddressPool().getIndex(Begin));
+          Asm->OutStreamer->AddComment("  end index");
+          Asm->emitULEB128(DD.getAddressPool().getIndex(End));
+        } else {
           Asm->OutStreamer->AddComment(StringifyEnum(StartxLength));
           Asm->emitInt8(StartxLength);
           Asm->OutStreamer->AddComment("  start index");
           Asm->emitULEB128(DD.getAddressPool().getIndex(Begin));
           Asm->OutStreamer->AddComment("  length");
           Asm->emitLabelDifferenceAsULEB128(End, Begin);
+        }
       } else {
         Asm->OutStreamer->emitSymbolValue(Begin, Size);
         Asm->OutStreamer->emitSymbolValue(End, Size);
       }
       EmitPayload(*RS);
     }
   }
 
   if (UseDwarf5) {
     Asm->OutStreamer->AddComment(StringifyEnum(EndOfList));
     Asm->emitInt8(EndOfList);
   } else {
     // Terminate the list with two 0 values.
     Asm->OutStreamer->emitIntValue(0, Size);
     Asm->OutStreamer->emitIntValue(0, Size);
   }
 }
 
 // Handles emission of both debug_loclist / debug_loclist.dwo
 static void emitLocList(DwarfDebug &DD, AsmPrinter *Asm, const DebugLocStream::List &List) {
-  emitRangeList(DD, Asm, List.Label, DD.getDebugLocs().getEntries(List),
-                *List.CU, dwarf::DW_LLE_base_addressx,
-                dwarf::DW_LLE_offset_pair, dwarf::DW_LLE_startx_length,
-                dwarf::DW_LLE_end_of_list, llvm::dwarf::LocListEncodingString,
-                /* ShouldUseBaseAddress */ true,
-                [&](const DebugLocStream::Entry &E) {
-                  DD.emitDebugLocEntryLocation(E, List.CU);
-                });
+  emitRangeList(
+      DD, Asm, List.Label, DD.getDebugLocs().getEntries(List), *List.CU,
+      dwarf::DW_LLE_base_addressx, dwarf::DW_LLE_offset_pair,
+      dwarf::DW_LLE_startx_length, dwarf::DW_LLE_startx_endx,
+      /* ShouldUseBaseAddress */ true, [&](const DebugLocStream::Entry &E) {
 }
 
 void DwarfDebug::emitDebugLocImpl(MCSection *Sec) {
   if (DebugLocs.getLists().empty())
     return;
 
   Asm->OutStreamer->switchSection(Sec);
 
void DwarfDebug::emitDebugARanges() {
   }
 }
 
 /// Emit a single range list. We handle both DWARF v5 and earlier.
 static void emitRangeList(DwarfDebug &DD, AsmPrinter *Asm,
                           const RangeSpanList &List) {
   emitRangeList(DD, Asm, List.Label, List.Ranges, *List.CU,
                 dwarf::DW_RLE_base_addressx, dwarf::DW_RLE_offset_pair,
-                dwarf::DW_RLE_startx_length, dwarf::DW_RLE_end_of_list,
-                llvm::dwarf::RangeListEncodingString,
+                dwarf::DW_RLE_startx_length, dwarf::DW_RLE_startx_endx,
+                dwarf::DW_RLE_end_of_list, llvm::dwarf::RangeListEncodingString,
                 List.CU->getCUNode()->getRangesBaseAddress() ||
                     DD.getDwarfVersion() >= 5,
                 [](auto) {});
 }
 
 void DwarfDebug::emitDebugRangesImpl(const DwarfFile &Holder, MCSection *Section) {
   if (Holder.getRangeLists().empty())
     return;

llvm/lib/CodeGen/MachineFunction.cpp

 
 /// True if this function needs frame moves for debug or exceptions.
 bool MachineFunction::needsFrameMoves() const {
   return getMMI().hasDebugInfo() ||
          getTarget().Options.ForceDwarfFrameSection ||
          F.needsUnwindTableEntry();
 }
 
+bool MachineFunction::hasRelaxationsEnabled() const {
+  if (getSubtarget().checkFeatures("-relax"))
+    return false;
+
+  if (F.hasFnAttribute("target-features")) {
+    StringRef FS = F.getFnAttribute("target-features").getValueAsString();
+    if (FS.contains("+relax"))
+      return true;
+    if (FS.contains("-relax"))
+      return false;
+  }
+
+  return getSubtarget().checkFeatures("+relax");
+}
+
 namespace llvm {
 
   template<>
   struct DOTGraphTraits<const MachineFunction*> : public DefaultDOTGraphTraits {
     DOTGraphTraits(bool isSimple = false) : DefaultDOTGraphTraits(isSimple) {}
 
     static std::string getGraphName(const MachineFunction *F) {
       return ("CFG for '" + F->getName() + "' function").str();

llvm/test/DebugInfo/RISCV/dwarf-riscv-range-list-relocs.ll

+; REQUIRES: riscv-registered-target
+; The LLVM IR is generated from the following source code with "clang -S -emit-llvm -target riscv32 -g -mrelax -ffunction-sections"
+; int baz()
+; 
+; { return 0; }
+; int foo()
+;
+; { return baz(); }
+; int bar()
+;
+; { return foo(); }
+
+
+; RUN: llc -filetype=asm -mtriple=riscv32 -mattr="+relax" -target-abi=ilp32 -dwarf-version=5 --function-sections %s -o %t.s
+; RUN: FileCheck -check-prefix=ASM -input-file %t.s %s
+; RUN: llc -filetype=obj -mtriple=riscv32 -mattr="+relax" -target-abi=ilp32 -dwarf-version=5 --function-sections %s -o %t.o
+; RUN: llvm-dwarfdump --verify %t.o
+; RUN: llvm-readobj -r %t.o | FileCheck -check-prefix=OBJ %s
+; RUN: ld.lld %t.o -o %t.out
+; RUN: llvm-dwarfdump --verify %t.out
+; RUN: llvm-dwarfdump --debug-info --debug-addr --debug-rnglists %t.out | FileCheck -check-prefix=DWARFDUMP %s
+
+
+; When using relaxations, since the symbol's actual address range will only be
+; known after linking, make sure we do not generate address range offsets as
+; label differences as they might get resolved before linking. Instead we
+; generate indices into the .debug_addr so that they will need relocations which
+; will be resolved during linking.
+
+; ASM:              .section        .debug_info,"",@progbits
+; ASM:      .Ldebug_info_start0:
+; ASM-NEXT:         .half   5                               # DWARF version number
+; ASM:              .word   0                               # DW_AT_low_pc
+; ASM-NEXT:         .byte   0                               # DW_AT_ranges
+; ASM-NEXT:         .word   .Laddr_table_base0              # DW_AT_addr_base
+; ASM-NEXT:         .word   .Lrnglists_table_base0          # DW_AT_rnglists_base
+
+; ASM:              .section        .debug_rnglists,"",@progbits
+; ASM:      .Lrnglists_table_base0:
+; ASM-NEXT:         .word   .Ldebug_ranges0-.Lrnglists_table_base0
+; ASM-NEXT: .Ldebug_ranges0:
+; ASM-NEXT:         .byte   2                               # DW_RLE_startx_endx
+; ASM-NEXT:         .byte   0                               #   start index
+; ASM-NEXT:         .byte   3                               #   end index
+; ASM-NEXT:         .byte   2                               # DW_RLE_startx_endx
+; ASM-NEXT:         .byte   1                               #   start index
+; ASM-NEXT:         .byte   4                               #   end index
+; ASM-NEXT:         .byte   2                               # DW_RLE_startx_endx
+; ASM-NEXT:         .byte   2                               #   start index
+; ASM-NEXT:         .byte   5                               #   end index
+; ASM-NEXT:         .byte   0                               # DW_RLE_end_of_list
+; ASM-NEXT: .Ldebug_list_header_end0:
+
+; ASM:              .section        .debug_addr,"",@progbits
+; ASM:      .Laddr_table_base0:
+; ASM-NEXT:        .word   .Lfunc_begin0
+; ASM-NEXT:        .word   .Lfunc_begin1
+; ASM-NEXT:        .word   .Lfunc_begin2
+; ASM-NEXT:        .word   .Lfunc_end0
+; ASM-NEXT:        .word   .Lfunc_end1
+; ASM-NEXT:        .word   .Lfunc_end2
+
+
+; OBJ:       Section ({{.*}}) .rela.debug_addr {
+; OBJ-NEXT:    0x8 R_RISCV_32 - 0x0
+; OBJ-NEXT:    0xC R_RISCV_32 - 0x0
+; OBJ-NEXT:    0x10 R_RISCV_32 - 0x0
+; OBJ-NEXT:    0x14 R_RISCV_32 - 0x0
+; OBJ-NEXT:    0x18 R_RISCV_32 - 0x0
+; OBJ-NEXT:    0x1C R_RISCV_32 - 0x0
+; OBJ-NEXT:  }
+
+
+; DWARFDUMP:                    DW_TAG_compile_unit
+; DWARFDUMP-NEXT:               DW_AT_producer    ("clang")
+; DWARFDUMP-NEXT:               DW_AT_language    (DW_LANG_C11)
+; DWARFDUMP-NEXT:               DW_AT_name        ("dwarf-riscv-range-list-relocs.c")
+; DWARFDUMP-NEXT:               DW_AT_str_offsets_base    (0x00000008)
+; DWARFDUMP-NEXT:               DW_AT_stmt_list   (0x00000000)
+; DWARFDUMP-NEXT:               DW_AT_comp_dir    (".")
+; DWARFDUMP-NEXT:               DW_AT_low_pc      (0x00000000)
+; DWARFDUMP-NEXT:               DW_AT_ranges      (indexed (0x0) rangelist = 0x00000010
+; DWARFDUMP-NEXT:                  [0x[[R1_BEGIN:.*]], 0x[[R1_END:.*]])
+; DWARFDUMP-NEXT:                  [0x[[R2_BEGIN:.*]], 0x[[R2_END:.*]])
+; DWARFDUMP-NEXT:                  [0x[[R3_BEGIN:.*]], 0x[[R3_END:.*]]))
+; DWARFDUMP-NEXT:               DW_AT_addr_base   (0x00000008)
+; DWARFDUMP-NEXT:               DW_AT_rnglists_base       (0x0000000c)
+
+; DWARFDUMP:      .debug_addr contents
+; DWARFDUMP-NEXT: Address table header: length = 0x0000001c, format = DWARF32, version = 0x0005, addr_size = 0x04, seg_size = 0x00
+; DWARFDUMP-NEXT: Addrs: [
+; DWARFDUMP-NEXT: 0x[[R1_BEGIN]]
+; DWARFDUMP-NEXT: 0x[[R2_BEGIN]]
+; DWARFDUMP-NEXT: 0x[[R3_BEGIN]]
+; DWARFDUMP-NEXT: 0x[[R1_END]]
+; DWARFDUMP-NEXT: 0x[[R2_END]]
+; DWARFDUMP-NEXT: 0x[[R3_END]]
+; DWARFDUMP-NEXT: ]
+
+; DWARFDUMP:      .debug_rnglists contents
+; DWARFDUMP-NEXT: range list header: length = 0x00000016, format = DWARF32, version = 0x0005, addr_size = 0x04, seg_size = 0x00, offset_entry_count = 0x00000001
+; DWARFDUMP-NEXT: offsets: [
+; DWARFDUMP-NEXT: 0x00000004
+; DWARFDUMP-NEXT: ]
+; DWARFDUMP-NEXT: ranges:
+; DWARFDUMP-NEXT: [0x[[R1_BEGIN]], 0x[[R1_END]])
+; DWARFDUMP-NEXT: [0x[[R2_BEGIN]], 0x[[R2_END]])
+; DWARFDUMP-NEXT: [0x[[R3_BEGIN]], 0x[[R3_END]])
+; DWARFDUMP-NEXT: <End of list>
+
+
+; ModuleID = 'dwarf-riscv-range-list-relocs.c'
+source_filename = "dwarf-riscv-range-list-relocs.c"
+target datalayout = "e-m:e-p:32:32-i64:64-n32-S128"
+target triple = "riscv32"
+
+; Function Attrs: noinline nounwind optnone
+define dso_local i32 @baz() #0 !dbg !9 {
+entry:
+  ret i32 0, !dbg !14
+}
+
+; Function Attrs: noinline nounwind optnone
+define dso_local i32 @foo() #0 !dbg !15 {
+entry:
+  %call = call i32 @baz(), !dbg !16
+  ret i32 %call, !dbg !17
+}
+
+; Function Attrs: noinline nounwind optnone
+define dso_local i32 @bar() #0 !dbg !18 {
+entry:
+  %call = call i32 @foo(), !dbg !19
+  ret i32 %call, !dbg !20
+}
+
+attributes #0 = { noinline nounwind optnone "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic-rv32" "target-features"="+32bit,+a,+c,+m,+relax,-save-restore" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5, !6, !7}
+!llvm.ident = !{!8}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "dwarf-riscv-range-list-relocs.c", directory: ".", checksumkind: CSK_MD5, checksum: "b97b465da8a801f3455e96db63ead01f")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 1, !"target-abi", !"ilp32"}
+!6 = !{i32 7, !"frame-pointer", i32 2}
+!7 = !{i32 1, !"SmallDataLimit", i32 8}
+!8 = !{!"clang"}
+!9 = distinct !DISubprogram(name: "baz", scope: !1, file: !1, line: 1, type: !10, scopeLine: 3, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !13)
+!10 = !DISubroutineType(types: !11)
+!11 = !{!12}
+!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!13 = !{}
+!14 = !DILocation(line: 3, column: 3, scope: !9)
+!15 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 4, type: !10, scopeLine: 6, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !13)
+!16 = !DILocation(line: 6, column: 10, scope: !15)
+!17 = !DILocation(line: 6, column: 3, scope: !15)
+!18 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 7, type: !10, scopeLine: 9, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !13)
+!19 = !DILocation(line: 9, column: 10, scope: !18)
+!20 = !DILocation(line: 9, column: 3, scope: !18)