cpython icon indicating copy to clipboard operation
cpython copied to clipboard

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

Open brandtbucher opened this issue 1 year ago • 3 comments

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?

brandtbucher avatar Oct 24 '24 04:10 brandtbucher

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

Zheaoli avatar Oct 24 '24 06:10 Zheaoli

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

diegorusso avatar Oct 24 '24 10:10 diegorusso

if you need any help, please reach out!

diegorusso avatar Oct 24 '24 10:10 diegorusso

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.

brandtbucher avatar Oct 24 '24 15:10 brandtbucher

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 https://github.com/python/cpython/pull/125499 though, to reduce the amount of conflicts.

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

savannahostrowski avatar Oct 24 '24 17:10 savannahostrowski