1523571 - Add an assertion when trying to store the result of a bool-returning VMFunction in CodeGeneratorShared::oolCallVM

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[André Bargull [:anba]]

When a VMFunction has no out-param and returns bool, it doesn't make sense to store the bool return value in CodeGeneratorShared::oolCallVM, because the caller will always only see true, since false indicates the failure path needs to be taken. An assertion will help to catch this misuse earlier (during code generation) rather than at runtime when executing the OOL code, which could've helped me catching a thinko in another patch earlier. :-)

Comment 1

[André Bargull [:anba]]

Attachment: bug1523571.patch

Add assertions to ensure the VMFunction argument count and return type matches the ArgSeq and StoreOutput arguments. And then update CodeGenerator::visitRecompileCheck to use StoreNothing, because neither ForcedRecompileFnInfo nor RecompileFnInfo have actual return-data to store meaningfully.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9039784 [details] [diff] [review]
bug1523571.patch

Review of attachment 9039784 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: js/src/jit/CodeGenerator.cpp
@@ +11662,5 @@
>    jumpTable->setOutOfLine();
>    auto& labels = jumpTable->labels();
>  #if defined(JS_CODEGEN_ARM64)
> +  AutoForbidPools afp(
> +      &masm, (labels.length() + 1) * (sizeof(void*) / vixl::kInstructionSize));

nit: undo this changes from this patch.

Comment 3

[André Bargull [:anba]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)

::: js/src/jit/CodeGenerator.cpp
@@ +11662,5 @@

jumpTable->setOutOfLine();
auto& labels = jumpTable->labels();
#if defined(JS_CODEGEN_ARM64)

• AutoForbidPools afp(

•  &masm, (labels.length() + 1) * (sizeof(void*) / vixl::kInstructionSize));
  

nit: undo this changes from this patch.

This a clang-format change, so it'll happen anyway when Sylvestre does his next clang-format across the whole tree. Do you still want me to undo it?

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to André Bargull [:anba] from comment #3)

(In reply to Nicolas B. Pierron [:nbp] from comment #2)

::: js/src/jit/CodeGenerator.cpp
@@ +11662,5 @@

jumpTable->setOutOfLine();
auto& labels = jumpTable->labels();
#if defined(JS_CODEGEN_ARM64)

• AutoForbidPools afp(

•  &masm, (labels.length() + 1) * (sizeof(void*) / vixl::kInstructionSize));
  

nit: undo this changes from this patch.

This a clang-format change, so it'll happen anyway when Sylvestre does his next clang-format across the whole tree. Do you still want me to undo it?

Yes, because this is unrelated to this change, and Sylvestre commit would be excluded from the history traversal.

Comment 5

[André Bargull [:anba]]

Attachment: bug1523571.patch

Updated to exclude unrelated clang-format change, carrying r+.

Comment 6

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec48de5f17801ca8e7c15b4d68142dd46a980b97

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f34ff529f92e
Prevent storing VMFunction return value when no actual return-data is present. r=nbp

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f34ff529f92e