[erlang-bugs] Bugs with hibernate/3 and HiPE

[Paul Guyot]

Mikael,

Thank you for your e-mail.

>> There are several bugs related to hibernate/3 BIF and HiPE:
>> 
>> 1. erlang:hibernate/3 simply fails with badarg when called from HiPE.
>> 2. erlang:hibernate/3 simply fails with badarg when called dynamically, in a way that the compiler and the loader cannot replace with the i_hibernate beam instruction.
>> 3. There is a segfault in the garbage collector when calling erlang:hibernate/3 (from beam unless bug #1 is fixed) if the process went through HiPE. This is because erts_garbage_collect_hibernate would not clean up the hipe process state.
>> 
>> These bugs are highly related: 1 and 2 are actually the same bug (hibernate/3 BIF isn't implemented, and apparently wasn't since it was introduced), and 3 causes segfaults with any reasonable test once bug #1 is fixed.
> 
> #1 and #2 are not HiPE bugs but BEAM bugs.  Especially the fact that
> erlang:hibernate/3 can be called in ways that the BEAM compiler/loader
> combo doesn't recognize and turn into the i_hibernate BEAM instruction.
> #3 is a HiPE bug, in that HiPE never added support for hibernation,
> (it's not that we added the support in a broken form).

Who's bug it is does not really matter. From a user perspective, in R14B and earlier, erlang:hibernate/3 would not work when called from a native module and can crash in complex scenarios if the process previously went through some native code.

> a. Testing a flag in a-bif-just-trapped situations is expedient but
> extremely ugly.  BEAM has the i_hibernate instruction.  Either the
> trap should target a BEAM stub which executes i_hibernate, or i_hibernate
> should be eliminated altogether in favour of the flag test(s).  There
> should be only one way to hibernate.

While I agree that there should be only one way to hibernate (hence the erts_hibernate function), I chose not to TRAP to a new BEAM stub and test against the status flag for two reasons:
- the proposed solution involves the smallest change
- going to do_schedule instead of immediatly calling the TRAP function (HiPE) or doing DoDispatch (BEAM) when status flag is P_WAITING (in other words, if the process should be rescheduled) doesn't break any semantics at all, works for hibernating and could be extended for other yield cases (I think TRAP is used in some BIFs to give CPU time to other processes).

> b. Drop the redundant braces in
> 	if (cond) {
> 		goto blah;
> 	}

Is there a reference or commonly approved style guidelines for OTP source code? The braces match what can be found in beam_emu.c.

> c. In the hibernate_3 BIF you're declaring an unused `result' variable.

Well spotted!

> d. In the hibernate_3 BIF you're assigning to BIF_P->def_arg_reg[3] but
> I don't see anything in the patch that would use it.  Is it redundant?

It is used in BEAM TRAP handler:
http://github.com/erlang/otp/blob/dev/erts/emulator/beam/beam_emu.c#L3200

Paul
-- 
Semiocast                    http://semiocast.com/
+33.175000290 - 62 bis rue Gay-Lussac, 75005 Paris