Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fixing an extremely hard to track down y_inline crash...
The instuction that is crashing is this one in Callback_CallHandler_, which is the function that sets up the stack and jumps in to inline functions: ```pawn #emit SCTRL __cip // 37 ``` However, it was after the function was called, so the return. But looking at it, I realised that the exact crash address was in the middle of that instruction (which I couldn't have done without a local repro, since there's no way to map addresses to code without both). The return address is established by this code elsewhere: ```pawn @emit JUMP YSI_g_sCallbackCallAddress + (39 * cellbytes) ``` It's a fake RETN, but the effect is the same. The code is very tightly controlled, and I knew the exact number of instructions in that function, hence the 39 there, to land directly after the SCTRL (hence the 37 in that comment). Technically the 37 is the offset to the end of that instruction, and it is immediately followed by two `NOP`s because depending on the compiler options you may have an additional two instructions at the start of the function ('BREAK`s), so there's some wiggle room. I've recently been converting more of the assembly in YSI to use named constants instead of random numbers (I only recently learnt that that was possible, hence all the magic numbers before), so I added this in to the function: ```pawn const E_INLINE_CALL_diff = (_:E_INLINE_CALL_FLAGS - _:E_INLINE_CALL_PARAMS) * cellbytes; ``` This is a compile-time constant that just establishes one offset in to the inline data structure. What it does isn't important, and it is entirely executed at compile-time, so I didn't think it mattered. It did... It turns out that adding a `const` in to a function with debugging enabled still counts as a statement, and all statements are separated by `BREAK` opcodes for the VM. So despite the fact that this `const` generates no variables or runtime code, it still added a single additional `BREAK` in, and upset the offsets, leading to the return address being a single cell out, and jumping in to the middle of an instruction. Now the real question is - why didn't I crash? I don't know. Jumping to the middle of `SCTRL __cip` will result in instruction `6` being executed, which is `LREF.alt [address]`, and since the next instruction is `NOP` and gets interpreted as the parameter `134` we end up executing `LREF.alt 134`. I can only guess that in my test script the data at address `134` was sufficiently valid as to work in this context, and load something random successfully, but for other people it wasn't good. Then because I had a second `NOP` immediately after the code was correctly re-aligned and continued as normal. So the solution was to remove that extra generated `BREAK`, so the fix is changing this: ```c static Callback_CallHandler_(...) { const E_INLINE_CALL_diff = (_:E_INLINE_CALL_FLAGS - _:E_INLINE_CALL_PARAMS) * cellbytes; // This is called by `@`, and given the above enum in `INDIRECTION_DATA`. // Get the size, and update `INDIRECTION_DATA` to point to the data. #emit LOAD.alt INDIRECTION_DATA // 2 ``` To this: ```c const E_INLINE_CALL_diff = (_:E_INLINE_CALL_FLAGS - _:E_INLINE_CALL_PARAMS) * cellbytes; static Callback_CallHandler_(...) { // This is called by `@`, and given the above enum in `INDIRECTION_DATA`. // Get the size, and update `INDIRECTION_DATA` to point to the data. #emit LOAD.alt INDIRECTION_DATA // 2 ``` Just moving that purely compile-time const outside the function was enough to fix it!
- Loading branch information