[PATCH] D15779: [ELF] - Implemented optimization for R_X86_64_GOTPCREL relocation.

[George Rimar via llvm-commits]

grimar added a comment.

In http://reviews.llvm.org/D15779#437390, @rafael wrote:

> I decided to apply the patch to give it a better review.
>
> Some of the issues I pointed out in the review (R_RELAX_GOT referring
>  to got entry) were the causes of other changes (unnecessary continue).


Thanks !

> Also, the *GOTPCRELX relocations can always be relaxed if the symbol

>  is not ifunc or preemptable, no? It is just that it is not always a

>  mov->lea.


Sure, but this patch supports only mov->lea conversion. So for our case
I decided to check that instruction is exactly mov, because of next:

> In any case, I have attached the result of simplifying your patch a

>  bit. Let me know what you think.


I think it looks better than mine, but it have an issue:
Imagine you have next code to relax:

  .text
  .globl foo
  .type foo, @function
  foo:
   nop
  
  .text
  .globl _start
   call	*foo at GOTPCREL(%rip)

Since your patch does not check the mov opcode before relaxing, it "relax" that to lea instead
of "nop call foo" or "call foo nop". That is because relaxGot() does not yet know about how to relax the other
instructions yet.

> Cheers,

> Rafael

> 

> - F1976708: t.diff <http://reviews.llvm.org/F1976708>


If you dont mind, I`ll use your code as a base, will fix the issue above, add the testcase for it and update.
(I also plan to add other relaxations, but I wonder will it be better to do that in this patch or separatelly,
I`ll take a look how to do that cleaner, probably implementing all of them at once can help to simplify
the patch, probably not).


================
Comment at: ELF/Target.cpp:762
@@ +761,3 @@
+void X86_64TargetInfo::relaxGot(uint8_t *Loc, uint64_t Val) const {
+  if (Val + 0x80000000 > 0xFFFFFFFF)
+    fatal("Relaxation overflow");
----------------
rafael wrote:
> It is not clear why you need this. relocateOne has a value check. Why you need one here?
It would write something like "relocation R_X86_64_PC32 is out of range". That does not imo provide enough
info here about what really happened. 
Ideally we just should not do that relaxation if we know that overflow happens.


http://reviews.llvm.org/D15779