Page MenuHomePhabricator

[RISCV] Don't incorrectly force relocation for %pcrel_lo
AbandonedPublic

Authored by lewis-revill on Jan 25 2019, 8:10 AM.

Details

Reviewers
asb
PkmX
jrtc27
Summary

This patch updatess the logic introduced by D54029, which attempted to check if a %pcrel_lo relocation should be forced based on whether the previous %pcrel_hi fixup would have a relocation. The check forced the relocation if the fragment containing the symbol associated with the %pcrel_hi and the fragment containing the symbol referenced by the %pcrel_hi were different.

However there are some occassions where the symbol referenced by %pcrel_hi is in a different fragment to the %pcrel_hi itself but the assembler determines it can be resolved. The %pcrel_lo would be incorrectly emitted in this case, resulting in an invalid object file that would not link (dangerous relocation: %pcrel_lo missing matching %pcrel_hi).

This patch explicitly stores %pcrel_hi fixups that were previously resolved and uses this to check whether a relocation should be forced for a %pcrel_lo fixup. A %pcrel_lo relocation should never be forced if the corresponding %pcrel_hi fixup was resolved without a relocation.

Diff Detail

Repository
rL LLVM

Event Timeline

lewis-revill created this revision.Jan 25 2019, 8:10 AM
lewis-revill edited the summary of this revision. (Show Details)

Remove irrelevant dependencies.

Hi @lewis-revill, thanks for the patch.

In https://reviews.llvm.org/rL349764#305030 I noted a similar issue but I forgot to follow it up later. Your change fixes it, so feel free to use it as a regression test if you want.

Kind regards,

jrtc27 added a comment.Feb 5 2019, 8:25 AM

Could we not have a situation where the pcrel_hi20 is seen *after* the lo12, in which case the lo12 would be forced but the hi20 could still be evaluated? E.g. I think the following would be problematic:

foo:
    j 1f
0:  addi a0, a0, %pcrel_lo(1f)
    ret
1: auipc a0, %pcrel_hi(bar)
    j 0b

.align 2
bar:
    # ...

Obviously extremely pathological code, but nonetheless valid.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 8:25 AM

Could we not have a situation where the pcrel_hi20 is seen *after* the lo12, in which case the lo12 would be forced but the hi20 could still be evaluated? E.g. I think the following would be problematic:

foo:
    j 1f
0:  addi a0, a0, %pcrel_lo(1f)
    ret
1: auipc a0, %pcrel_hi(bar)
    j 0b

.align 2
bar:
    # ...

Obviously extremely pathological code, but nonetheless valid.

Good call. It's a pathological case but maybe there are other cases as well. It's not a nice situation. Somehow we have to ensure there can never be a pcrel_lo12 relocation without a corresponding pcrel_hi20 relocation. It might help to know whether the pcrel_hi20 has been seen at all, regardless of whether it was resolved.

So I guess the intended behaviour might be:

pcrel_hi20 seen and not resolved (IE relocation emitted) -> force relocation
pcrel_hi20 seen and resolved -> don't force relocation
pcrel_hi20 not seen -> don't force relocation? If evaluatePcRelLo thinks it can be evaluated does it matter if the pcrel_hi20 can or not?

I'll have another think and take a look at your testcase.

Could we not have a situation where the pcrel_hi20 is seen *after* the lo12, in which case the lo12 would be forced but the hi20 could still be evaluated? E.g. I think the following would be problematic:

foo:
    j 1f
0:  addi a0, a0, %pcrel_lo(1f)
    ret
1: auipc a0, %pcrel_hi(bar)
    j 0b

.align 2
bar:
    # ...

Obviously extremely pathological code, but nonetheless valid.

Good call. It's a pathological case but maybe there are other cases as well. It's not a nice situation. Somehow we have to ensure there can never be a pcrel_lo12 relocation without a corresponding pcrel_hi20 relocation. It might help to know whether the pcrel_hi20 has been seen at all, regardless of whether it was resolved.

So I guess the intended behaviour might be:

pcrel_hi20 seen and not resolved (IE relocation emitted) -> force relocation
pcrel_hi20 seen and resolved -> don't force relocation
pcrel_hi20 not seen -> don't force relocation? If evaluatePcRelLo thinks it can be evaluated does it matter if the pcrel_hi20 can or not?

I'll have another think and take a look at your testcase.

My suspicion is that it would be easier to teach MCAssembler::evaluateFixup how to deal with an extra level of indirection, since the problem stems from it believing the relocation can be evaluated and not knowing about the *real* symbol/relocation we care about.

My suspicion is that it would be easier to teach MCAssembler::evaluateFixup how to deal with an extra level of indirection, since the problem stems from it believing the relocation can be evaluated and not knowing about the *real* symbol/relocation we care about.

I've been thinking about this and I'm wondering whether it makes sense for shouldForceRelocation to be able to somehow indicate 'I don't know yet'. The assembler should then keep those results instead of recording the relocation, and allow the target to make a final decision later. Or alternatively allow the target to continue to remove things from this 'don't know' list and just force relocations for all the remaining ones at the end. Either way there needs to be some separation of the pass for evaluating fixups and recording the relocations / applying the fixups.

lewis-revill abandoned this revision.Jun 19 2019, 3:34 AM

Abandoned in favour of D61907