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

RESOLVED FIXED in Firefox 67

Status

()

enhancement
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla67
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

5 months ago

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. :-)

Assignee

Comment 1

5 months ago
Posted patch bug1523571.patch (obsolete) — Splinter Review

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.

Attachment #9039784 - Flags: review?(nicolas.b.pierron)
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.
Attachment #9039784 - Flags: review?(nicolas.b.pierron) → review+
Assignee

Comment 3

5 months ago

(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?

(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.

Assignee

Comment 5

5 months ago

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

Attachment #9039784 - Attachment is obsolete: true
Attachment #9039830 - Flags: review+

Comment 7

5 months ago

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

Keywords: checkin-needed

Comment 8

5 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.