NETMF / netmf-interpreter

.NET Micro Framework Interpreter
http://netmf.github.io/netmf-interpreter/
Other
487 stars 224 forks source link

wieredness with BIG_ENDIAN macro #481

Closed josesimoes closed 8 years ago

josesimoes commented 8 years ago

Starting with GCC 5.4.1 the BIG_ENDIAN define seems to be defined internally in the compiler. This has been reported by @cw2 in #474. I can confirm that I'm seeing this too on my builds.

I've opened a ticket with GNU ARM Embedded Toolchain. Waiting on their comments. Thinking on a solution for this, I found that CMSIS has a macro defined for this purpose (see "Pre-processor Macros" in http://www.keil.com/pack/doc/cmsis/DSP/html/index.html. It's ARM_MATH_BIG_ENDIAN. As we seem to be moving towards CMSIS and we are already using other macros from there (__FPU_PRESENT for example), suggest that the current BIG_ENDIAN in NETMF is replaced with ARM_MATH_BIG_ENDIAN.

I can submit a PR for this immediately.

smaillet-ms commented 8 years ago

Well that's a pretty crappy turn of affairs... (And yet another reason why I hate preprocessor macros...)

NETMF is not bound to CMSIS nor to ARM so we shouldn't make such assumptions. How about we go with NETMF_TARGET_BIG_ENDIAN? That makes it specific for NETMF AND implies, correctly, that it is for the target.

cw2 commented 8 years ago

Just for reference, this is the bug report https://bugs.launchpad.net/gcc-arm-embedded/+bug/1609078 (thanks @josesimoes )

josesimoes commented 8 years ago

The PR is submitted and it has been live tested in hardware with the Discovery4 solution. I had to replace the little endian too. A few comments:

Right now I don't have the time to revert the code back to a version previous to those changes and retest again. Anyways it's working now and the fix seems perfectly reasonable. So lacking a reason for going deeper into this and not having any further suggestions, I suggest that this is considered as fixed.

PS: because of the still open issue with the "end" symbol (see #474) I had to add that to my test build, although I haven't committed that change.

cw2 commented 8 years ago

Well, I have found the cause, it is due to a change in header files in GCC 5.4.1:

arm-none-eabi\include\machine\endian.h contains unconditional

define BIG_ENDIAN 4321

whilst in GCC 5.3.1 BIG_ENDIAN symbol is defined only when

#if __BSD_VISIBLE
#define LITTLE_ENDIAN   _LITTLE_ENDIAN
#define BIG_ENDIAN  _BIG_ENDIAN
#define PDP_ENDIAN  _PDP_ENDIAN
#define BYTE_ORDER  _BYTE_ORDER
#endif

Thus, the trivial repro case

#include <stdio.h>
int main(void)
{
#ifdef BIG_ENDIAN
#error BIG_ENDIAN
#endif
return 0;
}

works in GCC 5.4.1 if the #include is removed.

Initially, I was only searching through NETMF codebase, I completely forgot the standard headers. (facepalm)

smaillet-ms commented 8 years ago

Well, I think Charlie Brown said it best - "Oh, good grief!" :disappointed:

josesimoes commented 8 years ago

@cw2 nice finding! Unless there is an incredible coincidence, someone posted this exact same finding, description and test code on the ARM GCC bug report... 😉 Your secret identity has been compromised!! 😆

josesimoes commented 8 years ago

Having no answer from the ARM GCC team suggest the following:

I'm pointing this out because there are BIG_ENDIAN and LITTLE_ENDIAN macros in the LwIP source that were replaced too. It's not such a big deal but it's a third party source that is in the repo and was changed.