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

[Mikael Pettersson]

Paul Guyot writes:
 > Hello,
 > 
 > 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).

 > The following patch fixes these issues:
 > http://github.com/pguyot/otp/commit/fcc005c8758d97d7f0c402bfffede0b22b8ab401
 > git fetch git://github.com/pguyot/otp.git fix-hibernate-with-hipe

At a high level I think this is a step in the right direction.

The commit has some issues which I can only comment on abstractly
since you didn't inline it here.

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.

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

c. In the hibernate_3 BIF you're declaring an unused `result' variable.
Also you're breaking up the local variables with a multi-line comment
in the middle: they should all follow the comment, or the comment should
be moved above the function head.

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?

/Mikael