Page MenuHomePhabricator

[RISCV] Properly evaluate fixup_riscv_pcrel_lo12
AcceptedPublic

Authored by PkmX on Nov 2 2018, 1:16 AM.

Details

Summary

This is a update to D43157 to correctly handle fixup_riscv_pcrel_lo12.

Notable changes:

  • Rebased onto trunk
  • Handle and test S-type
  • Test case pcrel-hilo.s is merged into relocations.s

Diff Detail

Event Timeline

PkmX created this revision.Nov 2 2018, 1:16 AM
PkmX updated this revision to Diff 174879.Wed, Nov 21, 12:17 AM

Rebase onto trunk as the declaration of RISCVAsmBackend is split into its own header file.

@asb Any chance to look at this patch?

jrtc27 added a subscriber: jrtc27.Thu, Dec 6, 5:49 AM
jrtc27 added inline comments.Thu, Dec 6, 6:24 AM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
44

getPCRelHiExpr can return null if you do stupid things in your assembly (ie use %pcrel_lo(foo) where foo doesn't refer to an auipc x?, %pcrel_hi(sym) but is still at a constant offset), and in future most definitely will be when using %got_pcrel_hi.

asb added inline comments.Thu, Dec 6, 6:34 AM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
44

Other than this issue, this patch looks good to me. @PkmX: can you add test cases for these potential issues?

PkmX updated this revision to Diff 177657.Mon, Dec 10, 9:27 PM

Rebased onto trunk.

  • Error out if %pcrel_lo(sym) does not point to a %pcrel_hi.
  • Added a corresponding test case pcrel-lo12-invalid.s.
PkmX updated this revision to Diff 177658.EditedMon, Dec 10, 9:38 PM
PkmX marked an inline comment as done.

Specify that pcrel-lo12-invalid.s will only produce an error with relaxation disabled, otherwise the assembler still emits a relocation which will fail at link time.

asb accepted this revision.Thu, Dec 13, 3:38 AM

This looks good to me, but there's one addition test I'd like to see. You explain that the intended behaviour is that there will be no compile-time error if linker relaxation is enabled. It would be good to have this reflected in a test. Maybe adding extra RUN lines to pcrel-lo12-invalid.s and adding a comment that explains that is expected with linker relaxation enabled.

This revision is now accepted and ready to land.Thu, Dec 13, 3:38 AM
In D54029#1329560, @asb wrote:

This looks good to me, but there's one addition test I'd like to see. You explain that the intended behaviour is that there will be no compile-time error if linker relaxation is enabled. It would be good to have this reflected in a test. Maybe adding extra RUN lines to pcrel-lo12-invalid.s and adding a comment that explains that is expected with linker relaxation enabled.

I personally would regard that as an implementation detail. There's no reason we couldn't give an error; we bail out of shouldForceRelocation early without bothering to check the relocation if relaxation is enabled, but there's nothing stopping us from putting the %pcrel_lo error check before that other than being slightly slower and increasing code complexity.

asb added a comment.Thu, Dec 13, 8:36 AM

I personally would regard that as an implementation detail. There's no reason we couldn't give an error; we bail out of shouldForceRelocation early without bothering to check the relocation if relaxation is enabled, but there's nothing stopping us from putting the %pcrel_lo error check before that other than being slightly slower and increasing code complexity.

It's an implementation detail, but it's useful for the tests to capture the current behaviour so it becomes obvious if it changes unexpectedly. In terms of testing, it's also a case where there's an obvious possibility for the assembler to do something wrong (mis-assemble or crash). From that perspective, it's worth covering the case. Though I agree, the comment could be clear that this is the current behaviour rather than the only valid behaviour.

PkmX added a comment.Thu, Dec 13, 8:48 AM

@jrtc27 Actually I agree it should produce an error regardless of whether relaxation is enabled, since it is most likely an user error to write a %pcrel_lo that doesn't point to an auipc, and the earlier we can catch it the better.

@jrtc27 Actually I agree it should produce an error regardless of whether relaxation is enabled, since it is most likely an user error to write a %pcrel_lo that doesn't point to an auipc, and the earlier we can catch it the better.

If you're editing this revision, could you please consider incorporating the getPCRelHiExpr -> getPCRelHiFixup changes (or equivalent) in D55279 which are necessary for supporting things other than %pcrel_hi? It seems slightly wrong to be landing a revision knowing that it will be refactored later.

PkmX updated this revision to Diff 178183.Thu, Dec 13, 11:09 PM
  • %pcrel_lo that doesn't point to a valid auipc will always produce an error, even if relaxation is enabled. The test pcrel-lo12-invalid.s is modified accordingly.
  • getPCRelHiExpr is renamed to getPCRelHiFixup and returns a MCFixup instead, to allow expansion for non %pcrel_hi modifiers in later patches.

Thanks, looks good to me now.