This is a update to D43157 to correctly handle fixup_riscv_pcrel_lo12.
- Rebased onto trunk
- Handle and test S-type
- Test case pcrel-hilo.s is merged into relocations.s
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.
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.
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.
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.