davidgiven / ack

The Amsterdam Compiler Kit
http://tack.sf.net
Other
420 stars 59 forks source link

llgen invokes UB in a bunch of left shifts #237

Open GabrielRavier opened 2 years ago

GabrielRavier commented 2 years ago

I've been trying to compile ack with -fsanitize=undefined for a little while now and I'm getting this error from llgen:

/tmp/ack-build/obj/util/LLgen/llgen/llgen /home/gravier/src/programs/programming-tools/compilers/ack/lang/basic/src/basic.g
"/home/gravier/src/programs/programming-tools/compilers/ack/lang/basic/src/basic.g", line 65: (Warning) terminal DBLVALUE not used
"/home/gravier/src/programs/programming-tools/compilers/ack/lang/basic/src/basic.g", line 67: (Warning) terminal UNARYSYM not used
"/home/gravier/src/programs/programming-tools/compilers/ack/lang/basic/src/basic.g", line 77: (Warning) terminal BOOLOP not used
"/home/gravier/src/programs/programming-tools/compilers/ack/lang/basic/src/basic.g", line 83: (Warning) terminal LESYM not used
"/home/gravier/src/programs/programming-tools/compilers/ack/lang/basic/src/basic.g", line 84: (Warning) terminal GESYM not used
"/home/gravier/src/programs/programming-tools/compilers/ack/lang/basic/src/basic.g", line 85: (Warning) terminal NESYM not used
"/home/gravier/src/programs/programming-tools/compilers/ack/lang/basic/src/basic.g", line 86: (Warning) terminal UNARYMINUS not used
util/LLgen/src/compute.c:278:4: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
util/LLgen/src/compute.c:432:27: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
util/LLgen/src/compute.c:435:5: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
util/LLgen/src/compute.c:1155:5: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
util/LLgen/src/gencode.c:556:8: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
util/LLgen/src/sets.c:206:7: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
util/LLgen/src/gencode.c:1458:7: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'

which seem to be caused by several instances of code that should probably be casting the left operand of their shifts to avoid invoking undefined behavior here here. (Also, to even get this far with -fsanitize I actually had to edit the generated build file manually. Is there any reason LDFLAGS doesn't seem to be used properly ?)

GabrielRavier commented 2 years ago

Also, another note, while this isn't exactly a major leak, -fsanitize does also point me to a memory leak in llgen:

=================================================================
==1468910==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 8 byte(s) in 1 object(s) allocated from:
    #0 0x7fc988194cb8 in __interceptor_realloc (/lib64/libasan.so.6+0xaecb8)
    #1 0x407525 in ralloc util/LLgen/src/alloc.c:46
    #2 0x415f5b in genname util/LLgen/src/gencode.c:1400
    #3 0x41022d in gencode util/LLgen/src/gencode.c:155
    #4 0x4032f0 in main util/LLgen/src/main.c:204
    #5 0x7fc987f3eb74 in __libc_start_main (/lib64/libc.so.6+0x27b74)

(which seems to come from the fact gencode never bothers to free the return value of genname)

davidgiven commented 2 years ago

I'm honestly not surprised. All this code comes from a different era where everyone was relying on the de-facto C behaviours which weren't the same as the standard C behaviours.

Re leaks: that could be an oversight, or it could be the traditional compiler memory allocation strategy where you never bother to free anything because the lifetime of all allocated objects is until process exit, at which point they'll all go away anyway.

Re the build system and LDFLAGS: assuming you're doing make LDFLAGS=..., it's because the ninjafile which is actually doing the work doesn't inherit variable settings from the makefile. (It's a ninja design choice which I disagree with.) The correct place to put these is the LDFLAGS definition in the top-level Makefile. That will trigger a ninjafile rebuild.

GabrielRavier commented 2 years ago

I was using the top-level Makefile, so this shouldn't be the problem