Re: [PATCH v3 03/14] sim/erc32: Switched emulated memory to host endian order.

[Jiri Gaisler <jiri at gaisler dot se>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 17/03/15 09:06, Mike Frysinger wrote:
> On 14 Mar 2015 21:44, Jiri Gaisler wrote:
>> On 14/03/15 11:23, Mike Frysinger wrote:
>>> On 14 Mar 2015 10:23, Jiri Gaisler wrote:
>>>> On 14/03/15 08:45, Mike Frysinger wrote:
>>>>> On 13 Mar 2015 09:24, Jiri Gaisler wrote:
>>>>>> On 13/03/15 00:55, Mike Frysinger wrote:
>>>>>>> On 12 Mar 2015 22:25, Jiri Gaisler wrote:
>>>>>>>> On 02/03/15 02:13, Mike Frysinger wrote:
>>>>>>>>>> +#ifdef HOST_LITTLE_ENDIAN
>>>>>>>>>>> +		    for (i = 0; i < (count / 4); i++) wbuffer[i] = ntohl(wbuffer[i]); // endian swap +#endif
>>>>>>>>> 
>>>>>>>>> sim-endian.h already provides a lot of helper funcs that i'm pretty sure you can use here.
>>>>>>>> 
>>>>>>>> I don't understand why ntohl() is a problem. It is a common Posix function that converts big endian to host endian, exactly what is needed. Using sim-endian.h pulls in a lot of the sim-*.c files due to dependencies and makes the simulator larger than necessary
>>>>>>>> ....
>>>>>>> 
>>>>>>> "network" has no meaning here.  using it as a proxy for moving between big endian and native endian when there are clear functions that the sim has standardized on isn't correct.  your code also (1) requires duplicating branches and (2) inline preprocessor
>>>>>>> checks.  it also does not properly handle bi-endian builds.  sim-endian does all of these for you.  the whole point of common/ is to delete code from each sim rather than open coding it everywhere.
>>>>>>> 
>>>>>>> wrt size, i don't think that's a compelling argument.  we're talking units of KiB here, and i can't even count that low :P.
>>>>>>> 
>>>>>>> if you're having trouble converting the build over (compiling/linking errors), then we can discuss that.  but it'd be a matter of "do we do it now or later" rather than "do we do ever convert".
>>>>>> 
>>>>>> Right. I tried to use the T2H_4 macro, but can't get it to compile. I included <sim-basics.h> and added sim-endian.o and sim-io.o to the Makefile, but it complains about unresolved function etc. Do I really need to create a sim-main.c and sim-main.h just to use
>>>>>> T2H?
>>>>> 
>>>>> i've pushed a patch to bury the sim-io.h include in sim-assert.h (since that's the header that actually uses the sim_io_xxx funcs).  if you define your own ASSERT/SIM_ASSERT macros, it should be avoided for now.  just make sure you add a note that they should get
>>>>> converted to sim-assert.h at some point.
>>>> 
>>>> I'm not sure this helps. sim-endian.c includes sim-assert.h, so I get the same problem even after your patch:
>>>> 
>>>> gcc -DHAVE_CONFIG_H     -DPROFILE=1 -DWITH_PROFILE=-1         -DWITH_HOST_BYTE_ORDER=LITTLE_ENDIAN -DDEFAULT_INLINE=0           -DFAST_UART -I../../../../../ibm/src/gdb/binutils-gdb/sim/erc32/../..   -I. -I../../../../../ibm/src/gdb/binutils-gdb/sim/erc32 -I../common 
>>>> -I../../../../../ibm/src/gdb/binutils-gdb/sim/erc32/../common -I../../include -I../../../../../ibm/src/gdb/binutils-gdb/sim/erc32/../../include -I../../bfd -I../../../../../ibm/src/gdb/binutils-gdb/sim/erc32/../../bfd -I../../opcodes 
>>>> -I../../../../../ibm/src/gdb/binutils-gdb/sim/erc32/../../opcodes  -g -O2 -static-libstdc++ -static-libgcc  -o run \ run.o libsim.a ../../bfd/libbfd.a ../../opcodes/libopcodes.a  ../../libiberty/libiberty.a -ltermcap -ldl -lz -lnsl  ../../readline/libreadline.a
>>>> -ltermcap -lm libsim.a(sim-endian.o): In function `offset_1': /home/jiri/src/gdb/v4/sim/erc32/../../../../../ibm/src/gdb/binutils-gdb/sim/erc32/../common/sim-n-endian.h:145: undefined reference to `sim_io_error'
>>> 
>>> did you define ASSERT/SIM_ASSERT before including sim-endian.h ?
>> 
>> No, I included sim-basic.h in my code. Including sim-endian.h only will not work due to dependencies on other include files. I don't see how this will change how sim-endian.o is built though, as it is compiled separately.
>> 
>> I did manage to compile the code by including sim-endian.c directly into my own code (func.c) rather then building it separately:
>> 
>> #include <sim-assert.h> #undef  ASSERT #define ASSERT(x) if (0) {} #include <sim-endian.c>
>> 
>> Is this acceptable ...?
> 
> lets go with your first patch with some /* TODO */ added in these areas.  we need to some more clean up in this sim and lay some basic ground work before we can have it start using common/.  i didn't realize just how disconnected erc32 was from everything else. -mike

v4 of this patch uses sim-endian.c and everything compiles fine.
Should I revert to not using sim-endian.c and post a v5 of the patches?

Jiri.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJVDddLAAoJEIAIwBKmO2+bOjUQALoZ/PXNgnYal4UXOndq6C5l
5onSEB4jTU72eK1aDMWSQPlcTLwT2uuG9sqp1yMcoL1sZY7rGDawaFd8SzP5WeV+
1vD6v7buhakLWzR5wn8O1yDD2+hHyKOZdMpMTzND3MVaf4KIHs4EVPaCg/6Um2qW
Co2e9xP0cgj+vRhg2BWY9bL3PvpBGy/pa0/KWqxZd7SsYuh3vWFnFq2htZTlPgi5
jFKlJN2t4rQ67fZOaicD5rNutmNbmaTaakfOReA4d/UvEXR/dTOqReEkQaUAT4MM
YxncGa+ecuVApQ5qIPZaZQPZHXNbTQb6mfOvpIR+yXnYA+6S2VKWf3DGBzg5K8eS
APz9DEj4677tmtWRjs/PkFQpCyAX34s1pEscMWnJJQgQXY17IpRwmcXZx+QlKMdQ
peOmS2YmjYxjAApXGZ9gRa4yRG5sW09SDSjj0mXaH/2lD0u3C76Aqp1n/jTPauVk
/TdmWhoVRdVhLQ0QZ9uU1NSql84taDeGfvJSGE/MQxSMAulj0GbAHDlph1EiCIvu
5WPmII6ibCX2B5uVvgSJN4fyxnmunzcDEdUH/7L7IljIfI9DjyXp48dDTkmFn7mX
wggEjAULUxHIss8/ldGEPvdJyU8+jQVzusi9xNMTS3qo7RX6fBMGo9FRyvcY38b/
t1GBKmiDO8IT72/uEtgI
=vYan
-----END PGP SIGNATURE-----