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

Mike Frysinger
2015-02-22T20:51:37Z

[-- 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 --]