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
Differential D54029
[RISCV] Properly evaluate fixup_riscv_pcrel_lo12 PkmX on Nov 2 2018, 1:16 AM. Authored by
Details This is a update to D43157 to correctly handle fixup_riscv_pcrel_lo12. Notable changes:
Diff Detail Event TimelineComment Actions Rebase onto trunk as the declaration of RISCVAsmBackend is split into its own header file. @asb Any chance to look at this patch?
Comment Actions Rebased onto trunk.
Comment Actions 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. Comment Actions 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. Comment Actions 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. Comment Actions 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. Comment Actions @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. Comment Actions 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. Comment Actions
Comment Actions Hi James, I encountered another case not handled yet in this patch. It produces the error: could not find corresponding %pcrel_hi .option push Comment Actions 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. Comment Actions Sure enough, threading through an MCSubtargetInfo to EmitLabel and checking for CanReuseDataFragment before inserting the label into the current section fixes the issue. |
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.