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

[Mikael Pettersson]

Yiannis Tsiouris writes:
 > 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:

Note that it's ultimately the assembler not GCC that creates the relocations.

I infer from the above that you're not using your own assembler, but instead
are relying on GNU binutils or something like that to produce the object code.
How lazy :-)

 >  > "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.

If your loader has an <address-of-imm32, symbol> reloc entry, then it could load
address-of-imm32 into addend and send <address-of-imm32, value(symbol)+addend>
to the existing loader BIFs.

/Mikael