Page MenuHomePhabricator

[RISCV] Don't fold symbol diff
ClosedPublic

Authored by edward-jones on Apr 18 2018, 8:39 AM.

Details

Summary

When emitting the difference between two symbols, the standard behavior is that the difference will be resolved to an absolute value if both of the symbols are offsets from the same data fragment. This is undesirable on architectures such as RISC-V where relaxation in the linker may cause the computed difference to become invalid. This caused an issue when compiling to object code, where the size of a function in the debug information was already calculated even though it could change as a consequence of relaxation in the subsequent linking stage.

This patch inhibits the resolution of symbol differences to absolute values where the target's AsmBackend has declared that it does not want these to be folded.

Diff Detail

Repository
rL LLVM

Event Timeline

edward-jones created this revision.Apr 18 2018, 8:39 AM
aprantl removed a subscriber: aprantl.Apr 18 2018, 9:36 AM
asb added a comment.Apr 25 2018, 8:47 AM

Thanks, this seems sensible to me. Could you add a test please?

Working on creating a test at the moment, however while trying to create a test I realized this change doesn't actually work when using llc to generate an object file. It seems that when compiling with llc, the RISCV::FeatureRelax bit is never set, so RISCV::requiresDiffExpressionRelocations always returns false. This happens even with "target-features"="+relax" in the function attributes.

Added a test

kito-cheng added inline comments.May 10 2018, 8:30 PM
lib/MC/MCObjectStreamer.cpp
59 ↗(On Diff #144682)

I got compile error here:

MCObjectStreamer.cpp:94:7: error: ‘TAB’ was not declared in this scope

simoncook requested changes to this revision.May 11 2018, 3:28 AM
simoncook added inline comments.
lib/MC/MCObjectStreamer.cpp
59 ↗(On Diff #144682)

Hi Kito. Ed is out of the office today so I can't update this diff right now (I'll ask him to do this on Monday), but I've rebased it and this line should now be

if (getAssembler().getBackendPtr()->requiresDiffExpressionRelocations())
This revision now requires changes to proceed.May 11 2018, 3:28 AM
asb accepted this revision.May 17 2018, 2:13 AM

Looks good to me, thanks!

include/llvm/MC/MCObjectStreamer.h
172 ↗(On Diff #146558)

hi -> Hi

asb added inline comments.May 17 2018, 2:19 AM
include/llvm/MC/MCObjectStreamer.h
172 ↗(On Diff #146558)

On second thought, I'd be tempted to keep this helper as a static function and add an MCAssembler & parameter to it. It only has two callsites so it's not much hassle to update.

asb added a comment.May 23 2018, 6:02 AM

What did you think about keeping absoluteSymbolDiff as static and updating the callsites?

In D45773#1109391, @asb wrote:

What did you think about keeping absoluteSymbolDiff as static and updating the callsites?

Agreed, keeping it static is a better idea. I'll make the change and update this patch.

Updated to incorporate Alex's suggestion - keeping absoluteSymbolDiff static

asb accepted this revision.May 31 2018, 7:32 AM

Thanks Ed, looks good to me.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 16 2018, 4:27 AM
This revision was automatically updated to reflect the committed changes.