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

Mon Jun 15 14:54:27 PDT 2020

stefanp added inline comments.

================
Comment at: lld/ELF/Arch/PPC64.cpp:382
+// pieces separately and assemble/disassemble them accordingly.
+static void writePrefixedInstruction(uint8_t *loc, uint64_t insn) {
+  insn = config->isLE ? insn << 32 | insn >> 32 : insn;
----------------
sfertile wrote:
> nemanjai wrote:
> > stefanp wrote:
> > > sfertile wrote:
> > > > Can I ask the rational on why we want to treat a prefixed instruction as a single 8-byte instruction as opposed to a 4-byte prefix and a 4-byte instruction?  I'm not necessarily disagreeing we should treat them as a single entity, I just want to make sure we have though about what the best representation is considering all the new relocations to be added.
> > > Good question. I like to treat them as one because conceptually they are one instruction. So, in my mind I tend to think of them as 8 byte instructions. 
> > > Some reasons I tend to think of them this way:
> > > 1) We can have the instruction part without the prefix but we can never have a prefix without a following instruction to go with it.
> > > 2) Immediates are represented across the boundary of the prefix/instruction. For example `paddi` has 18 bits of immediate in the prefix and 16 bits of the same immediate in the instruction. Those two are concatenated together to form the actual immediate. 
> > > 3) Flags in the prefix change the meaning of the subsequent instruction. Think of the PC Relative flag.
> > > 4) In llc we treat them as one instruction (I know this is a bit circular...)
> > > For example in `PPCInstrPrefix.td`we have the base class for all prefixed instructions:
> > > ```
> > > // Top-level class for prefixed instructions.
> > > class PI<bits<6> pref, bits<6> opcode, dag OOL, dag IOL, string asmstr,
> > >          InstrItinClass itin> : Instruction {
> > >   field bits<64> Inst;
> > >   field bits<64> SoftFail = 0;
> > >   bit PCRel = 0; // Default value, set by isPCRel.
> > >   let Size = 8;
> > > ```
> > > 
> > > In the end the reason is purely conceptual from my perspective. I don't have a technical reason to treat them as one.
> > Not that it makes a significant difference, but also one store vs. two.
> I can see why we think of them as a single instruction in the backend. The reason I asked though is that in the object file we have a 4-byte prefix and a 4-byte instruction not an 8-byte instruction (as you can see from how we have to shuffle in the read/writes),  and in this case the bits being relocated are broken into bits in the prefix and bits in the instruction. (with the 2 fields being discontinuous) The prefix28 relocations was similar (high 12 in the prefix, lo 16 in the instruction and again the 2 fields are discontinuous). I haven't looked at all the new relocations though, which format does the paddi use?
> 
paddi uses the same idea where it splits the 34 bit relocation bits into 18 bits in the prefix and 16 bits in the instruction.  The two fields are discontinuous as you say. So, what you are saying is that you prefer to have this in a struct or class with two parts because we are going to be working with them separately anyway?

================
Comment at: lld/ELF/Arch/PPC64.cpp:1011
+    uint64_t si1 = (val & si1Mask);
+    uint64_t instr = readPrefixedInstruction(loc) & ~fullMask;
+
----------------
nemanjai wrote:
> sfertile wrote:
> > MaskRay wrote:
> > > 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.
> > >  FWIW, AArch64.cpp marks out the bits as well.
> > 
> > AARCH64 is inconsistent: It masks to zero out the bits in `write32AArch64Addr`, but doesn't in `or32le`. 
> > 
> > ```
> >  static void or32le(uint8_t *p, int32_t v) { write32le(p, read32le(p) | v); }
> > ```
> > 
> > With respect to D80496 --> I'm not sure how this is relevant. IIUC the option is to use REL instead of RELA for the dynamic relocations. ie LLDs output, the masking we are talking about here is in relation to the linkers input.
> > 
> > 
> > > 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.
> > 
> > I had looked at what LLVM outputs, what gas produces for a bunch of inputs (since I can't look at gas source) and the fact that the AARCH64 target doesn't mask out the bits to be relocated (in `or32le`) and though the best way to make this consistent was to omit the masking. 
> > 
> > I think we should be consistent both for all input relocations but also across targets. The reason I think its important to omit the masking (if its valid to omit) is precisely for the reason you pick option 2. The// wrong// input (ie with the addend encoded into the instruction) produced the right output which made you think the addend had to be there.  The linker being 'nice' lead to us not catching the improper LLVM codegen. If the linker had been stricter we would have realized the problem earlier.
> > 
> > A lot of these minor details aren't well documented and the tools implementations essentially becomes the documentation. If it is supposed to work with garbage in the bits then I am all for consistently masking anywhere we need a wider read then the bits to be relocated: but I'd like to see something concrete that suggest it is intended to work that way.
> I too am in favour of masking out the bits. This is what `ld.bfd` does. It doesn't hurt performance. It is conservatively correct with minimal overhead.
By "hide the problem" I'm basically saying that there could be a compiler that is setting garbage bits into where the relocation is supposed to go. If that's the case we won't know about it because the linker is nice enough to fix it for us. It's a bug that is not detected.

I have not been able to find a place that specified whether or not the relocation bits need to be zero or not when we are using RELA.
What do people think of the following compromise:
A warning or a debug message. We do compile everything but if we find that `readPrefixedInstruction(loc) & ~fullMask != readPrefixedInstruction(loc)` we emit a warning (or debug) saying that we have found garbage bits. We can still link everything that LD compiles but we notify potential problems.

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

https://reviews.llvm.org/D81457