On Jun 27, 2006, at 8:50 AM, Jason Thorpe wrote:

>
> On Jun 27, 2006, at 6:02 AM, Simon Burge wrote:
>
>> ld.elf_so is having alignment issues with mips and gcc4 C++ programs.
>> The core of the problem is that we have a function store_ptr()  
>> which is
>>
>> 	static inline void
>> 	store_ptr(void *where, Elf_Addr val)
>> 	{
>> 		memcpy(where, &val, sizeof(val));
>> 	}
>>
>> The idea of this is that we sometimes want to store a Elf_Addr
>> (uint32_t) at an unaligned address, so we use memcpy to write the  
>> data.
>> This was added for gcc3 which started using aligned data for some GOT
>> info.
>
> I think if you do this:
>
> static inline void
> store_ptr(void *where, Elf_Addr val)
> {
> 	void *from = &val;

Oops, make that:

	const void *from = &val;

:-)

>
> 	memcpy(where, from, sizeof(val));
> }
>
> that it will work.  And that is a lot less invasive than your  
> proposed change.
>
>
>>
>> The code that calls this does:
>>
>> 	Elf_Addr *where;
>>
>> 	where = ....
>> 	store_ptr(where, value);
>>
>> With gcc3 this generated code like:
>>
>>         swl     $2,3($8)
>>         swr     $2,0($8)
>>
>> and with gcc4 this generates:
>>
>>         sw      $2,0($9)
>>
>> That is, with gcc4 it's an aligned store instead of an unaligned  
>> store.
>>
>> Just adding casts at various places didn't help, both at the caller
>> and inside store_ptr() didn't help.  The patch below works around  
>> this
>> by starting with a void * variable, which obviously doesn't have any
>> alignment constraints.
>>
>> Now, the question is "is what gcc4 is doing right?".  Since we  
>> started
>> with a pointer to Elf_Addr, is the optimiser legal in saying that  
>> that
>> pointer is always aligned so it an optimise the store in to an  
>> aligned
>> store?  Or is it a bug with gcc4 and memcpy and assuming alignment?
>>
>> Cheers,
>> Simon.
>> --
>>
>> Index: libexec/ld.elf_so/arch/mips/mips_reloc.c
>> ===================================================================
>> RCS file: /cvsroot/src/libexec/ld.elf_so/arch/mips/mips_reloc.c,v
>> retrieving revision 1.51
>> diff -d -p -u -r1.51 mips_reloc.c
>> --- libexec/ld.elf_so/arch/mips/mips_reloc.c	3 Apr 2006 13:23:15  
>> -0000	1.51
>> +++ libexec/ld.elf_so/arch/mips/mips_reloc.c	27 Jun 2006 05:20:55  
>> -0000
>> @@ -86,6 +86,7 @@ _rtld_relocate_nonplt_self(Elf_Dyn *dynp
>>  {
>>  	const Elf_Rel *rel = 0, *rellim;
>>  	Elf_Addr relsz = 0;
>> +	void *wherev;
>>  	Elf_Addr *where;
>>  	const Elf_Sym *symtab = NULL, *sym;
>>  	Elf_Addr *got = NULL;
>> @@ -133,7 +134,8 @@ _rtld_relocate_nonplt_self(Elf_Dyn *dynp
>>
>>  	rellim = (const Elf_Rel *)((caddr_t)rel + relsz);
>>  	for (; rel < rellim; rel++) {
>> -		where = (Elf_Addr *)(relocbase + rel->r_offset);
>> +		wherev = (void *)(relocbase + rel->r_offset);
>> +		where = wherev;
>>
>>  		switch (ELF_R_TYPE(rel->r_info)) {
>>  		case R_TYPE(NONE):
>> @@ -146,7 +148,7 @@ _rtld_relocate_nonplt_self(Elf_Dyn *dynp
>>  			if (__predict_true(RELOC_ALIGNED_P(where)))
>>  				*where += relocbase;
>>  			else
>> -				store_ptr(where, load_ptr(where) + relocbase);
>> +				store_ptr(wherev, load_ptr(wherev) + relocbase);
>>  			break;
>>
>>  		default:
>> @@ -246,9 +248,12 @@ _rtld_relocate_nonplt_objects(const Obj_
>>  	got = obj->pltgot;
>>  	for (rel = obj->rel; rel < obj->rellim; rel++) {
>>  		Elf_Addr        *where, tmp;
>> +		void		*wherev;
>>  		unsigned long	 symnum;
>>
>> -		where = (Elf_Addr *)(obj->relocbase + rel->r_offset);
>> +		/* store address in a non-alignment restricted pointer first */
>> +		wherev = obj->relocbase + rel->r_offset;
>> +		where = wherev;
>>  		symnum = ELF_R_SYM(rel->r_info);
>>
>>  		switch (ELF_R_TYPE(rel->r_info)) {
>> @@ -263,12 +268,12 @@ _rtld_relocate_nonplt_objects(const Obj_
>>  				if (__predict_true(RELOC_ALIGNED_P(where)))
>>  					tmp = *where;
>>  				else
>> -					tmp = load_ptr(where);
>> +					tmp = load_ptr(wherev);
>>  				tmp += got[obj->local_gotno + symnum - obj->gotsym];
>>  				if (__predict_true(RELOC_ALIGNED_P(where)))
>>  					*where = tmp;
>>  				else
>> -					store_ptr(where, tmp);
>> +					store_ptr(wherev, tmp);
>>
>>  				rdbg(("REL32/G %s in %s --> %p in %s",
>>  				    obj->strtab + def->st_name, obj->path,
>> @@ -293,7 +298,7 @@ _rtld_relocate_nonplt_objects(const Obj_
>>  				if (__predict_true(RELOC_ALIGNED_P(where)))
>>  					tmp = *where;
>>  				else
>> -					tmp = load_ptr(where);
>> +					tmp = load_ptr(wherev);
>>
>>  				if (def->st_info ==
>>  				    ELF_ST_INFO(STB_LOCAL, STT_SECTION)
>> @@ -307,7 +312,7 @@ _rtld_relocate_nonplt_objects(const Obj_
>>  				if (__predict_true(RELOC_ALIGNED_P(where)))
>>  					*where = tmp;
>>  				else
>> -					store_ptr(where, tmp);
>> +					store_ptr(wherev, tmp);
>>
>>  				rdbg(("REL32/L %s in %s --> %p in %s",
>>  				    obj->strtab + def->st_name, obj->path,
>> @@ -319,7 +324,7 @@ _rtld_relocate_nonplt_objects(const Obj_
>>  			rdbg(("sym = %lu, type = %lu, offset = %p, "
>>  			    "contents = %p, symbol = %s",
>>  			    symnum, (u_long)ELF_R_TYPE(rel->r_info),
>> -			    (void *)rel->r_offset, (void *)load_ptr(where),
>> +			    (void *)rel->r_offset, (void *)load_ptr(wherev),
>>  			    obj->strtab + obj->symtab[symnum].st_name));
>>  			_rtld_error("%s: Unsupported relocation type %ld "
>>  			    "in non-PLT relocations\n",
>
> -- thorpej

-- thorpej