-
Notifications
You must be signed in to change notification settings - Fork 345
[next] Various JIT failure workarounds #11024
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
Conversation
LLVM currently emits dubious symbol sizes for aliases. E.g. assembling the following with LLVM top-of-tree... ``` $ cat foo.s <snip> .data .globl base base: .dword 42 .size base, 8 .set alias, base+4 ``` results in both base and alias having symbol size 8, even alias starts at base + 4. This also means that alias extends past the end of the .data section in this example. We should probably teach LLVM not to do this in the future, but as a short-term fix this patch teaches JITLink to simply truncate symbols that would extend past the end of their containing block. rdar://114207607 (cherry picked from commit 6267697) Conflicts: llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h (cherry picked from commit 2f53a4c)
This change fixed the size of the symbol but the check that emited the error was still using the old size. Updated to use the correct size, which will make the code in the if unreachable, but it's probably better to leave it to avoid merge conflicts. rdar://133510063 (cherry picked from commit d629b38)
Swift is currently generating multiple .rodata sections with a combination of SHF_ALLOC and (SHF_ALLOC | SHF_WRITABLE) flags and this was tripping an assert in the ELFLinkGraphBuilder. As a temporary workaround this patch just uses the union of the requested permissions. rdar://114207428 (cherry picked from commit ead32e1) Conflicts: llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h (cherry picked from commit a655fbe)
We've cherry-picked these the last 3 (?) rebranches now (and have to investigate each time). Would love if they get fixed upstream, but for now I think merging to next is the best thing to do. |
@swift-ci please test llvm |
I remember we discussed cherry-picking these to next last year when this came up, but I thought we already agreed to do this. Did we just get distracted, or was there some blocker I've forgotten? I continue to agree we should cherry-pick them to next since we don't have any other immediate fixes. The changes themselves LGTM, but I haven't worked on the JIT in a while so I would like to @lhames to give his stamp of approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@lhames Could you confirm please? |
This failure isn’t turning up in #11027, which also has these exact same cherry-picks, so I’m going to assume we’re good. |
Cherry-picks from
stable/20240723
:rdar://133510063
The ETA for an upstream resolution to these issues is uncertain, so avoid having to keep carrying these around across stable branches.