This is analogous to D58943, which correctly finds the corresponding
fixup. However, when linker relaxations are disabled and we evaluate the
fixup, we need to also ensure we use an offset of 0 rather than the size
of the previous fragment.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
I don't understand what you're doing here.
You have a symbol pointing at the end (one past the last byte) of a fragment, so it's really pointing at the first byte of the next fragment. You want to handle that specially. That part seems okay... but I don't understand how simply zeroing out AUIPCOffset has the right effect.
Really, it's suspicious that this code is using Fixup->getOffset() at all: the offset isn't meaningful if you don't know what fragment is relative to. The "PC-relative" part of PC-relative relocations is supposed to be resolved by the caller, MCAssembler::evaluateFixup().
Comment Actions
I think ultimately, the problem here is that the relocation is marked FKF_IsPCRel. That has a specific meaning that doesn't apply here: the ultimate value encoded into the addi is based on the distance between two symbols: the symbol in the pcrel_lo, and the symbol in the corresponding pcrel_hi. The address of the addi itself isn't relevant.
Comment Actions
If you look at the implementation of getPCRelHiFixup, the fixup returned is normally the same fragment as the AUIPC symbol, which is what this code was relying on. However, the one case where it doesn't is the special case of being at the end of a fragment, where getPCRelHiFixup gets the fixup from the *next* fragment *at offset 0*. This logic therefore needs to be mirrored in evaluatePCRelLo so that they agree on what fragment they're talking about for the AUIPC fixup. The issue arises because .option (and other directives) are delayed until the next code/data, but emitting a symbol does not flush that, so the local symbols end up at the end of the previous fragment.
Comment Actions
Yes, FK_IsPCRel isn't really accurate for this, and we have talked in the past about maybe teaching LLVM about indirection in fixups, but the approach that's here has proved sufficient in all the cases we can come up with, and has the advantage of keeping this RISC-V-specific fixup internal to the backend.
Comment Actions
Perhaps this helps. Let's take:
nop
.option pop
1: auipc a1, %pcrel_hi(foo)
addi a1, a1, %pcrel_lo(1b)As fragments, this ends up being:
Fragment1 {
nop
.Ltmp0:
}
Fragment2 {
auipc a1, %pcrel_hi(foo)
addi a1, a1, %pcrel_lo(.Ltmp0)
}What this code is trying to do is make the %pcrel_lo(.Ltmp0) look like a more conventional %pcrel_lo(foo + (. - .Ltmp0)). To do so, it requires . and .Ltmp0 be in the same fragment, since getOffset for .Ltmp0 and the %pcrel_lo fixup both return their offset within the fragment. This is checked earlier by ensuring findAssociatedFragment() matches for each.
AUIPCSymbol here is .Ltmp0 and has offset 4 within Fragment1. We call getPCRelHiFixup, which sees that the symbol is at the end of the fragment, looks in the next one (Fragment2) at offset 0 and gives us back that %pcrel_hi(foo). The caller is however unaware that the fixups are in a different fragment to AUIPCSymbol and so should do the same check as happens inside getPCRelHiFixup so they agree on when to advance to the next fragment. The caller should thus see that .Ltmp0 is at the end of Fragment1, and that the AUIPC is therefore really at the very beginning of the next fragment, ie offset 0. This gives us the correct value for . - .Ltmp0 of 4 - 0 rather than 4 - 4.
Comment Actions
it requires . and .Ltmp0 be in the same fragment [...] This is checked earlier by ensuring findAssociatedFragment() matches for each.
That check you're mentioning doesn't actually do what you say it does; both findAssociatedFragment() actually return the fragment containing .Ltmp0.
This logic therefore needs to be mirrored in evaluatePCRelLo so that they agree on what fragment they're talking about for the AUIPC fixup.
This makes sense. Please add comments linking the two.
Comment Actions
Hm, you're right, I'm not sure what that's actually checking then, it seems like findAssociatedFragment by definition will end up using AUIPCSRE->findAssociatedFragment()?
This logic therefore needs to be mirrored in evaluatePCRelLo so that they agree on what fragment they're talking about for the AUIPC fixup.
This makes sense. Please add comments linking the two.
Actually, thinking about this more, since getPCRelHiFixup returns a non-null fixup if and only if its offset matches the (potentially zeroed because it's at the end of the fragment) AUIPCSymbol's offset, we should just use TargetFixup->getOffset and not have to worry about duplicating the condition? (But still with a comment in evaluatePCRelLo explaining why we have to use that and not the symbol's offset despite them looking very similar)
Comment Actions
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.
Comment Actions
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:
- 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...).
- Add a new validateRelocation hook so we can move our errors there.
- 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).
Comment Actions
Instead of returning a symbol difference from evaluatePCRelLo and folding it MCAssembler, can just fold the symbol difference to a constant in evaluatePCRelLo itself? MCExpr::evaluateAsRelocatableImpl does something like that in EvaluateSymbolicAdd.
If we can't do that for some reason, FKF_Target seems reasonable.