Skip to content

The JIT doesn't call for combine_symbol_mask on the initial trampoline for a trace #125911

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

Closed
brandtbucher opened this issue Oct 24, 2024 · 5 comments
Assignees
Labels
3.14 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-JIT type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@brandtbucher
Copy link
Member

brandtbucher commented Oct 24, 2024

Crash report

_PyJIT_Compile doesn't consider the initial (big) trampoline "group" when collecting the (little) trampolines needed to compile a given trace.

This doesn't break anything right now, since we don't have the initial "big" trampoline on AArch64, and only emit the "little" trampolines on AArch64. But with the immenent move to LLVM 19, this is breaking stuff.

Also, our use of the term "trampoline" here is overloaded. Let's call the big trampoline a "shim" from here on out. We can rename trampoline.c to shim.c and update all of the references to the old name to make it more clear. That should wait until after GH-125499 though, to reduce the amount of conflicts.

CC @diegorusso and @savannahostrowski. Either one of you want to take this?

Linked PRs

@brandtbucher brandtbucher added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump 3.14 bugs and security fixes topic-JIT labels Oct 24, 2024
@Zheaoli
Copy link
Contributor

Zheaoli commented Oct 24, 2024

I'm interested on this issue. May I take a try if there is no one is working on this?

@diegorusso
Copy link
Contributor

@Zheaoli try having a go, otherwise I can do it next week.

@diegorusso
Copy link
Contributor

if you need any help, please reach out!

@brandtbucher
Copy link
Member Author

Thanks @Zheaoli. The fix should be pretty simple (just one line to add the missing call). I don't think this needs docs or tests, since there's no way to actually hit the bug right now.

Do note that this is a blocker for our LLVM upgrade, so we'd appreciate it if you could open the PR sometime soon, like today or tomorrow. Otherwise we might just go ahead and fix it as part of the LLVM 19 PR.

@savannahostrowski
Copy link
Member

Also, our use of the term "trampoline" here is overloaded. Let's call the big trampoline a "shim" from here on out. We can rename trampoline.c to shim.c and update all of the references to the old name to make it more clear. That should wait until after #125499 though, to reduce the amount of conflicts.

I can take this piece on after #125499 is merged.

Zheaoli added a commit to Zheaoli/cpython that referenced this issue Oct 25, 2024
Zheaoli added a commit to Zheaoli/cpython that referenced this issue Oct 25, 2024
ebonnal pushed a commit to ebonnal/cpython that referenced this issue Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.14 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-JIT type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Development

No branches or pull requests

4 participants