Closed Bug 1021251 Opened 6 years ago Closed 6 years ago

OdinMonkey: simplify AsmJSFrameIter a bit

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(1 file)

This patch cleans things up a little in preparation for later patches and also removes the 'returnAddress' field.
Attachment #8435234 - Flags: review?(benj)
Comment on attachment 8435234 [details] [diff] [review]
tweak-asmjs-frame-iter

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

Nice refactoring!

::: js/src/jit/AsmJSLink.cpp
@@ +57,5 @@
> +    // at ShadowStackSpace above sp; an exit to Ion will store the return
> +    // address at sp. To distinguish the two cases, the low bit of sp (which is
> +    // aligned and therefore zero) is set for Ion exits.
> +    if (uintptr_t(sp) & 0x1) {
> +        sp = *psp -= 0x1;  // Clear the low bit

That's a dense statement!

@@ +69,3 @@
>  
> +static uint8_t *
> +ReturnAddressForJitCall(uint8_t *sp)

I would just move this function right above settle or operator++ in this file, so that definition and use are close to each other, but that's a detail.

@@ +124,5 @@
> +
> +    sp_ += callsite_->stackDepth();
> +
> +    if (callsite_->isExit())
> +        return settle(ReturnAddressForJitCall(sp_));

Wow, I didn't know we can return a call returning void in a void function. Neat recursive call btw.
Attachment #8435234 - Flags: review?(benj) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> I would just move this function right above settle or operator++ in this
> file, so that definition and use are close to each other, but that's a
> detail.

That makes sense and I'd usually do just that, but in this case, it felt subjectively better to put the two Return* together, so you could compare/contrast them easier.

> Wow, I didn't know we can return a call returning void in a void function.
> Neat recursive call btw.

lol, neither did I.  I just looked it up (in case it was only allowed as a gcc extension), and it seems to be explicitly allowed in the spec (6.6.3.3).
https://hg.mozilla.org/mozilla-central/rev/4511d9ac1000
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(In reply to Luke Wagner [:luke] from comment #2)
> > Wow, I didn't know we can return a call returning void in a void function.
> > Neat recursive call btw.
> 
> lol, neither did I.  I just looked it up (in case it was only allowed as a
> gcc extension), and it seems to be explicitly allowed in the spec (6.6.3.3).

It's a C++ thing, I think primarily to make templated-return-type code not have to special-case.  In contrast C (even up to C11) disallows returning an expression in a void function.
You need to log in before you can comment on or make changes to this bug.