[PATCH] D114172: [ARM] implement support for ALU/LDR PC-relative group relocations

[Ard Biesheuvel via Phabricator via llvm-commits]

ardb marked an inline comment as done.
ardb added inline comments.


================
Comment at: lld/ELF/Arch/ARM.cpp:431
+    rem = val;
+    val &= (uint64_t)0xffffffU >> lz;
+  } while (group--);
----------------
MaskRay wrote:
> ardb wrote:
> > MaskRay wrote:
> > > MaskRay wrote:
> > > > MaskRay wrote:
> > > > > 
> > > > val is uint32_t. Remove `(uint64_t)`?
> > > Is `(uint64_t)0xffffff >> 32` possible? If lz can be 32, casting to `(uint64_t)` LGTM but it needs a comment. I am not familiar with the encoding and do not know whether lz can be 32.
> > Yes, lz will be 32 if val == 0. This may happen when using multiple groups if the value can be fully encoded by the previous group(s).
> > 
> > Note that in that case, doing &= should not have any effect, but a 32-bit shift of a 32-bit type is better avoided entirely, I'd assume?
> Unaddressed: needs a uint64_t cast to fix UB. `val &= (uint64_t)0xffffff >> lz;  // lz may be 32` 
> 
This is no longer true: lz == 32 only occurs if rem == 0x0, in which case we break out of the loop before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114172