Beefy Boxes and Bandwidth Generously Provided by pair Networks
Don't ask to ask, just ask


by BrowserUk (Pope)
on Sep 27, 2011 at 04:51 UTC ( #928010=note: print w/replies, xml ) Need Help??

in reply to Re^7: XS: EXTEND/mPUSHi
in thread XS: EXTEND/mPUSHi

Hm. If you consider this "not broken", then I guess it isn't. Hm. If you consider this "not broken", then I guess it isn't.

But if some of that seems unnecessary, then ... :)

Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
"Science is about questioning the status quo. Questioning authority".
In the absence of evidence, opinion is indistinguishable from prejudice.

Replies are listed 'Best First'.
by ikegami (Pope) on Sep 27, 2011 at 07:43 UTC

    The doing it twice is not a problem. Compilers will just get rid of the dead code. There's unreachable code every time you end an XS function using XSRETURN and the likes. That can't be that rare (especially XSRETURN_YES to return true), and it's got nothing to do with Inline::C.

    If you had look at the .xs instead of the .c, you can't even tell there's duplication.

    The problem is that Inline::C differs from XS. In XS, one doesn't call PUTBACK. In Inline::C, one must. In XS, the args are poped for you. In Inline::C, one must do so. Is this documented? Don't know. But even if it is, it's confusing and error prone.

by syphilis (Chancellor) on Sep 29, 2011 at 08:41 UTC
    But if some of that seems unnecessary, then ... :)

    Ike obviously recognised the duplication to which you alluded ... but not me, alas.
    Could you elaborate ?

    Update: Also, ikegami mentioned making Inline_Stack_Reset a no-op. That just means amending Inline.h to do something like:
    #define Inline_Stack_Reset {}
    Right ?


      The the XS wrappers generated by I::C:

      XS(XS_main_rnd64); /* prototype to pass -Wmissing-prototypes */ XS(XS_main_rnd64) { #ifdef dVAR dVAR; dXSARGS; #else dXSARGS; #endif if (items != 1) croak_xs_usage(cv, "n"); PERL_UNUSED_VAR(ax); /* -Wall */ SP -= items; ### <<<<<<< Has no effect { int n = (int)SvIV(ST(0)); #line 64 "monkeys.xs" I32* temp; #line 135 "monkeys.c" #line 66 "monkeys.xs" temp = PL_markstack_ptr++; rnd64(n);

      Prior to calling the C-func, the stack is adjusted to account for the number of input parameters passed:

      SP -= items;

      But this never has any affect because SP is a local copy of the real stack pointer. In order to 'remove' the passed parameters from the stack (inside the C-func) you have to call Inline_Stack_Reset; which translates to sp = mark;

      Then after you've pushed your return parameters onto the stack, you have to remember to call Inline_Stack_Done; which translates to PUTBACK;, which translates to PL_stack_sp = sp;. Which updates the real stack pointer from tha local copy to account for the net effect of removing the input parameters and adding the output parameters.

      But the generated XS wrapper already has code to take care of this, except it has been disabled by having been preceded with a(nother) return:

      return; /* assume stack size is correct */ #line 146 "monkeys.c" PUTBACK; ### << Never reached return; } }

      What we discussed above is that if PUTBACK was called after SP -= items: and before the C-sub is invoked, there would be no need to call Inline_Stack_Reset; within the C-sub, because that stack would have already been adjusted to account for the input parameters. And the Stack access (ST(0); xPUSHy() & Inline_Stack_Push() etc.) macros would still operate correctly.

      And then, if the first return statement was removed from the wrapper sub, so that the currently unreached PUTBACK; was reached, then the real stack would again be updated this time reflecting whatever output parameters had been push onto the stack (if any), thereby removing the need for the Inline_Stack_done; macro.

      In summary: The theory went that if the wrapper sub were generated as (discarding the extraneous stuff for clarity) :

      XS(XS_main_xxxxx) { dXSARGS; if (items != 1) croak_xs_usage(cv, "n"); SP -= items; { int n = (int)SvIV(ST(0)); I32* temp = PL_markstack_ptr++; PUTBACK; xxxxx(n); if (PL_markstack_ptr != temp) { PL_markstack_ptr = temp; XSRETURN_EMPTY; } PUTBACK; return; } }

      Then Inline_Stack_Reset; & Inline_Stack_Done; become redundant for new code, and could be made noops for backward compatibility.

      The fly in this ointment is that there are the XS macros POPx, which would no longer operate correctly if mixed into I::C.

      I wouldn't consider this a loss as there are alternative and better ways of accessing the input stack, but I've no doubt that some of those p5p for who backward compatibility is sacrosanct would be up in arms.

      Bottom line: Your 'do nothing' was probably the right call. Sorry for having suggested otherwise.

      Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
      "Science is about questioning the status quo. Questioning authority".
      In the absence of evidence, opinion is indistinguishable from prejudice.
        Heh - I must've looked at the C code a hundred times over the years, and never noticed the unreached
        PUTBACK; return;
        ... though, as I write this, I'm suddenly struck with a contradictory sense of "deja vu".

        That unreached code *is* confusing and/or annoying, and it would be nice to make that piece of xsubbp-generated code reachable (as you suggest) - which means amending in a way that makes me feel a bit uneasy.

        Your 'do nothing' was probably the right call

        Maybe ... I *have* been right before, y'know !! ... by accident, of course ( ... which would also be the case in this instance :-)

        I'll chew this over for a while, as time permits. I've already checked that ikegami's suggested changes re the "no-op" and the "PUTBACK" insertions don't break the test suite. (However, they still leave us with that unreached "return" ... not that he suggested they would do otherwise.)

        Thanks BrowserUk, ikegami.


Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://928010]
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others studying the Monastery: (2)
As of 2018-08-18 12:00 GMT
Find Nodes?
    Voting Booth?
    Asked to put a square peg in a round hole, I would:

    Results (185 votes). Check out past polls.