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

[Yiannis Tsiouris]

Hi all,

On 02/03/2014 03:00 PM, Mikael Pettersson wrote:
> Yiannis Tsiouris writes:
<snip>
>   > > 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
>
> So for the 2nd and and 5th instructions above, I guess you generate
> inital object code with 40 and 52, respectively, in the imm32 fields,
> and then have relocation entries referring to those imm32s and the
> _2D_test_2F_0_2D_fun_2D_1_2D_ label.  If so then I can see why you
> would want the runtime patcher to do += rather than =.
>
> In ELF terms, you're using Elf32_Rel while HiPE uses Elf32_Rela.
>
>   > 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! :-)
>
> Your approach is reasonable.  My concern is only that you're changing
> the semantics of two HiPE runtime primitives, "justified" by the observation
> that HiPE/x86 works also with the changed semantics.  However:
>
> - these two primitives are implemented separately for each HiPE backend,
>    and there are currently 6 of those
> - there is only one high-level loader, and it expects these primitives
>    to have essentially the same semantics for every backend
> - you have suggested to change x86, but haven't considered the other
>    5 backends; of those, at least 3 (ppc, ppc64, sparc) will need
>    non-trivial coding to implement += rather than = semantics
>
> I would be much happier if you'd initially just store 0 in those imm32
> fields, put label and offset in your relocation entries, and have your
> loader perform the add and then call the runtime to "=" the imm32s.
>
> If your change to the x86 version of these primitives is accepted,
> then we have to document that (a) x86 differs from the other backends,
> and (b) the initial value of a patchable immmediate MUST be zero.
Firstly, let me confess that I'd love to be able to solve this problem by
passing a flag to the compiler (llc or gcc) that says: "please use the RELA
relocations scheme". This would be by far the easiest and clearest solution of
all! :-) However,...

I 've been searching the web for hours to verify that this is a reasonable
demand, i.e. a compiler should have a flag that sets the relocation model to
something more than "static", "pic" or "dynamic-no-pic", and I couldn't find
anything! Thus, I've been wondering: "WHY, dear god, can't I find this anywhere?
Am I the first one to need that?". The clearest explanation that I've found [1] is
that on x86 the ELF ABI *only* uses REL for relocations:

 > "REL relocations thus take 2 words of storage, and RELA relocations, 3. On
 > x86, the ELF ABI only uses REL relocations, and words are 32 bits, making
 > each relocation take 8 bytes ; on x86-64, the ELF ABI only uses RELA
 > relocations, and words are 64 bits, thus making each relocation take 24
 > bytes, 3 times that of x86 relocations. On ARM, both may be used, but
 > apparently the compiler and the linker only use REL relocations."

All things considered, I believe that the most clear solution would be to
change the loader wherever needed. IMO, there's no reason to modify the other
backends; documenting this behavior would be more than enough. Nevertheless,
If you think that those changes increase the code complexity of the backend,
I _might_ be able to hack a way around this by extracting the "addend" from the
code segment and turning our "REL"s to "RELA"s, just before I feed them to
hipe_unified_loader. The reason I say "I might" is that I 'm not 100% sure that 
I will
be able to modify the "value" that will be patched by the loader in such a way that
the addendum is effectively added, without changing the format of this value or
adding an extra parameter here-and-there.


~Y.

[1]: http://glandium.org/blog/?m=201011

-- 
Yiannis Tsiouris
Ph.D. student,
Software Engineering Laboratory,
National Technical University of Athens
WWW: http://www.softlab.ntua.gr/~gtsiour

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://erlang.org/pipermail/erlang-bugs/attachments/20140204/0392d0ec/attachment.htm>