[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