IonMonkey: ContainsCodeAddress has an off by 1 error

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mjrosenb, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 596214 [details] [diff] [review]
/home/mrosenberg/patches/fixFunctionCheck-r0.patch

we call containsCodeAddress with a pointer that is the return address from a function.
For the most part, this works, but in some cases, when we know a call isn't going to return, no code is placed after the call, so the return address is not technically part of the function, and this makes walking up the stack *quite* sad.
Attachment #596214 - Flags: review?(sstangl)
Comment on attachment 596214 [details] [diff] [review]
/home/mrosenberg/patches/fixFunctionCheck-r0.patch

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

::: js/src/ion/IonCode.h
@@ +286,5 @@
> +        // however, when the code for exceptions is generated, there is no code
> +        // after the call.  If that was the last instruction in the function,
> +        // then the return address would be exactly at the upper bound of the
> +        // function, so it has been changed to <=
> +        return method()->raw() <= addr && addr <= method()->raw() + method()->instructionsSize();

This change causes containsCodeAddress() to lie for the purpose of appeasing exceptions. Instead of having this function lie, could we solve the problem locally in the exception generators by inserting NOPs into the code stream, with commentary?
Attachment #596214 - Flags: review?(sstangl)
(Reporter)

Comment 2

6 years ago
Created attachment 597509 [details] [diff] [review]
/home/mrosenberg/patches/fixFunctionCheck-r1.patch
Attachment #596214 - Attachment is obsolete: true
Attachment #597509 - Flags: review?(sstangl)

Updated

6 years ago
Attachment #597509 - Flags: review?(sstangl) → review+
(Reporter)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.