1021251 - OdinMonkey: simplify AsmJSFrameIter a bit

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Luke Wagner [:luke]]

Attachment: tweak-asmjs-frame-iter

This patch cleans things up a little in preparation for later patches and also removes the 'returnAddress' field.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 2

[Luke Wagner [:luke]]

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

Comment 3

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4511d9ac1000

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4511d9ac1000

Comment 5

[Jeff Walden [:Waldo]]

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