TTQ-788 Dev fuzzer malloc #429
Conversation
There was a problem hiding this comment.
Looks good to me. If @frederikvs can give his 2cts, then he can merge
There was a problem hiding this comment.
Nice work.
I'm not sure about the frequency of opening-closing that file there, but l wouldn't worry about that for now. If it turns out to be a problem, it'll only be a problem in our test setup, and we can have a look at it then.
A few minor comments inline, once those are resolved I'll merge.
|
|
||
| #ifdef CHECK_MEM | ||
| //New log for malloc information | ||
| fopen("mem_test.log", "w"); |
There was a problem hiding this comment.
Are you doing anything with the file you're opening here?
There was a problem hiding this comment.
The idea was to make sure to clear the file before running a new set of tests but this was flawed since it was getting called with each new test and so only the last test's mallocs were getting logged in the end. I changed to outputting this in stdout, which also makes it clear which tests the mallocs occurred in and which ASAN errors are related to them.
| fopen("mem_test.log", "w"); | ||
|
|
||
| //Set rand seed | ||
| char *buf = getenv("MEM_TEST_SEED"); |
There was a problem hiding this comment.
Are you also putting this seed into some kind of log, so that we can try to recreate afterwards?
There was a problem hiding this comment.
I'll try to get this done. Just noticed there's a problem of getting the environment variable when I run it through the RF test rather than just the unit tests.
There was a problem hiding this comment.
Could be good to make a single function that reads all environment variables into local/global variables. Then you can also log these immediatly.
What's wrong with getting them in RF tests? it's possible that these set up a virtual environment...
| } | ||
|
|
||
| #define PICO_ZALLOC(x) \ | ||
| ((start_failing_mallocs && (((double)(rand())/(double)RAND_MAX) < 0.4)) \ |
There was a problem hiding this comment.
Could we get the probability of failure out of an environment variable as well? Might be interesting to automatically test with that.
|
Oh, could you attach some example outputs to this PR? Then we could have a quick look, see how easy it is to figure out what's going on :-) |
| #define PICO_ZALLOC(x) \ | ||
| ((start_failing_mallocs && (((double)(rand())/(double)RAND_MAX) < 0.4)) \ | ||
| ? (log_malloc("mem_test.log", "Malloc FAILED\n"), append_backtrace("mem_test.log"), NULL) \ | ||
| : (log_malloc("mem_test.log", "Malloc Succeeded\n"), append_backtrace("mem_test.log"), pico_zalloc(x))) |
There was a problem hiding this comment.
An afterthought : it looks like this statement expression is GNU C [0], not entirely standard. In the interest of compiler compatibility, it may be better to just put this into a function...
[0] https://gcc.gnu.org/onlinedocs/gcc-3.3.1/gcc/Statement-Exprs.html
There was a problem hiding this comment.
How would you do that? The problem with that is that 'pico_zalloc()' isn't defined yet
There was a problem hiding this comment.
Isn't pico_zalloc() defined in the arch-file that was included a little above this?
There was a problem hiding this comment.
Oops you're right. I tried it again. The problem is actually multiple definitions of the function I create since pico_config is compiled into everything. Trying to fix it
There was a problem hiding this comment.
would inline solve this?
There was a problem hiding this comment.
Yup! static inline does. Thanks :)
@frederikvs
-Implementation of pseudo-randomly failing malloc
-Uses env variable 'MEM_TEST_SEED' to specify the seed to rand
-Compile with flag "CHECK_MEM=1' to use this zalloc implementation