embench / embench-iot

The main Embench repository
https://www.embench.org/
GNU General Public License v3.0
259 stars 104 forks source link

md5sum: make calloc() use calloc_beebs() and use XOR for result value #147

Closed hirooih closed 2 years ago

hirooih commented 3 years ago

Although malloc_beebs() was used for malloc(), but calloc_beebs() was not used for calloc(). As result calloc() failed. Use calloc_beebs() for calloc() and increase the heap size for it.


The result value was the sum of four uint32_t values. The result changes between 32bit and 64bit CPUs.

Using XOR (Exclusive OR) fixes the issue.

jeremybennett commented 3 years ago

@hirooih Thanks for this. We are explicit in our use or the beebs functions, we don't overload the standard functions, so users can see where we have made the changes. The correct fix is to change the call to calloc to be a call to calloc_beebs.

Can you explain more how the use of XOR makes the code more portable? We try to avoid rewriting code except to correct errors, in order to maintain a variety of coding styles in the benchmark.

BTW, the _beebs suffix is because may of the Embench benchmarks came originally from the Bristol-Embecosm Embedded Benchmark Suite (BEEBS).

Roger-Shepherd commented 3 years ago

I think there is a problem with the return line of benchmark_body. With my understanding of the promotion rules of C the expression (h0 + h1 + h2 + h3) will have type int as this is what is being returned from benchmark_body and on a 64-bit machine the hi (unsigned 32-bits) will be converted to ints and then added. There can be a problem in that the result can have more than 32-bits. As the operation is only being performed as a (crude) check the XOR solution would seem acceptable; it is not really part of the benchmark.

hirooih commented 3 years ago

We are explicit in our use or the beebs functions, we don't overload the standard functions, so users can see where we have made the changes.

I see. I thought it was better not to change the original code. I've pushed the fix.

Can you explain more how the use of XOR makes the code more portable?

benchmark_body() returns an int value. In md5sum it returns a sum of 4 uint32_t values. The expected value 0x3_0C0D_A225 is valid only on 64bit CPUs. XOR does not have the issue.

We try to avoid rewriting code except to correct errors, in order to maintain a variety of coding styles in the benchmark.

I think this is not the case. benchmark_body() is added by Embench.

hirooih commented 3 years ago

@Roger-Shepherd Thank you. You commented at the same time!

jeremybennett commented 2 years ago

@hirooih I'm sorry - I didn't get round to merging this earlier. Thanks for all your work.

jeremybennett commented 2 years ago

Now good to merge.

hirooih commented 2 years ago

@jeremybennett

Thanks!