Incorrect results under Keil C51

[TianpeiLee]

hi ,

I am trying to port it to 8051, but the calculation output is always incorrect. Can anyone provide a 51 project that can run normally, or give me some advice?

I have changed the definition of data types, and the conflicts between some variables and C51 keywords.

[spth]

The bug here is that Coremark requires sizeof(ee_ptr_int) == sizeof(ee_u8 *); instead it should require sizeof(ee_ptr_int) >= sizeof(ee_u8 *).

Fix: change in core_portme.c:

if (sizeof(ee_ptr_int) != sizeof(ee_u8 *)) {
		ee_printf("ERROR! Please define ee_ptr_int to a type that holds a pointer!\n");
	}

to

if (sizeof(ee_ptr_int) < sizeof(ee_u8 *)) {
		ee_printf("ERROR! Please define ee_ptr_int to a type that holds a pointer!\n");
	}

Since compilers targeting the MCS-51 tend to have uintptr_t as 32 bits, but void * as 24 bits, you'll run into this bug there (I don't use Keil, but ran into the same problem using SDCC).

[joseph-yiu]

Thanks for the details. I will look into this in a couple of weeks time.
Joseph

[joseph-yiu]

Hi @TianpeiLee ,
Could you let us know whether the fix suggested by @spth works for you?
Thanks and regards,
Joseph

[joseph-yiu]

I tried SDCC for testing the change but the compiler feature support seems very outdated. For example:

• Use of PRIu32 format specifier introduced in Use PRIu32 format specifier when printing time_in_secs #76 is not supported.
• inttypes.h and size_t is not supported
  (I have added --std-c11 option already).
  I can still include the fix proposed by @spth in the next CoreMark maintenance update, but for now I won't be able to test the change for MCS51 architecture (e.g. 8051, 8052). I have tested the change in Cortex-M (gcc, clang, Arm compiler 6), Raspberry Pi 5 and x86.

[spth]

Which SDCC version did you use? The current release is 4.6.0.
While SDCC was indeed late adding inttypes.h, support for it has been there for about three years, and size_t has been supported for as long as I can remember (i.e. more than 25 years). printf still lacking features wouldn't surprise me, though.

[joseph-yiu]

I need to double check the error message and version number when I get home later. (Possibly an oversight on my part).

[joseph-yiu]

The sdcc version on my ubuntu desktop (x86) is 4.2.0. I installed this using sudo apt install sdcc.
Moving to 4.6.0 seems better, but I get this error:

sdcc   -c core_portme.c \
       -I coremark -I . \
       -DITERATIONS=1000 -DSTANDALONE -DPERFORMANCE_RUN=1 --std-c11 --model-medium
./core_portme.h:94: error 1: Syntax error, declaration ignored at 'size_t'
./core_portme.h:94: syntax error: token -> 'ee_size_t' ; column 24
subprocess error 512
make: *** [makefile:29: core_portme.rel] Error 1

Line 94 in core_portme.h is:
typedef size_t ee_size_t;

A workaround by changing that line to
#define ee_size_t size_t

Now getting to other issues I am a bit lost (sorry, I am new to sdcc):

sdcc   -c coremark/core_list_join.c \
       -I coremark -I . \
       -DITERATIONS=1000 -DSTANDALONE -DPERFORMANCE_RUN=1 --std-c11 --model-medium
coremark/core_list_join.c:206: error 78: incompatible types
from type 'int function   ( struct list_data_s generic* fixed, struct list_data_s generic* __pdata, struct RESULTS_S generic* __pdata) fixed'
  to type 'int function   ( struct list_data_s generic* fixed, struct list_data_s generic* fixed, struct RESULTS_S generic* fixed) __code* fixed'
coremark/core_list_join.c:223: error 78: incompatible types
from type 'int function   ( struct list_data_s generic* fixed, struct list_data_s generic* __pdata, struct RESULTS_S generic* __pdata) fixed'
  to type 'int function   ( struct list_data_s generic* fixed, struct list_data_s generic* fixed, struct RESULTS_S generic* fixed) __code* fixed'
coremark/core_list_join.c:306: error 78: incompatible types
from type 'int function   ( struct list_data_s generic* fixed, struct list_data_s generic* __pdata, struct RESULTS_S generic* __pdata) fixed'
  to type 'int function   ( struct list_data_s generic* fixed, struct list_data_s generic* fixed, struct RESULTS_S generic* fixed) __code* fixed'
coremark/core_list_join.c:551: error 92: Functions called via pointers must be 'reentrant' to take this many (bytes for) arguments
-:0: warning 85: in function core_list_mergesort unreferenced function parameter : 'res'
make: *** [makefile:9: core_list_join.rel] Error 1

[spth]

Wait, are you writing your core_portme.h based on the template from barebones/core_portme.h?
In that case there is a size_t problem, though not an SDCC-specific one: neither barebones/core_portme.h nor barebones/core_portme.c include any C standard library header that provides size_t, so it is indeed not available at that point. IMO, there should be a comment about that on the line that uses size_t in barebones/core_portme.h (stating that the user would have to either include such a header, or use a type different from size_t there).

P.S.: There are some examples of using Coremark with SDCC (though not for MCS-51), since the SDCC project uses some benchmarks to track code size and performance, which for STM8, Z80, Rabbit 3000A, Rabbit 4000 that includes Coremark. For STM8 since SDCC 3.5.0, released 11 years ago. The resulting code size / score graphs, and the patches applied to Coremark can be found in the SDCC repo at https://sourceforge.net/p/sdcc/code/HEAD/tree/trunk/sdcc-extra/historygraphs/

[joseph-yiu]

Wait, are you writing your core_portme.h based on the template from barebones/core_portme.h?

Yes

neither barebones/core_portme.h nor barebones/core_portme.c include any C standard library header that provides size_t

Ooops! Good catch, thanks.
Both posix/core_portme.c and simple/core_portme.c has stdlib.h, hence there is no problem there.
I will add stddef.h in barebones/core_portme.h.

[HepoH3]

I will add stddef.h in barebones/core_portme.h

If I understand correctly, the missing #include <stddef.h> might be related to CoreMark's goal of supporting compilers that do not fully comply with the ISO C standard, requiring manual adjustments for such environments. From the ISO C perspective, including stddef.h seems to be the correct fix.

[joseph-yiu]

Interesting :)
The file core_portme.c and core_portme.h are modifiable, so people can change them if then need to use a compiler that don't have stddef.h.

[HepoH3]

Interesting :) The file core_portme.c and core_portme.h are modifiable, so people can change them if then need to use a compiler that don't have stddef.h.

Yup, I agree. I just wanted to provide some context from the previous discussion in case it was relevant.

[joseph-yiu]

I have managed to compile CoreMark with SDCC but it doesn't seems to work. I guess it is a stack overflow issue as the stack is too small.
(https://github.com/joseph-yiu/coremark_mcs51)
I will see if I could get hold of a Keil C51 eval license to test this.

Update: The eval version of C51 is limited to 2KB of object code. So this is not going to work.