Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should PUSHSTACKi() set the mark? #13331

Closed
p5pRT opened this issue Oct 4, 2013 · 6 comments
Closed

Should PUSHSTACKi() set the mark? #13331

p5pRT opened this issue Oct 4, 2013 · 6 comments

Comments

@p5pRT
Copy link

p5pRT commented Oct 4, 2013

Migrated from rt.perl.org#120100 (status was 'rejected')

Searchable as RT120100$

@p5pRT
Copy link
Author

p5pRT commented Oct 4, 2013

From @nwc10

As best I can tell, PUSHSTACKi() is completely undocumented. It pushes a new
(Perl) stack (ie the thing for @​_), so that the old stack doesn't need to
move in memory if it needs to get larger.

If I understand it correctly, the mark stack marks places on the perl stack.
marks are stored as I32s [yay :-(] which give the index of the marked spot
on the Perl stack. ie, positions in the array which implements the current
stack. It works like this​:

#define PUSHMARK(p) \
  STMT_START { \
  if (++PL_markstack_ptr == PL_markstack_max) \
  markstack_grow(); \
  *PL_markstack_ptr = (I32)((p) - PL_stack_base);\
  } STMT_END

and that last assignment is using pointer arithmetic to calculate the offset.

Now, when PUSHSTACKi() does its stuff, it calls SWITCHSTACK, which does this​:

#define SWITCHSTACK(f,t) \
  STMT_START { \
  AvFILLp(f) = sp - PL_stack_base; \
  PL_stack_base = AvARRAY(t); \
  PL_stack_max = PL_stack_base + AvMAX(t); \
  sp = PL_stack_sp = PL_stack_base + AvFILLp(t); \
  PL_curstack = t; \
  } STMT_END

ie, use a different array. So at this point, the index represented by the
mark is garbage.

This strikes me as a bit of gotcha. So should we change SWITCHSTACK() to
set a mark at the start of the swapped-in stack?
I don't think that anything on CPAN is using SWITCHSTACK() directly, and
the 9 non-multicall users of PUSHSTACK()/PUSHTACKi() on CPAN all seem to set
a mark themselves (or I think don't need it). Likewise all the core code
either sets a mark, or calls Perl_load_module(). So I don't think that there
are any current bugs. It just feels like something we could make more robust.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Oct 4, 2013

From @cpansprout

On Fri Oct 04 05​:14​:19 2013, nicholas wrote​:

As best I can tell, PUSHSTACKi() is completely undocumented. It pushes
a new
(Perl) stack (ie the thing for @​_), so that the old stack doesn't need
to
move in memory if it needs to get larger.

If I understand it correctly, the mark stack marks places on the perl
stack.
marks are stored as I32s [yay :-(] which give the index of the marked
spot
on the Perl stack. ie, positions in the array which implements the
current
stack. It works like this​:

#define PUSHMARK(p) \
STMT_START { \
if (++PL_markstack_ptr == PL_markstack_max) \
markstack_grow(); \
*PL_markstack_ptr = (I32)((p) - PL_stack_base);\
} STMT_END

and that last assignment is using pointer arithmetic to calculate the
offset.

Now, when PUSHSTACKi() does its stuff, it calls SWITCHSTACK, which
does this​:

#define SWITCHSTACK(f,t) \
STMT_START { \
AvFILLp(f) = sp - PL_stack_base; \
PL_stack_base = AvARRAY(t); \
PL_stack_max = PL_stack_base + AvMAX(t); \
sp = PL_stack_sp = PL_stack_base + AvFILLp(t); \
PL_curstack = t; \
} STMT_END

ie, use a different array. So at this point, the index represented by
the
mark is garbage.

This strikes me as a bit of gotcha. So should we change SWITCHSTACK()
to
set a mark at the start of the swapped-in stack?
I don't think that anything on CPAN is using SWITCHSTACK() directly,
and
the 9 non-multicall users of PUSHSTACK()/PUSHTACKi() on CPAN all seem
to set
a mark themselves (or I think don't need it). Likewise all the core
code
either sets a mark, or calls Perl_load_module(). So I don't think that
there
are any current bugs. It just feels like something we could make more
robust.

Anything popping the caller’s mark is already buggy, so having a bad
mark that will crash will make the bug more apparent, no?

Unless you meant pushing a new markstack, but that may be overkill.
Nobody stores pointers into the markstack as far as I know.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Oct 4, 2013

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Oct 4, 2013

From @iabyn

On Fri, Oct 04, 2013 at 06​:16​:27AM -0700, Father Chrysostomos via RT wrote​:

Anything popping the caller’s mark is already buggy, so having a bad
mark that will crash will make the bug more apparent, no?

+1

Also, when dumping the stack with -Ds and -Dsv, the si_markoff field
under DEBUGGING builds already records the "high water mark" of the
mark stack at the time of switching so that the '*'s it outputs are still
correct.

The only thing that makes me slightly twitchy is the bare
SWITCHSTACK in pp_split() (i.e it doesn't have an accompanying
PUSHSTACKi), which allows one to split directly into an array while
pretending you're just pushing items on the stack. But I can't think of
any way that will mess with the mark stack.

--
I thought I was wrong once, but I was mistaken.

@p5pRT
Copy link
Author

p5pRT commented Oct 26, 2013

From @cpansprout

On Fri Oct 04 15​:36​:19 2013, davem wrote​:

On Fri, Oct 04, 2013 at 06​:16​:27AM -0700, Father Chrysostomos via RT wrote​:

Anything popping the caller’s mark is already buggy, so having a bad
mark that will crash will make the bug more apparent, no?

+1

I think that means I can mark this as rejected.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Oct 26, 2013

@cpansprout - Status changed from 'open' to 'rejected'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant