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

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked 2 inline comments as done.
sfertile added inline comments.


================
Comment at: ELF/SyntheticSections.cpp:3060
+void LongBranchTargetSection::addEntry(Symbol &Sym) {
+  assert(Sym.LtIndex == -1U);
+  Sym.LtIndex = Entries.size();
----------------
MaskRay wrote:
> sfertile wrote:
> > MaskRay wrote:
> > > LtIndex is `uint32_t` but -1U is undefined. Though they are usually the same type, `UINT32_C(-1)` or `(uint32_t)-1` may be better
> > I initially changed this, but looking at it again I think -its interpreted as `-(1U)` which I believe is well defined. I'll see if I can verify that.
> Yes, it is interpreted as -(1U) as there is no negative integer literal. It applies unary minus operator to 1U.
> 
> `(uint32_t)-1` LG.
Ok, in my understanding the comparison to -1U has well defined behavior then. If thats the case I should stick with that for consistency (we do the same check in a number of other places as well). If it is indeed undefined or implementation defined I can update all the uses in this patch and follow up with an NFC patch to convert the other comparisons to what ever we settle on. 


================
Comment at: ELF/Thunks.cpp:618
+  uint16_t OffHa = (Offset + 0x8000) >> 16;
+  uint16_t OffLo = Offset;
 
----------------
MaskRay wrote:
> ruiu wrote:
> > 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...
> I pointed out this because I saw some `& 0xffff` used otherwhere... and not sure if warnings would make some MSVC build bots unhappy..
> 
> `/W3` does not warn implicit `int -> char` but `/W4` does. I think it may be acceptable for a warning which is only enabled on `/W4`... Need some Windows expert to confirm this.
> 
> `long long -> unsigned` `unsigned long long -> unsigned` is warned in `/W3`.
> `http://www.brianlheim.com/2018/04/09/cmake-cheat-sheet.html` says `/W3` is the default 
So if we enable warnings with `-DLLVM_ENABLE_WARNINGS=on` by default that is going to be `/W3`? If that's the case I'll leave `&0xffff` off unless the windows bot complains. (Sorry I would test this out myself but I don't have a windows machine to check it on).


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D53408