[PATCH] D71978: [RISCV] Fix evalutePCRelLo for symbols at the end of a fragment

[James Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

In D71978#1809215 <https://reviews.llvm.org/D71978#1809215>, @efriedma wrote:

> LGTM
>
> > the approach that's here has proved sufficient in all the cases we can come up with
>
> I'm still sort of concerned we silently miscompile when the addi is in a different fragment from the auipc, but maybe that doesn't matter much in practice.


Yes, you're right, I can come up with broken trivial test cases. Some of our existing unit tests actually have invalid input, too (apparently we silently allow `%pcrel_lo(foo)` if foo is undefined or a constant literal), both of which should be forbidden, although they do assemble to the right thing with a relocation... but we probably don't want to allow any of those, even if GNU as is lax here, as BFD will bork at runtime (although I think our LLD would support it if there happened to be a `%pcrel_hi` at the the now-defined target at link time). If we turn the `%pcrel_lo` into a symbol difference expression, remove the `FK_IsPCRel` from `fixup_pcrel_lo12_[is]`, and teach `MCAssembler::evaluateFixup` it can fold symbol difference expressions (as we already do in e.g. `ELFObjectWriter::recordRelocation` too late, but using the generic `isSymbolRefDifferenceFullyResolved` hook), we get the right computation for valid cases, get some new correct errors (because the constant literal case is now believed resolved and fails in `shouldForceRelocation`), but regress on detecting other errors, because the assembler no longer thinks they're resolved and so we never call `shouldForceRelocation`. I think we can fix this in one of two (three?) ways:

1. Have a flag that forces IsResolved to be true so we always call shouldForceRelocation for pcrel_lo (also negates the need to teach MCAssembler::evaluateFixup it can fold symbol difference expressions, although that's probably something it should know anyway...).

2. Add a new validateRelocation hook so we can move our errors there.

3. Add a new FKF_Target flag and resolveTargetRelocation which just delegates the resolving to the backend entirely (probably after evaluation). This would also allow us to avoid all this pcrel_lo hackery altogether and have it always evaluate to the AUIPC symbol, I believe, without having to encode any further knowledge of it in the generic assembler beyond "it's special, go ask the target".

You can of course come up with other variants. I tried 1 and got it to pass all the MC/RISCV tests, but I don't think that's the nicest solution, as it's not really the right intent. I feel like 3 is probably the best. Thoughts? http://paste.debian.net/1125099/ is the preliminary implementation of 1 for what it's worth (and shows what tests need to change, but I think that should be true regardless of which option we pick).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71978/new/

https://reviews.llvm.org/D71978