zsaleeba / picoc

A very small C interpreter
1.45k stars 182 forks source link

Test 63_typedef fails #6

Open e8johan opened 9 years ago

e8johan commented 9 years ago

When running the tests, the typedef test case fails.

$make test
...
Test: 63_typedef...
error in test 63_typedef
--- 63_typedef.expect   2015-06-02 08:03:21.363460115 +0200
+++ 63_typedef.output   2015-06-02 08:25:12.031715146 +0200
@@ -1,11 +1,11 @@
 -104<1>
 -17768<2>
 -19088744<4>
--1250999861249<8>
+-1164378113<4>
 152<1>
 47768<2>
 4275878552<4>
-280223976849407<8>
+3130589183<4>
 -19088744
 (1, 3)
 (1, 2)
Makefile:70: recipe for target '63_typedef.test' failed

I'm running the test on an i686 installation.

$ uname -a
Linux pelagic-joth 3.19.0-18-generic #18-Ubuntu SMP Tue May 19 18:30:59 UTC 2015 i686 i686 i686 GNU/Linux
hellerve commented 9 years ago

This is related to the size of int64/uint64 on your machine. The number within angle brackets designates the size of the number. It is expected to be 8 bytes, but is 4 bytes in size.

The tests expects a 64bit version OS, I guess. unsigned long is declared to be uint64_t at the top of the test, which does not hold true for every architecture.

Bottom line: The test is not failing because the software is not correct, but because of false internal premises.

Disclaimer: I am not affiliated with this software in any way, I just found your issue and thought I might clear things up a bit.

e8johan commented 9 years ago

Yup, definitely something broken with the 64bit types. I just assumed that the interpreter would execute in a run-time that was independent of sizeof(unsigned long) for the host architecture. I don't have time to dig deeper here right now, but I guess the type definitions for the interpreter run-time, and not necessarily for the host platform, is the issue.

hellerve commented 9 years ago

This is actually standard-conformant behaviour. unsigned long is not guaranteed to be 64 bits in any way. If you just typedef uint64_t as that, you will have a bad time.

e8johan commented 9 years ago

Yup, the test is broken. I had a look in the code, and it seems that TypeUnsignedLong is defined in interpreter.h. Then in type.c, TypeSizeValue, an assumption is made that sizeof(unsigned long) <= sizeof(ALIGN_TYPE), which happens to be void * (defined in platform.h). Thus, it is broken.

I did a fugly fix to the test (using sizeof(x64) and sizeof(u64) to fake the output on non-64-bit platform). I'm preparing the pull request right now.

e8johan commented 9 years ago

This is the fix mentioned in the previous comment: https://github.com/zsaleeba/picoc/pull/8 .

galacticstudios commented 9 years ago

I have a fork that fixes this problem (https://github.com/galacticstudios/picoc/commit/abd747392f669314de6bee5fcbe2bda593ade782). It implements picoc's 'long' as an int64_t, so that picoc's long will be 64 bits on every platform. I've tested on Windows (MS Visual C++ 2013 or newer) and on Linux. I've also fixed a bunch of compiler warnings on MSVC.

cgjordan commented 8 years ago

Anyone know if/when these fixes will make it into master? I ran into this issue today on my Raspberry Pi 3 B, running 32-bit Raspbian 8.

root@raspberrypi:~# uname -r -m -o
4.4.13-v7+ armv7l GNU/Linux
zsaleeba commented 8 years ago

Hey galacticstudios, want to send me a pull request with your fixes?

galacticstudios commented 8 years ago

I've sent a pull request - I think. I haven't done it before so let me know if I did it wrong.