Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Immediate] Use the large code model #64720

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

c0d1f1ed
Copy link

The default 'small' code model assumes that code and data sections are allocated less than 2 GiB apart so that RIP-relative displacements can be 32-bit. On UNIX this produces ELF object code with R_X86_64_PC32 relocations. However, the default SectionMemoryManager for LLJIT simply makes mmap() allocations which, depending on the platform's implementation, may end up far apart.

While LLVM provides an alternate JITLinkMemoryManager which aims to provide support for the small code model, it is still a work in progress: https://llvm.org/docs/JITLink.html#jitlink-availability-and-feature-status

This change instead ensures the large code model is used instead, which makes no assumptions about memory offset sizes and produces R_X86_64_64 relocations. Additional information about x64 code models can be found at: https://eli.thegreenplace.net/2012/01/03/understanding-the-x64-code-models

Fixes #60673

The default 'small' code model assumes that code and data sections are
allocated less than 2 GiB apart so that RIP-relative displacements can be
32-bit. On UNIX this produces ELF object code with R_X86_64_PC32
relocations. However, the default SectionMemoryManager for LLJIT simply
makes mmap() allocations which, depending on the platform's implementation,
may end up far apart.

While LLVM provides an alternate JITLinkMemoryManager which aims to provide
support for the small code model, it is still a work in progress:
https://llvm.org/docs/JITLink.html#jitlink-availability-and-feature-status

This change instead ensures the large code model is used instead, which
makes no assumptions about memory offset sizes and produces R_X86_64_64
relocations. Additional information about x64 code models can be found at:
https://eli.thegreenplace.net/2012/01/03/understanding-the-x64-code-models

Fixes swiftlang#60673
@c0d1f1ed
Copy link
Author

c0d1f1ed commented Apr 4, 2023

@jrose-apple @mikeash Please have a look, or suggest other reviewers.

@c0d1f1ed
Copy link
Author

@cachemeifyoucan @eeckstein please take a look.

@eeckstein
Copy link
Contributor

eeckstein commented Apr 13, 2023

@lhames does this look good?

@eeckstein eeckstein requested a review from lhames April 13, 2023 06:00
@lhames
Copy link
Contributor

lhames commented Apr 13, 2023

Switching to the Large code model isn't the right way to go here.

The JITLink document's status/roadmap is out of date: For single object file programs (and the minimal reproducer in #60673 looks like a single file case), small code model should just work. For multi-file programs where some symbols have hidden visibility a slab allocator must be used to guarantee that everything will be allocated in-range.

We should probably update Swift's immediate mode to use LLVM's slab allocator, but I'd also like to better understand what went wrong in #60673 in case it's indicative of other problems. I'll follow up on that issue.

@lhames
Copy link
Contributor

lhames commented Apr 14, 2023

Just checked the backtrace in #60673:

#31 0x0000000001874695 llvm::orc::RTDyldObjectLinkingLayer::emit(...) (/usr/bin/swift-frontend+0x1874695)

We're still using RuntimeDyld for x86-64 on Linux. We should switch that over to JITLink.

RuntimeDyld's section memory manager definitely doesn't handle small code model by default. JITLink's default InProcessJITLinkMemoryManager should handle it fine for single inputs, and we can switch to the slab allocator to handle multiple inputs.

@c0d1f1ed
Copy link
Author

Indeed ideally #60673 would be addressed by using JITLink, but last time I checked (LLVM 15) it was still quite incomplete.

I think the safest course of action is to merge this as-is, then attempt to make the JITLink changes. If something doesn't work properly for the latter and it needs to be reverted/disabled we'd at least fall back to a working state instead of a broken one.

I don't understand why the use of a slab allocator would be relevant to avoiding this bug when JIT-linking multiple pieces of object code. Could you elaborate on that and perhaps provide supporting arguments for your confidence that JITLink "should" work fine for all platforms?

@lhames
Copy link
Contributor

lhames commented Apr 14, 2023

Indeed ideally #60673 would be addressed by using JITLink, but last time I checked (LLVM 15) it was still quite incomplete.

How did you test it? Linux/x86-64 support for the PIC relocation model was already quite solid as of LLVM 15. There are a number of projects living on it already. The only reason I haven't made it the LLJIT default is that we haven't fleshed out all the static relocations (simply for lack of demand, since PIC was working well and is the default model).

I think the safest course of action is to merge this as-is, then attempt to make the JITLink changes. If something doesn't work properly for the latter and it needs to be reverted/disabled we'd at least fall back to a working state instead of a broken one.

This PR switches to Large Code model unconditionally, but that's not the known-good configuration on most platforms. I think it'd probably work, but it's not zero-risk in the way that you're implying. You could conditionalize it on Linux/x86-64 to address that, but I'm not sure it's worth it.

I don't understand why the use of a slab allocator would be relevant to avoiding this bug when JIT-linking multiple pieces of object code.

Consider a program constructed from the following two sources:

int __attribute__((visibility("hidden"))) x = 1;

and

extern int __attribute__((visibility("hidden"))) x;
int main(void) {
  return x;
}

Since the external reference to x has hidden visibility the compiler is allowed to reference it directly in small code model without going through a GOT. Since the definition is in a distinct graph that will be allocated separately we need to have reserved a slab up front to allocate both graphs into.

Could you elaborate on that and perhaps provide supporting arguments for your confidence that JITLink "should" work fine for all platforms?

As noted above, a number of projects are living on it already.

I should stress that "all platforms" in this context means "all platforms that I expect the Swift interpreter to be used on". JITLink is still missing some relocations for i386 and aarch32, and has no backends for ppc64, mips, or BPF yet. As far as I know those aren't currently relevant platforms for the swift interpreter.

Finally, I was planning a switch to JITLink for the Swift interpreter in the next couple of months anyway, since we want to start looking at adding lazy compilation support to it. This seems like a good motivation to flip the switch early. :)

@c0d1f1ed
Copy link
Author

Thanks for the clarifications! Fair point about the large code model not necessarily being the safer option everywhere. But for what it's worth, I have not encountered any issues across the various platforms I've tested on...

Let me back up a bit. I ran into the same assert as the one hit in #60673 while integrating a JIT compiler for https://github.com/NVIDIA/warp. It most reliably reproduced on a CentOS Docker container. While attempting to address it by using JITLink I got an error about "In graph C:\\Users\\Nicolas\\AppData\\Local\\NVIDIA Corporation\\warp\\Cache\\0.7.3\\gen\\wp___main__.cpp.obj, section .text: relocation target \"printf\" at address 0x7ffc6c89bacc is out of range of PCRel32 fixu..." on Windows, and if I recall correctly a similar looking issue on Linux. It might be worth noting that we're providing the addresses of CRT functions like printf through llvm::orc::DynamicLibrarySearchGenerator. I didn't investigate much further after finding the availibility and status section in the documentation. Using the large code model, which "makes no assumptions about addresses and sizes of sections", seemed like a much simpler fix with negligible performance impact. Security concerns also made us want to have a quick and uncomplicated solution.

So I wanted to contribute this back to Swift since #60673 was helpful in understanding the root cause of our bug. But I understand if you have a different set of tradeoffs. I'd be cautious about limiting the number of platforms deemed "relevant" though. It's not fun getting crash reports years later when knowledge of this issue has faded, and not being able to easily reproduce it.

Perhaps I should try upstreaming this to LLVM instead, who will care deeply about every platform/target. The combination of defaulting to the small code model and the random nature of SectionMemoryManager's allocations is undeniably very broken. While having JITLink available everywhere is a great plan-of-record for fixing it optimally, I'm not confident we'll get there soon, and it can't hurt to ensure having a safe fallback option.

@lhames
Copy link
Contributor

lhames commented Apr 18, 2023

JITLink became the LLJIT default JIT linker for ELF/x86-64 as of llvm/llvm-project@85c649b. I hope to switch it over for COFF soon too. I think these changes should be forwarded to Swift's next branch automatically. If they're needed in main we can cherry-pick them there too.

Let me back up a bit. I ran into the same assert as the one hit in #60673 while integrating a JIT compiler for https://github.com/NVIDIA/warp. It most reliably reproduced on a CentOS Docker container. While attempting to address it by using JITLink I got an error about "In graph C:\Users\Nicolas\AppData\Local\NVIDIA Corporation\warp\Cache\0.7.3\gen\wp___main__.cpp.obj, section .text: relocation target "printf" at address 0x7ffc6c89bacc is out of range of PCRel32 fixu..." on Windows, and if I recall correctly a similar looking issue on Linux.

Can you file bugs for those issues, including bitcode or object files to reproduce? Neither is necessarily a bug in the JIT: they could indicate incorrect visibility / linkage on symbols in the inputs. Neither bug has been reported by other groups that use the JIT, and programs that call printf are usually the first things anyone runs.

Using the large code model, which "makes no assumptions about addresses and sizes of sections", seemed like a much simpler fix with negligible performance impact. Security concerns also made us want to have a quick and uncomplicated solution.

I think that's fine to do out of tree but it's likely masking underlying bugs (either in LLVM the client code), so not a good in-tree solution.

If security is a concern to you then that's even more reason to move off RuntimeDyld: its error checking and propagation are poor. JITLink is in much better shape.

Perhaps I should try upstreaming this to LLVM instead, who will care deeply about every platform/target.

I am the LLVM JIT code owner. That review will also go to me. :)

RuntimeDyld has many problems besides code model support (e.g. those error checking issues), so we're trying very actively to move off it. Patches that go against the grain of that work aren't likely to be accepted.

A patch that selected the supported code and relocation models to use with RuntimeDyld would be very welcome, but even that's non-trivial: If you look at RuntimeDyld you'll find that each architecture implemented a hodgepodge of relocations. Large code model doesn't always work. PIC relocation model doesn't always work. Static relocation model doesn't always work. You'd need to audit and characterize the support for each architecture.

The combination of defaulting to the small code model and the random nature of SectionMemoryManager's allocations is undeniably very broken.

Agreed, but the solution is to ditch SectionMemoryManager rather than the small code model. :)

While having JITLink available everywhere is a great plan-of-record for fixing it optimally, I'm not confident we'll get there soon, and it can't hurt to ensure having a safe fallback option.

Again: this option isn't inherently safe. Plenty of people are living on JITLink already, so I'm inclined to fix this issue by switching sooner rather than later, mopping up any bugs that result, and moving into the glorious small-code-model future. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On QEMU, swift immediate mode crashes due to relative pointer relocation failure
3 participants