[PATCH] D47507: [MC] [X86] Teach leaq _GLOBAL_OFFSET_TABLE(%rip), %r15 to use R_X86_64_GOTPC32 instead of R_X86_64_PC32

[Eric Christopher via Phabricator via llvm-commits]

echristo accepted this revision.
echristo added a comment.
This revision is now accepted and ready to land.

One inline comment then OK.



================
Comment at: lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:355-356
     ImmOffset -= 4;
+    if (StartsWithGlobalOffsetTable(Expr) != GOT_None)
+      FixupKind = MCFixupKind(X86::reloc_global_offset_table);
+  }
----------------
MaskRay wrote:
> echristo wrote:
> > MaskRay wrote:
> > > ruiu wrote:
> > > > Is this the right place to add this code? There's code in this function that also set the same value to FixupKind. Can't you merge them?
> > > When `%rip` is mentioned, r_addend should be relative to the PC of the **next** instruction thus the `ImmOffset -= 4` here (accounting for a 4-byte PC offset). This is different from the cases above (no PC is involved unless the magic _GLOBAL_OFFSET_TABLE_ appears, where r_addend should be relative to the PC of the **currenct** instruction ).
> > > 
> > > I think keeping them separate as is is not bad.
> > Perhaps not right above, but I think the code might be clearer if we just add the change in Fixup right before this block and add the condition to this block.
> > 
> > Thoughts?
> This block deals with 4-byte pc-relative `FixupKind`, while the 8-byte branch has been handled above.
> 
>  `_GLOBAL_OFFSET_TABLE_` can only be used in 4-byte or 8-byte `FixupKind`, the 1-byte and 2-byte branches below do not need this fix. Thus I think the current way organizes the logic well. Did I miss something?
Could you add a comment sorta like this:

  // If this is a rip relative load off of the global offset table symbol:
  // leaq _GLOBAL_OFFSET_TABLE(%rip), %r15
  // this needs to be a GOTPC32 relocation.

Around the if statement?

Otherwise I played around with this and agree, without hoisting a bunch of the if conditionals into a separate function.


Repository:
  rL LLVM

https://reviews.llvm.org/D47507