[PATCH] D81457: [LLD][PowerPC] Add support for R_PPC64_PCREL34

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/PPC64.cpp:1011
+    uint64_t si1 = (val & si1Mask);
+    uint64_t instr = readPrefixedInstruction(loc) & ~fullMask;
+
----------------
stefanp wrote:
> MaskRay wrote:
> > sfertile wrote:
> > > I've got a patch which removes the masking of the instruction bits on the other relocations types we were doing it for [[ https://reviews.llvm.org/D81106 | (D81106)]].  My understanding is that for REL ELF archs the addend is encoded into the bits to be relocated, and for RELA ELF archs the bits to be relocated are zeros, with the addend in the relocation. The extra masking doesn't hurt anything functionally but it can lead to misunderstandings on what the bits to be relocated should  [[ https://reviews.llvm.org/D81082 | contain]] so I think its important to be precise and omit the extra masking despite it not hurting anything. 
> > Even if powerpc is a RELA target, if it is easy to drop the requirement, I hope we can make some efforts making that work. See D80496 (-z rel)
> I do see what you mean. The way I look at this is that we have 3 options:
> 1) We just ignore it and assume that the zeros are there. This is what you are suggesting. However, my worry is that this way we could be silently producing bad code. Basically if there is a garbage bit in there we end up with an "or" of that bad bit and a valid offset and we sometimes end up with a bad offset.
> 2) We mask it out. This is what I did. It basically hides the problem. If there are bad bits they get zeroed out and we end up with a valid instruction. We will never know that the initial encoding is wrong.
> 3) We assert on those bits. This means that if there are non-zero bits in the encoding we don't even link the code. I don't want to do this last one mainly because GCC doesn't do it. I know, it is not necessarily a valid reason to do this.
> I had originally picked option 2 because that is what GCC did. It produces the correct output despite the garbage I put in there. Again, just because GCC does this it doesn't mean we should.
> 
> 
> With respect to the second comment:
> At this point I'm not going to try to change this to support REL yet. I'm just looking at RELA. If we want to add that support I would say doing it in a completely different patch.
I changed my mind on D81106.

> 2. We mask it out. This is what I did. It basically hides the problem. If there are bad bits they get zeroed out and we end up with a valid instruction. We will never know that the initial encoding is wrong.

This one looks good to me. Why does it "hide the problem"? powerpc{,64} are RELA targets. For a RELA target, the implicit addend should not matter. FWIW, AArch64.cpp marks out the bits as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81457/new/

https://reviews.llvm.org/D81457