[PATCH] D87318: [LLD][PowerPC] Add support for R_PPC64_GOT_TLSGD_PCREL34 used in TLS General Dynamic

Thu Sep 24 10:07:26 PDT 2020

sfertile added inline comments.

================
Comment at: lld/ELF/Arch/PPC64.cpp:1277
+  case R_PPC64_PCREL34:
+  case R_PPC64_GOT_TLSGD_PCREL34: {
     const uint64_t si0Mask = 0x00000003ffff0000;
----------------
It looks like these 2 cases and `R_PPC64_GOT_PCREL34` et al share the same implementation, we should combine them.

================
Comment at: lld/ELF/Relocations.cpp:1350
+
+    if (type == R_PPC64_TLSGD && expr == R_TLSDESC_CALL) {
+      if (i == end) {
----------------
stefanp wrote:
> MaskRay wrote:
> > I am not sure we want to add error checking for every relocation usage. We should add them considering the possibility of such errors (can a compiler/tool/assembly generate such erroneous relocation usage?) 
> I do not believe that the compiler can generate either of these situations from C source code. 
> 
> I can generate the bad alignment in assembly (by adding `.space 1` for example) but it is not likely that someone will do that.
> 
> In general there are a couple of reasons why I add code like this.
> First, it is to prevent future development work from creating hidden bugs. Maybe a little like 
> https://llvm.org/docs/CodingStandards.html#assert-liberally 
> from a philosophical perspective. I know that these are not asserts but I think the argument remains valid...
> Second, it is my way of documenting assumptions in the code.
> 
> I understand your point of view. You don't want want half the code to be error checking so if you really want me to remove these two errors I can.  I just wanted to give you my perspective on this. Let me know what you think!
We dereference 'i' so we need the check that i is still valid. The validation of the offset is less critical and could be omitted or converted to an assert.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87318