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

[Anthony Ramine]

Hello,

Don’t know if that is relevant, but there is already an addition of an offset and a base address in hipe_unified_loader:

https://github.com/erlang/otp/blob/eec1d22c5aef21ad4606c79465084bbff46d42ee/lib/kernel/src/hipe_unified_loader.erl#L368-369

Regards,

-- 
Anthony Ramine

Le 1 févr. 2014 à 16:46, Mikael Pettersson <mikpelinux@REDACTED> a écrit :

> 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
> _______________________________________________
> erlang-bugs mailing list
> erlang-bugs@REDACTED
> http://erlang.org/mailman/listinfo/erlang-bugs