This is an archive of the discontinued LLVM Phabricator instance.

[X86] Emit RIP-relative access to local function in PIC medium code model
ClosedPublic

Authored by tkoeppe on Dec 22 2022, 5:29 PM.

Details

Summary

Currently, the medium code model for x86_64 emits position-dependent relocations (R_X86_64_64) for local functions, regardless of PIC or no-PIC mode. (This means generically that code compiled with the medium model cannot be linked into a position-independent executable.)

Example:

static int g(int n) {
  return 2 * n + 3;
}

void f(int(**p)(int)) {
  *p = g;
}

This results in:

Disassembly of section .text:

0000000000000000 <f>:
       0: 48 b8 00 00 00 00 00 00 00 00	movabs	rax, 0x0
       a: 48 89 07                     	mov	qword ptr [rdi], rax
       d: c3                           	ret
Relocation section '.rela.text' at offset 0xf0 contains 1 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000002  0000000200000001 R_X86_64_64            0000000000000000 .text + 10

This patch changes the behaviour to unconditionally emit a RIP-relative access, both in PIC and non-PIC mode. This fixes PIC mode, and is perhaps an improvement in non-PIC mode, too, since it results in a shorter instruction. A 32-bit relocation should suffice since the medium memory model demands that all code fit within 2GiB.

Diff Detail

Event Timeline

tkoeppe created this revision.Dec 22 2022, 5:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2022, 5:29 PM
tkoeppe requested review of this revision.Dec 22 2022, 5:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2022, 5:29 PM
MaskRay added a subscriber: MaskRay.EditedDec 22 2022, 5:48 PM

It's common to tag such changes as [X86] (git log llvm/lib/Target/X86 to get a taste)

I may a title like [X86] Emit RIP-relative access to local function in PIC medium code model. I'd avoid a godbolt link and paste the code directly into the summary.

-U99999 from https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

tkoeppe updated this revision to Diff 485007.Dec 22 2022, 5:59 PM
tkoeppe retitled this revision from In the x86_64 medium code model, emit RIP-relative access to local function instead of position-dependent relocation to [X86] Emit RIP-relative access to local function in PIC medium code model.
tkoeppe edited the summary of this revision. (Show Details)

Updated diff to use -U9999.

This could use a test.

This could use a test.

Yes, on it! I'll also need to update the existing tests.

But could anyone confirm in principle that the lack of PIC support of the medium model currently is indeed a bug, and that this solution is appropriate?

tkoeppe updated this revision to Diff 485094.Dec 23 2022, 5:01 AM

Added a comment in the code explaining the choice for "medium code model, function", and updated the tests.

MaskRay accepted this revision.EditedDec 24 2022, 3:09 PM

LGTM. I was also playing with how to hack small code model to behave like medium code model and found this piece of WrapperRIP code.

% curl -L 'https://reviews.llvm.org/D140593?download=1'
Index: llvm/lib/Target/X86/X86ISelLowering.cpp
===================================================================
--- llvm/lib/Target/X86/X86ISelLowering.cpp
+++ llvm/lib/Target/X86/X86ISelLowering.cpp
...

This uses patch -p0. It you use arc or git format-patch -1 --stdout, the patch file will require patch -p1, which is more common.

llvm/lib/Target/X86/X86ISelLowering.cpp
20500

This is the common path. So moving the medium code model code below this if will remove a redundant test for the common path.

This revision is now accepted and ready to land.Dec 24 2022, 3:09 PM
tkoeppe updated this revision to Diff 485212.Dec 24 2022, 4:53 PM
tkoeppe marked an inline comment as done.

Moved new condition further down, so it appears after the more common checks. Also changed the diff header to work with -p1, not -p0.

MaskRay added inline comments.Dec 24 2022, 5:16 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
20492

Delete this blank line.

tkoeppe added inline comments.Dec 24 2022, 6:48 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
20492

Are you sure? I thought since M is used in multiple conditions below now, it'd be better to not "stick" to any one of them, so it's easier to spot, and so it doesn't look like it just appertains to one of the if statements. I can change it if you like, but I thought it looks clearer with this blank line.

MaskRay added inline comments.Dec 24 2022, 9:55 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
20492

I'm sure. The prevailing style is compact, and the argument that it's used twice isn't really a strong one.

tkoeppe updated this revision to Diff 485245.Dec 25 2022, 3:14 PM
tkoeppe marked an inline comment as done.
tkoeppe added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
20492

OK, sure, done!

This revision was landed with ongoing or failed builds.Dec 28 2022, 11:14 AM
This revision was automatically updated to reflect the committed changes.