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

[Mikael Pettersson]

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

The existing code works, yes, but it does not read or depend on the old
value of *address in any way.  You make it sound as if the old value of
*address being zero is somehow significant; it is not.

 > 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

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.

/Mikael