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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp 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;
----------------
sfertile wrote:
> It looks like these 2 cases and `R_PPC64_GOT_PCREL34` et al share the same implementation, we should combine them.
Initially I wanted to do this after all of the LLD patches were in. However, you are now the second person to ask for this so I'm going do it now.
I'm going to do this in an NFC patch and then rebase this patch on top.


================
Comment at: lld/ELF/Relocations.cpp:1365
+                    getLocation(sec, sym, offset));
+        return;
+      }
----------------
NeHuang wrote:
> Will the `return` be executed after the `errorOrWarn`?
Yes it could be executed. The function `errorOrWarn` will sometimes exit and it will sometimes return back here.


================
Comment at: lld/ELF/Relocations.cpp:1371
+                    getLocation(sec, sym, offset));
+        return;
+      }
----------------
NeHuang wrote:
> ditto
Same as above. We may execute the return.


================
Comment at: lld/ELF/Relocations.cpp:1350
+
+    if (type == R_PPC64_TLSGD && expr == R_TLSDESC_CALL) {
+      if (i == end) {
----------------
sfertile wrote:
> 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.
In that case I'll keep the check on `i` but I'll remove the offset check.


================
Comment at: lld/test/ELF/ppc64-tls-pcrel-gd.s:67
+
+# GDTOLE-RELOC: There are no relocations in this file.
+
----------------
NeHuang wrote:
> Are we missing the  relocation `R_PPC64_TPREL34` here for GDTOLE?
That relocation should not be in the final linked object file. The linker can compute the value of it and just modify the immediate in the instruction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87318