process_info(P,current_function) and HiPE (was Re: Bugs with hibernate/3 and HiPE)

[Mikael Pettersson]

Paul Guyot writes:
 > Mikael,
 > 
 > Thank you for your e-mail.
 > 
 > > Please investigate:
 > > a) when and how BEAM updates current_function, and
 > 
 > c_p->current is either set to NULL or to a pointer to an {M,F,A} instruction triplet.
 > 
 > It is set:
 > - on context switch (which are always when entering a function)
 > - when calling a NIF
 > - on apply_bif
 > - on hibernate
 > - on various exception handlers and signaling.
 > 
 > > b) when you're allowed to call process_info(P, current_function) on
 > > a BEAM-mode function and what it returns; what if the target process
 > > is currently running? does it stop or does process_info then return
 > > stale data
 > 
 > I guess process_info returns stale data (simply the value of process->current).
 > 
 > > c) to what extent the hibernate test relies on the above
 > 
 > It is only used to figure out if the process is or is not hibernating. Indepently from the test, what I am mostly concerned with is that erts_hibernate sets current to {erlang,hibernate,3} and when the process wakes up, we should set it to a proper value (either the function being executed or NULL). If the wake up function is native, this does not happen and it remains {erlang,hibernate,3}.
 > 
 > The gen_server_SUITE:hibernate/1 test passes when simply setting c_p->current to I-3 in beam_emu.c's OpCase(hipe_trap_call), i.e. defining current to be the called native function. However, this looks weird because the data is definitely incorrect when the process is later switched out, waiting for a message. So I suggest to also clear current when exiting hipe_mode_switch in wait or suspend states. This is the content of the patch below. If you're fine with it, I'll submit it to erlang-patches@REDACTED
 > 
 > Paul
 > -- 
 > Semiocast                    http://semiocast.com/
 > +33.175000290 - 62 bis rue Gay-Lussac, 75005 Paris
 > 
 > diff --git a/erts/emulator/beam/beam_emu.c b/erts/emulator/beam/beam_emu.c
 > index 8a0e12d..ce49299 100644
 > --- a/erts/emulator/beam/beam_emu.c
 > +++ b/erts/emulator/beam/beam_emu.c
 > @@ -4850,6 +4850,7 @@ apply_bif_or_nif_epilogue:
 >           * ... remainder of original BEAM code
 >           */
 >          ASSERT(I[-5] == (Uint) OpCode(i_func_info_IaaI));
 > +        c_p->current = I-3;
 >          c_p->hipe.ncallee = (void(*)(void)) I[-4];
 >          cmd = HIPE_MODE_SWITCH_CMD_CALL | (I[-1] << 8);
 >          ++hipe_trap_count;
 > diff --git a/erts/emulator/hipe/hipe_mode_switch.c b/erts/emulator/hipe/hipe_mode_switch.c
 > index e5de244..181f9c8 100644
 > --- a/erts/emulator/hipe/hipe_mode_switch.c
 > +++ b/erts/emulator/hipe/hipe_mode_switch.c
 > @@ -425,6 +425,7 @@ Process *hipe_mode_switch(Process *p, unsigned cmd, Eterm reg[])
 >        case HIPE_MODE_SWITCH_RES_SUSPEND: {
 >           p->i = hipe_beam_pc_resume;
 >           p->arity = 0;
 > +         p->current = NULL;
 >           erts_smp_proc_lock(p, ERTS_PROC_LOCK_STATUS);
 >           if (p->status != P_SUSPENDED)
 >               erts_add_to_runq(p);
 > @@ -445,6 +446,7 @@ Process *hipe_mode_switch(Process *p, unsigned cmd, Eterm reg[])
 >           p->i = hipe_beam_pc_resume;
 >           p->arity = 0;
 >           p->status = P_WAITING;
 > +         p->current = NULL;
 >           erts_smp_proc_unlock(p, ERTS_PROC_LOCKS_MSG_RECEIVE);
 >        do_schedule:
 >           {
 > 

I don't like this, it's adding what I believe are redundant assignments
in hot paths.  The essential problem is that BEAM's hibernate test expects
->current to change soon after wakeup from hibernation.  There is no special
event for this, only a plain apply of an MFA.  In the bug case, that MFA
is native and nothing in that path sets ->current.

If you look at the start of hipe_mode_switch() you see an assignment of
NULL to p->i.  Just add "p->current = NULL;" at the same place, with a
comment that BEAM's hibernation test relies on it, and drop these other
3 assignments to ->current.  That should be enough.

/Mikael