Skip to content

Fix a cppia Bool constant stored as a number in an Any array under the JIT - #1371

Open
MeguminBOT wants to merge 1 commit into
HaxeFoundation:masterfrom
MeguminBOT:fix-cppia-bool-const-jit-boxing
Open

Fix a cppia Bool constant stored as a number in an Any array under the JIT#1371
MeguminBOT wants to merge 1 commit into
HaxeFoundation:masterfrom
MeguminBOT:fix-cppia-bool-const-jit-boxing

Conversation

@MeguminBOT

Copy link
Copy Markdown

The problem

Same script, different answers depending on whether the JIT is on. A bool constant in an
Array<Any> comes back out as a number:

var a:Array<Any> = [true, false];

Std.string(a[0]);         // "true" interpreted, "1" jitted
Std.isOfType(a[0], Bool); // true interpreted, false jitted

It really is an Int sitting in the array, so type checks against Bool fail and it serialises as a
number. It's still truthy, so if (a[0]) keeps working, which is why this can sit there unnoticed
until something trips over it.

Why

ArrayDef::genCode asks each item whether it's a bool to pick the store:

case etInt:
   items[i]->genCode(compiler, sJitArg2, etInt );
   if (items[i]->isBoolInt())
      compiler->callNative((void *)varraySetBool,array,i,sJitArg2.as(jtInt));
   else
      compiler->callNative((void *)varraySetInt,array,i,sJitArg2.as(jtInt));

Constants live in DataVal<T>, and getType says etInt for a bool, so isBoolInt is the only
thing that can tell one apart from a number. DataVal never overrode it, so it answered false and
the element went in through varraySetInt.

Everything else carrying a bool already does this. CppiaBoolExpr returns true outright,
MemReference gets it from ExprTypeIsBool, and the call and cast expressions read their signature.
DataVal was just missed.

The interpreter fills the same array through runObject, which boxes a Dynamic from the real
bool, so it was never wrong.

The fix

One override in src/hx/cppia/Cppia.cpp:

bool isBoolInt() HXCPP_OVERRIDE { return ExprTypeIsBool<T>::value; }

This only affects array literals. var d:Dynamic = true already boxed correctly, since that path
never asks isBoolInt.

Test

test/cppia covers it. ClientBoolConst in Client.hx returns the arrays and
cases/TestCommon.hx checks what comes back.

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

The -jit matters, and RunTests.hx already runs the suite both ways. It passes in both.

Without the fix, testBoolConstInAnyArray fails under -jit and passes interpreted.

One thing worth knowing if you touch these tests:
They hand the array back to the host on purpose.
Write it the obvious way, reading the array back in the same function, and the analyzer replaces it
with locals. No array gets built and the test quietly checks nothing. Took me a couple of runs to
notice. testBoolComparisonInAnyArray is there as a control, since a comparison in the same slot
already answers isBoolInt and passes either way.

Reproducing by hand

class Script {
   public static function run():Array<Any> {
      return [true, false];
   }

   public static function main():Void {}
}

Built with haxe -m Script --cppia script.cppia and loaded from a host built with
-D scriptable --dce no, calling cpp.cppia.Host.enableJit(true) before
cpp.cppia.Module.fromData(bytes).boot(), then reading Std.string(arr[0]).

result
before, JIT on 1
after, JIT on true
either way, JIT off true

Wheter there's a better fix for this I'm not sure, if there is, please point it out and I'll fix it up.

DataVal never overrode isBoolInt, so ArrayDef::genCode stored a constant
true in an Array<Any> through varraySetInt and it read back as 1. The
interpreter was never affected.
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.

1 participant