-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improving detection of garbage collection bugs #618
Conversation
e1082c7
to
33b74cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I'm not sold on the current user interface...
Please rebase this on top of master, before we merge it.
$ ./run-tests gc # Run all tests but with proper GC testing | ||
$ ./run-tests gc spec/parser_spec.lua # Run just one of the test suite files | ||
# but with proper GC testing | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if instead of a magic positional argument, we used an environment variables? We could make a new one just for GC
HARDMEMTESTS=1 ./runtests
or we could fit it with CFLAGS. (This would also work for other things that play with cflags, such as -fsanitize)
EXTRACFLAGS=-DHARDMEMTESTS ./runtests
I'm not a fan of the magic positional argument, because it would make more sense to be a --flag instead. And now that I think about it, I also don't like a --flag because we pass all flags through to busted. It would be strange to have only a single flag that's different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the EXTRACFLAGS idea, it's even more general. Will do that.
run-tests
Outdated
# When testing the garbage collector, we use -O2 for a greater chance of finding bugs. | ||
# The HARDMEMTESTS macro makes lualib do garbage collection whenever it can. | ||
[ -n "$test_gc" ] && | ||
CFLAGS='-DHARDMEMTESTS -O2 -std=c99 -Wall -Werror -Wundef -Wpedantic -Wno-unused' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of copy-pasting the whole CFLAGS line, can we add just the -DHARDMEMTESTS to the rest of the line?
Is -O0 too slow? It would be easier to debug with gdb if it comes to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied it because of -O2. -O0 is not too slow, but I thought I heard you talking about using -O2 for this GC test, and I thought doing optimizations would increase the chances of having some memory related error, but now I'm not so sure if this is true for GC specific stuff. Anyway, I'll change that and just append the EXTRAFLAGS env. variable to CFLAGS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember that. I think I might have suggested that out of the concern that it would make the tests run faster.
If -O0 is fast enough, let's stick with -O0
33b74cf
to
a51d121
Compare
a51d121
to
0002e01
Compare
Adding another mode for the run-tests script where it uses a macro that forces Lua to always run the GC, improving our chances of finding GC related bugs.