Re: [PATCH v2 10/22] sim/erc32: Switched emulated memory to host endian order.

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]

On 19 Feb 2015 23:31, Jiri Gaisler wrote:
> +	    *((unsigned short *) &(mem[waddr])) = *data & 0x0ffff;

this violates strict aliasing.  you can't cast the RHS side like this.  it also 
violates alignment since the buffer is passed in as unsigned char *.

you should use memcpy() instead.  on systems where unaligned access are OK, gcc 
should optimize it down to a few load/stores anyways.

this form shows up a few times in this patch and they should all get fixed.

> --- a/sim/erc32/exec.c
> +++ b/sim/erc32/exec.c
>  
> +static int
> +extract_short(uint32 data, uint32 address)

space before the (.  comes up multiple times in this patch.

> +    return((data>>((2 - (address & 2))*8)) & 0xffff);

need to fix the spacing here:
	return ((data >> ((2 - (address & 2)) * 8)) & 0xffff);

comes up multiple times in this patch.

> @@ -808,21 +808,28 @@ disp_mem(addr, len)
>  
>      uint32          i;
>      unsigned char   data[4];
> +    uint32          *wdata = (uint32 *) data;

you need to use a union here to avoid aliasing/alignment problems:
	union {
		unsigned char u8[4];
		uint32 u32;
	} data;

>  		while (section_size > 0) {
> -		    char            buffer[1024];
>  		    int             count;
> +		    char            buffer[1024];
> +		    uint32          *wbuffer = (uint32 *) buffer;

same here

> +      for (i=0; i<length; i+=4) {

need spaces around the operators here.  comes up a few times.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]