SirWumpus / post4

Post4 is an indirect threaded Forth dialect written in C.
Other
4 stars 1 forks source link

Uninitialized values, etc. #5

Closed wboeke closed 1 month ago

wboeke commented 1 month ago

In post4, sometimes a word that runs perfectly in isolation, will misbehave if it is part of a bigger program. In this case you should run it with linux tool valgrind. I did this with several post4 programs, and indeed a lot of issues appeared. Many uninitialized values were reported, and out-of-band pointers.

I tried to find the cause of some of the reported issues but I came not very far. I made an own, simplified version of post4 which is valgrind-proof, so that's not impossible. Maybe you could have a look at this?

SirWumpus commented 1 month ago

More detaild are required:

Linux is not my main development platform and NetBSD does not have a port of valgrind. Before chasing this, I need more specifics.

Also I recently enable GitHub's CodeQL, which I suspect can do more than point out compiler like issues, which I already get from gcc depending on -W flags (maybe I'll try clang too).

SirWumpus commented 1 month ago

Also is any of this tied to the full build with Java support? Does ./configure --without-java eliminate /isolate the issues?

Sidebar: It is now possible to ./configure --enable-exception-strings for more detailed errors (saves having to lookup the exception code).

wboeke commented 1 month ago

Hello, My simplified version: I did not document my modifications to your original code. I omitted what I didn not need, and reduced the LOC. It's now about 1300 lines. More or less by accident I replaced in your post4.c file in line 1168 the malloc by a calloc, because I did this in my version. And voila: all valgrind issues were gone! Post4 is one of my favorite forth's again!

SirWumpus commented 1 month ago

OK. Well I can certainly change the malloc to calloc in p4Create() context memory.

I'll spinning up a Linux VM and run the test suite with valgrind, see what happens.

Post4 is one of my favorite forth's again!

Out of curiosity what do you like about Post4 compared to other implementations?

wboeke commented 1 month ago

What I like in post4:

  1. The computed goto's, easier to use then the usual void functions which need a lot of global variables.
  2. Nearly all interesting forth words have been implemented.
  3. The C code is showing that it was written with great care.

Honestly, I did'nt like the many choices that you can make in config.h, but maybe they are useful for other people.

SirWumpus commented 1 month ago

The computed goto's, easier to use then the usual void functions, needing a lot of global variables.

There should be no globals, everything should be in P4_Ctx. The idea being is you could have multiple Forth interpreters being run by the same parent C program (not tested, but that was a goal). This also allowed for integration into a Java application. Of course the opposite is also supported, you can make Java a slave to a Forth driven program 😁 (though it might need more work), thanks to suggestions by @ruv.

Nearly all interesting forth words have been implemented.

Is there something you would like to see added? (Create separate Issues for tracking.)

The C code is showing that it was written with great care.

Thank you. Nice to hear. (I'll be smiling all day.)

Honestly, I did'nt like the many choices that you can make in config.h, but maybe they are useful for other people.

Well config.h is generated from config.h.in.in by ./configure. So most of the choices there are just checking for C and POSIX support under the control of ./configure (if not I was just lazy in adding the --enable-something). Actual options might be...

/* #undef NDEBUG */

/* #undef P4_BLOCK_SIZE */
/* #undef P4_STACK_SIZE */
/* #undef P4_STRING_SIZE */
/* #undef P4_SAFE_PATH */
#define P4_FILE_ACCESS
/* #undef P4_TRACE */

#define WITH_JAVA "yes"
#define HAVE_SEE 1
#define HAVE_HOOKS 1
#define USE_STACK_CHECKS 1
#define USE_EXCEPTION_STRINGS 1

Otherwise you can overrides for defined defaults found at the top of post4.h, eg. cd src; CLFLAGS='-DP4_MEM_SIZE=64' make build.

Also there are command-line options for some of the more frequent stuff like stacks.

SirWumpus commented 1 month ago

When testing with valgrind did you specify any options of note?

wboeke commented 1 month ago

I used one extra option for valgrind: valgrind --track-origins=yes ./post4 tst.f Then you get extra information in valgrind's output.

SirWumpus commented 1 month ago

I ran the Post4 test suite through Valgrind (on Rocky Linux) and only hit two possible errors (actual tests for failure concerning ALLOCATE and RESIZE). This was without the change in P4Create to use calloc.

roche$ valgrind --track-origins=yes -s ./post4 ../test/units.p4
...
Total Pass 1336 Fail 0 Skip 0 
JNI support disabled. 
Total Pass 0 Fail 0 Skip 0 
==41258== 
==41258== HEAP SUMMARY:
==41258==     in use at exit: 0 bytes in 0 blocks
==41258==   total heap usage: 1,818 allocs, 1,818 frees, 332,801 bytes allocated
==41258== 
==41258== All heap blocks were freed -- no leaks are possible
==41258== 
==41258== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
==41258== 
==41258== 1 errors in context 1 of 2:
==41258== Argument 'size' of function realloc has a fishy (possibly negative) value: -1
==41258==    at 0x484C184: realloc (vg_replace_malloc.c:1690)
==41258==    by 0x405F0C: p4Repl (post4.c:2074)
==41258==    by 0x408DA5: p4EvalFp (post4.c:2918)
==41258==    by 0x408E3D: p4EvalFile (post4.c:2931)
==41258==    by 0x402A0A: p4LoadFile (post4.c:238)
==41258==    by 0x406DEE: p4Repl (post4.c:2350)
==41258==    by 0x408DA5: p4EvalFp (post4.c:2918)
==41258==    by 0x408E3D: p4EvalFile (post4.c:2931)
==41258==    by 0x4092AE: main (post4.c:3081)
==41258== 
==41258== 
==41258== 1 errors in context 2 of 2:
==41258== Argument 'size' of function malloc has a fishy (possibly negative) value: -1
==41258==    at 0x484480F: malloc (vg_replace_malloc.c:442)
==41258==    by 0x405E1A: p4Repl (post4.c:2059)
==41258==    by 0x408DA5: p4EvalFp (post4.c:2918)
==41258==    by 0x408E3D: p4EvalFile (post4.c:2931)
==41258==    by 0x402A0A: p4LoadFile (post4.c:238)
==41258==    by 0x406DEE: p4Repl (post4.c:2350)
==41258==    by 0x408DA5: p4EvalFp (post4.c:2918)
==41258==    by 0x408E3D: p4EvalFile (post4.c:2931)
==41258==    by 0x4092AE: main (post4.c:3081)
==41258== 
==41258== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

Did you have a specific (short) program that generated more interesting results? If so can you share it here?

wboeke commented 1 month ago

Like you, I can't understand this output. Maybe it is harmless. With my forth test program, a few 100's LOC, I got countless valgrind issues, which vanished after the malloc->calloc modification. Also other programs that in the past crashed or gave false output, now behave like they should. It's not easy, to understand everything.

SirWumpus commented 1 month ago

Another variant to try where Valgrind is concerned, is instead of clearing (zero) memory is to set to some other value, the uninitialised values is resolved for Valgrind, but then you get to see if the program is accessing data that hasn't been set. Sometimes the zero memory values are safe and hide a deep issue.

MEMSET(ctx->mem, BYTE_ME, opts->mem_size * 1024);
SirWumpus commented 1 month ago

Right now, I'm running Valgrind against life.p4 and life1d.p4 to see if I get more reports.

SirWumpus commented 1 month ago

I've run post4 test suite, life.p4 (abridged), life1d.p4 through Valgrind on Rocky Linux v9.4 XFCE Desktop. Addressed the two reported issues WRT expected-to-fail test cases. Also one off-by-one bug in the life1d.p4 example (not the C code).

I have not been able to generate any uninitialised value errors from Valgrind (maybe a difference in versions?), however I've applied the requested calloc() change as it poses no obvious issue; have added the MEMSET mentioned above when --enable-debug for future testing efforts.

Without more details, example failure cases or what you pruned from the C source (as a pointer where to look in-depth), I am closing this issue.