Skip to content

Raise a jitted cppia exception at the boundary back to native code - #1368

Open
MeguminBOT wants to merge 1 commit into
HaxeFoundation:masterfrom
MeguminBOT:fix-cppia-jit-throw-at-boundary
Open

Raise a jitted cppia exception at the boundary back to native code#1368
MeguminBOT wants to merge 1 commit into
HaxeFoundation:masterfrom
MeguminBOT:fix-cppia-jit-throw-at-boundary

Conversation

@MeguminBOT

Copy link
Copy Markdown

The problem

With the JIT on, a cppia function that throws does not throw. The call returns null, and after
that every call into jitted cppia returns null for the rest of the process, including
functions that are perfectly fine.

Why

Jitted cppia does not raise C++ exceptions. ThrowExpr::genCode writes the value to
ctx->exception and calls addThrow, which jumps to the function epilogue. Jitted code then checks
ctx->exception after every call it makes to other jitted code, so inside jitted code the unwind
works.

The gap is where jitted code returns to native code:

  • ScriptCallable::runFunction and ScriptCallable::runFunctionClosure call compiled(ctx) and
    return without checking ctx->exception at all.
  • The compiled branch of CppiaClosure::__run does check it, but only to skip reading a return
    value, and then returns null() with the exception still sitting there.

So the caller gets null instead of an exception. Worse, ctx->exception is never cleared, so the
next jitted function to run bails out at its first checkException.

The fix

In src/hx/cppia/CppiaFunction.cpp, all three sites now raise the exception and clear the context:

if (ctx->exception)
{
   Dynamic caught = ctx->exception;
   ctx->exception = nullptr;
   HX_STACK_DO_THROW(caught);
}

That is the same thing TryExpr::runVoid already does to finish its own unwind on the non-jit
path:

if (ctx->exception)
   handleException(ctx, ctx->exception);

In runFunction and runFunctionClosure the AutoFrame is scoped so the frame is popped before the
throw. CppiaClosure::__run already does this a few hundred lines further down.

Test

test/cppia covers it. ClientThrower in Client.hx has one method that throws and one that
returns a value, and testThrowReachesTheCaller in cases/TestCommon.hx checks that the throw
arrives and that the second call still answers afterwards.

cd test/cppia
haxe compile-host.hxml
haxe compile-client.hxml
cd bin && ./CppiaHost.exe client.cppia -jit

The -jit matters. Host.main already takes that flag, and without it the non-jit path handles
the throw correctly and the test passes whether or not the fix is in.

With the fix reverted and everything else applied, the whole suite run with -jit does not complete
at all: no utest output, exit 127. The same build without -jit reports ALL TESTS OK.

Reproducing by hand

Script.hx, built with haxe -m Script --cppia script.cppia:

class Script {
   public static function run():String {
      throw 'boom';
   }

   public static function after():String {
      return 'still here';
   }

   public static function main():Void {}
}

Load it from a host built with -D scriptable, using
cpp.cppia.Module.fromData(bytes).boot(), then call Script.run and after that Script.after
through reflection, each in a try/catch.

run() after()
before, JIT on returns null returns null
after, JIT on throws boom still here
either way, JIT off throws boom still here

The second column is the reason this matters. Script.after never threw and has nothing wrong with
it. It returned null only because the context was still holding the earlier exception.

Let me know if I got anything wrong!

Jitted cppia does not throw. ThrowExpr::genCode writes the value to ctx->exception and
jumps to the function epilogue, and jitted code checks the context after every call it
makes to other jitted code, so within jitted code the unwind is complete.

Nothing did that where jitted code returns to native code. runFunction and
runFunctionClosure call compiled(ctx) and never look at ctx->exception, and the compiled
branch of CppiaClosure::__run looks only to decide against reading a return value, then
answers null() with the exception still set. The caller reads null, and since nothing
clears the context, every later call into jitted cppia returns at its first
checkException.

All three sites now raise it and clear the context, which is the shape TryExpr::runVoid
already uses on the interpreted path.

The cppia test suite covers it, run with -jit.
@tobil4sk

Copy link
Copy Markdown
Member

I think these functions can also be called internally within cppia code, but we want to change only the boundary between the host and cppia code.

@MeguminBOT

Copy link
Copy Markdown
Author

I think these functions can also be called internally within cppia code, but we want to change only the boundary between the host and cppia code.

I went and traced it properly.
runFunction gets reached from StackContext::run* in CppiaCtx.cpp, which the interpreter uses for its own CallFunExpr/CallMemberVTable calls, and doRun gets reached whenever jitted code calls a closure, since callDynamic goes through __Run and lands there. So both are internal paths too, not just the way in from the host. jit->jit calls go straight to function->compiled, so the hot path never touched any of this, but I was still converting the exception in the wrong place.

Dropping the runFunctionClosure hunk entirely while I'm at it. Its only caller is doRun, which bails out earlier if function->compiled is set, so that branch can't ever run.

What I have locally now is what CppiaLoadedModule::run() already does, check ctx->exception after the call and rethrow, but at the entry points: CppiaClosure::__Run and __run, which is what the test goes through. runFunction, runFunctionClosure and doRun go back to untouched.

Before I push it though, I want to check I've got the right line. __Run and __run are reachable from inside cppia as well, from callDynamic and from the interpreted call expressions, so what I'd really be changing is the native calling convention surface rather than something only the host touches.
The difference from runFunction is that every internal caller is already prepared for a C++ throw coming out of it, callDynamic wraps the call in TRY_NATIVE, and the interpreter throws C++ exceptions anyway. Is that the boundary you meant, or would you rather it was gated so it only fires on the outermost cppia frame and internal closure calls stay exactly as they are today?

@tobil4sk

Copy link
Copy Markdown
Member

would you rather it was gated so it only fires on the outermost cppia frame and internal closure calls stay exactly as they are today?

This would be ideal, to avoid introducing unnecessary overhead to the jit. It might not be trivial though. I wonder if @hughsando might have some suggestion on what to do here?

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.

2 participants