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

[Mikael Pettersson]

Yiannis Tsiouris writes:
 > Hey,
 > 
 > While trying to locate a bug in our x86 backend that caused a segfault at 
 > load-time, we came at something that we think is a bug in the final address 
 > calculation. In functions hipe_patch_load_fe and hipe_patch_insn in 
 > erts/emulator/hipe/hipe_x86.c file we think that the final address should always 
 > be calculated as the sum of "address" and "value". We base this on our 
 > observation that "address" seems to be the offset and "value" seems to be the 
 > base address.
 > 
 > The patch that works for us is attached.
 > 
 > If any developer can confirm that this is a bug (and that the patch is the 
 > correct way to fix it) I can submit it properly (if needed). :-)
 > 
 > Thanks,
 > Yiannis (as member of the ErLLVM team)
 > 
 > -- 
 > Yiannis Tsiouris
 > Ph.D. student,
 > Software Engineering Laboratory,
 > National Technical University of Athens
 > WWW: http://www.softlab.ntua.gr/~gtsiour
 > 
 > 
 > ----------------------------------------------------------------------
 > 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.

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.

/Mikael