This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Properly evaluate fixup_riscv_pcrel_lo12
ClosedPublic

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.Nov 21 2018, 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.Dec 6 2018, 5:49 AM
jrtc27 added inline comments.Dec 6 2018, 6:24 AM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
48

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.Dec 6 2018, 6:34 AM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
48

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.Dec 10 2018, 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.EditedDec 10 2018, 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.Dec 13 2018, 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.Dec 13 2018, 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.Dec 13 2018, 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.Dec 13 2018, 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.Dec 13 2018, 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.

This revision was automatically updated to reflect the committed changes.

Hi James, I encountered another case not handled yet in this patch. It produces the error: could not find corresponding %pcrel_hi

.option push
.option norelax
la gp, __global_pointer$
.option pop
la sp, _sp

Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2019, 1:01 PM

Hi James, I encountered another case not handled yet in this patch. It produces the error: could not find corresponding %pcrel_hi

.option push
.option norelax
la gp, __global_pointer$
.option pop
la sp, _sp

This also occurs when you manually expand la.

Regardless I believe the problem is caused because EmitLabel doesn't have any checks for whether a new data fragment should be introduced, whereas EmitInstToData does. So when the .Lpcrel_hiX label is expanded/parsed it's parent fragment is the one before the directive changed the STI flags, but the auipc and its %pcrel_hi fixup is located in the fragment after.

When getPCRelHiFixup() searches for the %pcrel_hi it fails to find it since it uses the fragment & offset of the label to search, whereas the fixup is in another fragment.

This is just from some initial debugging, so I'm not sure whether this is the only issue and/or what the best solution would be.

Hi James, I encountered another case not handled yet in this patch. It produces the error: could not find corresponding %pcrel_hi

.option push
.option norelax
la gp, __global_pointer$
.option pop
la sp, _sp

This also occurs when you manually expand la.

Regardless I believe the problem is caused because EmitLabel doesn't have any checks for whether a new data fragment should be introduced, whereas EmitInstToData does. So when the .Lpcrel_hiX label is expanded/parsed it's parent fragment is the one before the directive changed the STI flags, but the auipc and its %pcrel_hi fixup is located in the fragment after.

When getPCRelHiFixup() searches for the %pcrel_hi it fails to find it since it uses the fragment & offset of the label to search, whereas the fixup is in another fragment.

This is just from some initial debugging, so I'm not sure whether this is the only issue and/or what the best solution would be.

Sure enough, threading through an MCSubtargetInfo to EmitLabel and checking for CanReuseDataFragment before inserting the label into the current section fixes the issue.

Thanks for the analysis and suggestion Lewis.