[PATCH] D53408: [PPC64] Long branch thunks.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Symbols.h:214-216
+  // True if this symbol is in the Plt.
+  unsigned IsInPlt : 1;
+
----------------
nit: I'd add this before `IsInIplt` as it seems a more natural place to add this bit. (New code shouldn't look new but should be written as if it were there from the beginning.)


================
Comment at: ELF/Thunks.cpp:618
+  uint16_t OffHa = (Offset + 0x8000) >> 16;
+  uint16_t OffLo = Offset;
 
----------------
sfertile wrote:
> MaskRay wrote:
> > With `-DLLVM_ENABLE_WARNINGS=on` & MSVC, `/W4` is turned on which warn
> > `uint16_t OffLo = Offset;`
> > 
> > `warning C4244: 'initializing': conversion from 'int64_t' to 'uint16_t', possible loss of data`
> > 
> > 
> > Append `& 0xffff` to suppress it.
> Thanks, I don't ever compile with MSVC so I didn't realize it would warn on this. I believe I've added similar code in previous patches,  I'll find them and clean those up in a followup NFC patch.
Hmm, but that narrowing conversion is obvious and we do that everywhere (in particular from int to char because RHS is usually promoted to int due to the usual integer promotion). If MSVC warns on it, we need this, but I don't like that warning in the first place...


================
Comment at: ELF/Thunks.cpp:732-733
 static Thunk *addThunkPPC64(RelType Type, Symbol &S) {
-  if (Type == R_PPC64_REL24)
+  if (Type != R_PPC64_REL24)
+    fatal("unexpected relocation type");
+
----------------
Isn't this an assert? If this line is executed only when there is a bug in the code, this should be an assert.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D53408