Page MenuHomePhabricator

[RISCV] Fix evaluating %pcrel_lo against global and weak symbols
ClosedPublic

Authored by jrtc27 on Jan 22 2020, 9:20 AM.

Details

Summary

Previously, we would erroneously turn %pcrel_lo(label), where label has
a %pcrel_hi against a weak symbol, into %pcrel_lo(label + offset), as
evaluatePCRelLo would believe the target independent logic was going to
fold it. Moreover, even if that were fixed, shouldForceRelocation lacks
an MCAsmLayout and thus cannot evaluate the %pcrel_hi fixup to a value
and check the symbol, so we would then erroneously constant-fold the
%pcrel_lo whilst leaving the %pcrel_hi intact. After D72197, this same
sequence also occurs for symbols with global binding, which is triggered
in real-world code.

Instead, as discussed in D71978, we introduce a new FKF_IsTarget flag to
avoid these kinds of issues. All the resolution logic happens in one
place, with no coordination required between RISCAsmBackend and
RISCVMCExpr to ensure they implement the same logic twice. Although the
implementation of %pcrel_hi can be left as target independent, we make
it target dependent to ensure that they are handled identically to
%pcrel_lo, otherwise we risk one of them being constant folded but the
other being preserved. This also allows us to properly support fixup
pairs where the instructions are in different fragments.

Diff Detail

Event Timeline

jrtc27 created this revision.Jan 22 2020, 9:20 AM

This approach seems much easier to understand.

Can you add a testcase for the case where the auipc and addi are in different fragments, like I mentioned in D71978?

llvm/include/llvm/MC/MCAsmBackend.h
114

Can the "Target" be a const reference?

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
279

MCAssembler::evaluateFixup emits an error if evaluateAsRelocatable fails... maybe worth adding a comment if you're intentionally not emitting a similar error?

283

If "Target.getSymB()" is true, should getPCRelHiFixup fail?

323

shouldForceRelocation(Asm, AUIPCFixup, AUIPCTarget)? (I think the difference is significant; please make sure we have a testcase that covers it.)

llvm/test/MC/RISCV/pcrel-fixups.s
86

Did you mean to make both global_function and weak_function weak?

jrtc27 marked 5 inline comments as done.Jan 22 2020, 4:45 PM

This approach seems much easier to understand.

Can you add a testcase for the case where the auipc and addi are in different fragments, like I mentioned in D71978?

Thanks for the quick review. I shall add that and address the comments.

llvm/include/llvm/MC/MCAsmBackend.h
114

Yes, it probably should be. In theory a backend could leave the fixup unresolved but mutate the target, however that seems rare (and is precisely what we're trying to avoid with RISC-V...). If a backend grows to need it it can always be de-consted later.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
279

Yes, it's deliberately omitted since evaluateFixup will emit the same error when it sees the real pcrel_hi. I will add a comment.

283

That was intended to be AUIPCTarget, but that is already handled below in general regardless of the variant kind so I will just remove this.

323

It currently doesn't matter, the intent was only to pick up the FeatureRelax etc code path. But you're right, if I pass AUIPCFixup, things are better, and I can delete the WasForced case statements above (validating it's a valid fixup should already have happened in getPCRelHiFixup).

llvm/test/MC/RISCV/pcrel-fixups.s
86

We both know the answer is no :) will fix.

jrtc27 updated this revision to Diff 239744.Jan 22 2020, 5:16 PM

Addressed review comments.

jrtc27 edited the summary of this revision. (Show Details)Jan 22 2020, 5:17 PM
jrtc27 updated this revision to Diff 239745.Jan 22 2020, 5:22 PM
jrtc27 marked 5 inline comments as done.

Pass correct target to shouldForceRelocation (although unused).

jrtc27 marked an inline comment as done.Jan 22 2020, 5:22 PM
jrtc27 added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
323

After removing the big switch earlier in the function, this is caught by at least one test in relocations.s which uses the GOT (and I assume also the TLS-related relocations later in the file), and it is now using the correct (AUIPC) fixup. It also uses the correct target, although that is not currently consulted so it does not matter.

Harbormaster completed remote builds in B44653: Diff 239745.
This revision is now accepted and ready to land.Jan 22 2020, 5:53 PM
This revision was automatically updated to reflect the committed changes.

@jrtc27 This should probably also go in the release branch or was the commit that exposed the problem made after the branch point?

asb added a comment.Jan 23 2020, 5:35 AM

I've filed a request to merge this into the release branch here https://bugs.llvm.org/show_bug.cgi?id=44631 - thanks for the fix James!