[erlang-bugs] A bug in address patching in hipe_x86.c?

Yiannis Tsiouris gtsiour@REDACTED
Sun Feb 2 20:06:15 CET 2014


Hi all,

I'll try to address both Mikael's and Sveker's comments. :)

On 02/01/2014 05:46 PM, Mikael Pettersson wrote:
> Yiannis Tsiouris writes:
>  > ----------------------------------------------------------------------
>  > diff --git a/erts/emulator/hipe/hipe_x86.c b/erts/emulator/hipe/hipe_x86.c
>  > index 327c74e..6c233cb 100644
>  > --- a/erts/emulator/hipe/hipe_x86.c
>  > +++ b/erts/emulator/hipe/hipe_x86.c
>  > @@ -36,7 +36,7 @@
>  >  void hipe_patch_load_fe(Uint32 *address, Uint32 value)
>  >  {
>  >      /* address points to a disp32 or imm32 operand */
>  > -    *address = value;
>  > +    *address += value;
>  >  }
>  >  
>  >  int hipe_patch_insn(void *address, Uint32 value, Eterm type)
>  > @@ -46,14 +46,12 @@ int hipe_patch_insn(void *address, Uint32 value, Eterm type)
>  >        case am_constant:
>  >        case am_atom:
>  >        case am_c_const:
>  > -	break;
>  >        case am_x86_abs_pcrel:
>  > -	value += (Uint)address;
>  >  	break;
>  >        default:
>  >  	return -1;
>  >      }
>  > -    *(Uint32*)address = value;
>  > +    *(Uint32*)address += value;
>  >      return 0;
>  >  }
> 
> I don't understand why you would make that inference.  "address" is the
> address of the 32-bit immediate to be patched, "value" is the value to
> store there.  Except for the x86_abs_pcrel case, "value" is unrelated
> to "address".
> 
> This code is central to naive code loading, and if it indeed was broken
> then we would have seen hard crashes many years ago.  However, it's been
> known to work (with the HiPE compiler and loader) for 10+ years.

Mikael, you 're, of course right about what "address" and "value" are!
The code works exactly because of *address being zero (as you explained
in: "I believe the old values always will be zero, but that's only
because we have to pad the instruction stream with zeros to make room
for the to-be-patched immediates."). To verify that, I even tried adding
an assert(*address == 0) in both hipe_patch_load_fe and hipe_patch_insn
and, then, building the OTP with native libs (./configure
--enable-native-libs); I also run the "usual tests" and everything
worked like a charm.

> I belive your compiler and/or loader is doing something with relocations
> that differs from the HiPE compiler, and therefore doesn't work with the
> HiPE runtime.

After that, I tried to investigate why that doesn't work for us. One
test that fails is fun04.erl (attached). A small diff in the assembly
generated by vanilla hipe and ErLLVM hipe reveals:

Vanilla HiPE:
-------------
      a3 |                          | ._58:
      a3 | b800000000               |   mov
${{{fun04,'-test/0-fun-0-',1},71147871,0},closure}, %eax
      a8 | 894604                   |   mov %eax, 0x4(%esi)
      ab | 8b4828                   |   mov 0x28(%eax), %ecx
      ae | 894e0c                   |   mov %ecx, 0xc(%esi)
      b1 | c7461001000000           |   mov $0x1, 0x10(%esi)
      b8 | 83c034                   |   add $0x34, %eax
      bb | e800000000               |   call atomic_inc # [] 0x2 0 []
not_remote


ErLLVM HiPE:
------------
 # BB#13:
	movl	$_2D_test_2F_0_2D_fun_2D_1_2D_, 4(%esi)
	movl	_2D_test_2F_0_2D_fun_2D_1_2D_+40, %eax
	movl	%eax, 12(%esi)
	movl	$1, 16(%esi)
	leal	_2D_test_2F_0_2D_fun_2D_1_2D_+52, %eax
	calll	atomic_inc

That strange "_2D_test_2F_0_2D_fun_2D_1_2D_" is the name of the closure
after some mangling to be LLVM-compatible. Our (ErLLVM) code seems
semantically equivalent to (vanilla) HiPE's. However, the 2 additions
that involve the closure symbol complicate address patching. HiPE
doesn't face these problems because it always handles closures as shown
above.

IMHO, I don't think that we violate any kind of "runtime invariant" by
generating code in that way. I'd love to hear your opinion, too! :-)


On 01/31/2014 07:20 PM, Sverker Eriksson wrote:
> If this has worked before (which it has) then the old value of *address
> must always have been zero.
> 
> But it looks like hipe_patch_insn() can be called in a module upgrade
> scenario (see hipe_bifs_redirect_referred_from_1 in hipe_bif0.c)
> and then the old value of *address must (I guess?) refer to something in
> the old module instance.
> Adding old and new values can not be right (whatever "value" may be).
> 
> 
> Have you tried this patch with the existing x86 backend? And have you
> tried upgrading a module and verified that it is reachable after the
> upgrade?

I copied m.erl (attached) from here [1] and tried running:

gtsiour@REDACTED [~]>>= erl
Erlang/OTP 17 [DEVELOPMENT] [erts-6.0] [source-dee9682] [smp:4:4]
[async-threads:10] [hipe] [kernel-poll:false]

Eshell V6.0  (abort with ^G)
1> c(m).
{ok,m}
2> hipe:c(m).
{ok,m}
3> P = spawn(fun () -> m:loop() end).
<0.51.0>
4> P ! "bar".
Before: "bar"
"bar"
5> P ! "foo".
Before: "foo"
"foo"
6> c(m).
{ok,m}
7> hipe:c(m).
{ok,m}
8> P ! "foo".
Before: "foo"
"foo"

Then, I tried commenting-out line 9 and uncommenting line 10 and did:

9> P ! code_switch.
code_switch
10> P ! "foo".
After: "foo"
"foo"
11> q().
ok
12>

Do you think that this test suffices? I'd be happy to test more
thoroughly... Just point me to some tests/scenarios!


Thanks for the prompt replies!
~Y.


[1]: http://www.erlang.org/doc/reference_manual/code_loading.html

-- 
Yiannis Tsiouris
Ph.D. student,
Software Engineering Laboratory,
National Technical University of Athens
WWW: http://www.softlab.ntua.gr/~gtsiour
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fun04.erl
Type: text/x-erlang
Size: 460 bytes
Desc: not available
URL: <http://erlang.org/pipermail/erlang-bugs/attachments/20140202/f5b62f4b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: m.erl
Type: text/x-erlang
Size: 199 bytes
Desc: not available
URL: <http://erlang.org/pipermail/erlang-bugs/attachments/20140202/f5b62f4b/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 259 bytes
Desc: OpenPGP digital signature
URL: <http://erlang.org/pipermail/erlang-bugs/attachments/20140202/f5b62f4b/attachment-0002.bin>


More information about the erlang-bugs mailing list