Repository navigation
Incorrect results under Keil C51 #71
Description
Activity
The bug here is that Coremark requires
sizeof(ee_ptr_int) == sizeof(ee_u8 *); instead it should requiresizeof(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).
Thanks for the details. I will look into this in a couple of weeks time.
JosephHi @TianpeiLee ,
Could you let us know whether the fix suggested by @spth works for you?
Thanks and regards,
JosephI 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.
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.I need to double check the error message and version number when I get home later. (Possibly an oversight on my part).
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 1Line 94 in core_portme.h is:
typedef size_t ee_size_t;A workaround by changing that line to
#define ee_size_t size_tNow 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 1Wait, 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/
Reacted by Joseph YiuWait, 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.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, includingstddef.hseems to be the correct fix.Reacted by Joseph YiuInteresting :)
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.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.
Reacted by Joseph YiuI 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.
The friendly folks in Keil have provided me with a C51 license so I can continue the investigation.
Now I have made a bit more progress, but still got ECC errors to solve out. (Tested in simulator only).2K performance run parameters for coremark.
[0]ERROR! list crc 0x402e - should be 0xe714
[0]ERROR! matrix crc 0x0000 - should be 0x1fd7
[0]ERROR! state crc 0x0000 - should be 0x8e3a
...There are a few issues:
-
C51 does not have inttypes.h
This means the fix for ee_u32 accessed with %d format specifier #35
(Use PRIu32 format specifier when printing time_in_secs)
would not work. So for Keil C51 I need to use casting instead. -
check_data_types in core_util.c
This function also contains a comparison (sizeof(ee_ptr_int) != sizeof(int *))
This can be fixed easily. -
cmp_complex function needs to be declared as reentrant
I saw the same issue in SDCC. -
Use of size in typedef struct RESULTS_S
Size is a reserved keyword in Keil C51. I need to rename this. -
calc_func function in core_list_join.c use pdata and data as parameters
pdata and data are reserved keywords in Keil C51. I need to rename them to get it to work. -
crcu8 function in core_util.c use data as parameter.
data is a reserved keyword in Keil C51. I need to rename it to get it to work.
-
Updates:
With the code changes mentioned previously and some more changes, I have got CoreMark working (CRC error fixed) in Keil C51. There are some adjustments on stacks and I need to add reentrant attribute to another function in core_list_join.c.
For reference, those codes are in https://github.com/joseph-yiu/coremark_mcs51
I have only tested it in simulator as I don't have any suitable hardware for testing the MCS51 port.I have also made some unsuccessful attempts to port it to SDCC. Although I can get the project to compile, the program is non-functional, seems to be a stack size limitation. For SDCC, although the compiler has an option to put stack in external SRAM, the out-of-box toolchain does not include suitable runtime library for that so I decided not to spend more effort on that.
Anyway, for the error message reported in this issue, it is now resolved and tested in my test setup and I will look into merging the fixes in the development branch.
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.