Page MenuHomePhabricator

[RISCV] Leave pcrel_hi/pcrel_lo fixup pairs unresolved

Authored by lewis-revill on May 14 2019, 8:40 AM.



This patch modifies shouldForceRelocation to always return true for pcrel_hi and pcrel_lo fixups. This avoids having to use various levels of indirection in order to correctly determine whether a given one of these fixups can be safely resolved, since there are various situations where determining whether a given fixup can be safely resolved relies on knowing whether its corresponding fixup could be safely resolved.

Diff Detail


Event Timeline

lewis-revill created this revision.May 14 2019, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2019, 8:40 AM
asb added inline comments.May 23 2019, 6:04 AM

Shouldn't be this be .Ltmp1 0x0 just like in the RELAX-RELOC case?

asb added inline comments.Jul 5 2019, 1:31 AM

Hi Lewis, did you have any thoughts on this?

@asb I prefer D60657 over this as it seems to cover everything, and this is a rather big hammer to be hitting the problem with (that GNU as manages to avoid needing). Have you had a look at that one?

jrtc27 added inline comments.Jul 5 2019, 10:06 AM

Yes, it should be. It's because RISCVMCExpr::evaluateAsRelocatableImpl evaluates it fully to the indirected symbol since RAB.willForceRelocations is false, unlike the RELAX-RELOC case.

lewis-revill marked an inline comment as done.Jul 12 2019, 2:26 AM
lewis-revill added inline comments.

So, if I understand correctly, evaluatePCRelLo will do it's work regardless of whether the relocation is actually evaluated, it just uses this 'best guess' function RAB.willForceRelocations and some of its own logic. Is this not a bug regardless of this patch? Are we relying on the logic in shouldForceRelocations and evaluatePCRelLo producing the exact same answer?

Are there uses of evaluatePCRelLo that aren't part of this assembler pipeline? Because if not then doesn't that imply that for this patch the entire evaluatePCRelLo functionality would be completely removed?

Lewis, I believe this should now be marked abandoned in light of the other fixes?

lewis-revill abandoned this revision.Mar 18 2020, 1:21 PM