https://bugs.kde.org/show_bug.cgi?id=367995
--- Comment #5 from Ruurd Beerstra <[email protected]> --- Hi, Thank you for taking the time to look (very thoroughly) into this. >--- Comment #1 from Ivo Raisr <[email protected]> --- Thank you for the patch >and specially for regression >tests. > >Overall it looks quite good but needs some polishing first. >I have the following questions and comments: > >1. Please ensure all lines are shorter than 80 characters. >I even spotted some of them are > 100 chars. >I understand existing code is not 100% perfect and there can be some >exceptions but general rule >is <= 80 chars. For real? Personally, I like to take advantage of modern technology like big screens. But: Your source, your rules. I've changed it. >2. include/valgrind.h >+#define MEMPOOL_METABLOCKS 2 >I wonder why this is called MEMPOOL_METABLOCKS and not simply MEMPOOL_METAPOOL? >This would greatly simplify its description both in valgrind.h and >mc-manual.xml. >And also greatly enhance understanding of is functionality. You're right. My first stab at hacking valgrind was a fix at the block-level. When my understanding of valgrind grew, the code changed, the name did not. My bad. Fixed it. >3. describe_addr() >Why it is necessary to check for meta mempool almost at the end? >Please add some commentary. Done. >4. mempool_block_maybe_describe() >We could do better here with respect to meta mempool description. >Based on your experience, is it sufficient to differentiate between meta and >non-meta pools only >based on the allocation stack trace? >I think meta pool should deserve its own BlockKind value which >pp_addrinfo_WRK() will utilize. Out custom allocator allocates metapool blocks, and then doles out addresses from those blocks to the applications. That makes every address handed out to the application part of a metapool block, which is why describe_addr looks for that kind of description at the very end because all metablock descriptions are boring: the place in the allocator where the metablock is allocated. The allocation stack is even misleading, as a random event in the application may trigger the condition in the allocator where it decides in needs a new block for the pool. So a "better" metapool-block description will only always point to the (uninteresting/misleading) place in the allocator itself. So I did not change the description. >5. memcheck/mc_include.h >+ int is_mempool_block; >Use 'Bool' instead and 'True'/'False' for values. Done. But I could not help but noticing that this rule is violated often in valgrind sources... >6. MC_(detect_memory_leaks)() >+ // Another exception is that a custom allocator uses >VALGRIND_MALLOCLIKE_BLOCK >+ // to allocate mempool blocks to allocate from, indicated by the metablock >+ // property of the mempool block. > >I don't get meaning of this sentence. Please could you break it to more >swalloable chunks or rephrase it. Done. >7. memcheck/mc_leakcheck.c >+ if (! ((ch1->is_mempool_block && (mp1 = find_mp_of_chunk(ch1)) >+ != >NULL && mp1->metablock) || >+ (ch2->is_mempool_block && (mp2 = find_mp_of_chunk(ch2)) >+ != >NULL && mp2->metablock) >+ ) >Please do not use variable assignment in 'if' condition. This makes the logic >hard to follow. Hmm. A matter of taste and style. It turns the 2 lines into about 15. There is something to be said for brevity, too. Also, I saw plenty of places in the valgrind source where this technique is used. But: Your source: Done. >8. memcheck/mc_main.c 2016-08-16 11:20:22.000000000 +0200 > MC_(new_block) ( tid, p, sizeB, /*ignored*/0, is_zeroed, >- MC_AllocCustom, MC_(malloc_list) ); >+ MC_AllocCustom, /*not pool*/0, >+ MC_(malloc_list) ); >Use 'False' instead of '0'. The comment is not in sync with MC_(new_block)() >declaration. I interpreted the in-line comments as "show what the constant means", rather than a compulsory exact quoting of the name of the parameter (like the "ignored" in the same call). The definition of the function is only a single keystroke/mouse click away, if I want to look at it. But: Done. >9. memcheck/mc_main.c, around case VG_USERREQ__CREATE_MEMPOOL: >+ UInt flags = arg[4]; > - MC_(create_mempool) ( pool, rzB, is_zeroed ); >+ // The create_mempool function does not know these mempool >+ flags, >pass as booleans. >+ MC_(create_mempool) ( pool, rzB, is_zeroed, (flags & >MEMPOOL_AUTO_FREE), (flags & MEMPOOL_METABLOCKS) ); > >The comment here is redundant. For you, maybe. My first attempt simply passed the flags to the create_mempool function, which failed to compile. It surprised me that I could not use those constants in the receiving function. Extending the "flags" with more bits requires changing the signature of create_mempool. So a comment is appropriate IMHO. >10. memcheck/mc_malloc_wrappers.c >+ mc->is_mempool_block = is_mempool_block; > VG_(HT_add_node)( table, mc ); >- > if (is_zeroed) > >Why that empty line is removed? By accident. Restored. >11. void MC_(create_mempool)() >+ VG_(message)(Vg_UserMsg, "create_mempool(0x%lx, %u, %d, %d, %d)\n", >+ pool, rzB, is_zeroed, auto_free, >+ metablock); > >Please improve this by clarifying what is what in the message, for example: >"create_mempool(<addr>, size=%u, zerod=%d, autofree=%d, metablock=%d)" I just extended the existing message. The other functions also simply print their parameters without annotation (mempool_alloc, mempool_free, mempool_trim, etc). But: Done. >12. MC_(mempool_free)() >+ if (mp->auto_free) { >+ // All allocs in this memory block are not to been seen as leaked. >+ free_mallocs_in_mempool_block(mp,mc->data,mc->data + (mc->szB + 0UL)); >+ } >The comment is redundant. >You are missing spaces after commas for the argument list. IMHO: Comments are hardly ever redundant. And CSCOPE finds 23507 instances of a comma not followed by a space in C-sources of valgrind :-) But: Done. >13. free_mallocs_in_mempool_block() >+ >+ if (mp->auto_free) { >+ ... > >Why you test for mp->auto_free again? This was done back in >MC_(mempool_free)(). Because what is true today, can be false tomorrow. I am used to sprinkling sources with assertions and "superfluous" extra checks. Saved me on numerous occasions. But, on reflection, I've changed it to a tl_assert instead of an IF that silently ignores a bad call. >14. free_mallocs_in_mempool_block() >+ if (VG_(clo_verbosity) > 2) { >+ VG_(message)(Vg_UserMsg, "MEMPOOL_DELETE: pool from 0x%lx to >+ 0x%lx >(size %ld) auto-free\n", >+ StartAddr,EndAddr,(unsigned long int) (EndAddr - StartAddr)); >+ } >and >+ if (VG_(clo_verbosity) > 2) { >+ VG_(message)(Vg_UserMsg, "Auto-free of 0x%lx >size=%ld\n",mc->data,(long int) mc->szB); >+ } > >Please get these messages in sync with the rest of this module so they are >first class citizens. >Also use spaces after commas. You cannot use %ld to print unsigned long int. >Naked 'unsigned long int' is not used in Valgrind core - use something more >appropriate, for example >SizeT. Is the cast really necessary here? Done. >15. free_mallocs_in_mempool_block() >+ while (!found && (mc = VG_(HT_Next)(MC_(malloc_list))) ) { >Please do not use variable assignment in 'if' condition. This makes the logic >hard to follow. I disagree. So does the rest of the valgrind source: there are 173 occurrences of assignments in a while statement in the 3.11.0 C source. All but 2 of 21 HT_next usages in the 3.11.0 source tree are of the same form: Assignment in a while. In libhb_core.c (line 4861) are the only other 2 of HT_next uses: there it leads to code duplication (which, in my book, is a Truly Bad Thing). >16. free_mallocs_in_mempool_block() >+ int found; >Use 'Bool' and 'True'/'False'. Done. >17. memcheck/tests/leak-autofreepool.c >Why not having 'struct cell' and 'struct pool' typedef'd to something more >easily used, for example >'cell_t' and 'pool_t'? Because the test was copy/pasted/modified from the existing leak-pool.c test. >+struct pool _PlainPool, *PlainPool = &_PlainPool; struct pool >+_MetaPool, *MetaPool = &_MetaPool; >Use static. Done. >+static int MetaPoolFlags = 0; >For consideration: You can use arguments to create_meta_pool() and remove this >unnecessary variable. That is how I would have written it, but the other tests in the test-framework that I studied follow that pattern: Pass a single argument which has a magic value. The magic value is translated by the main function into a specific test-case (in this case: flags in MetaPoolFlags). For example, the existing leak-pool tests calls leak-pool with values 0 - 5, with all .EXP files having the same magic value in the name. I only learned enough about the test framework to be able to get the extra regression-test up & running, and tried to stay in the same vein as existing tests. Are you saying that is a Bad Thing? >+ /* A pool-block is expected to have overhead, and the core of >'overhead' is usually called 'metadata' in allocator's lingo. Just for >clarification. Our custom allocator calls it "overhead" :-) Updated. > Also when running the newly introduced regressions tests, I encountered this >failure: >$ cat memcheck/tests/leak-autofreepool-5.stderr.diff >--- leak-autofreepool-5.stderr.exp 2016-08-31 15:08:12.835033806 +0000 >+++ leak-autofreepool-5.stderr.out 2016-08-31 15:38:03.503967988 +0000 Embarrassing: The line-number of the assert sneaked inside the EXP file. The (new) filter left that line-number intact. Two bugs that cancelled each other out. Worked for me, but nowhere else, of course. Darn, but programming is hard :-) Fixed the filter and the EXP file for test 5. It now says: make[1]: Leaving directory `/home/rbeerstr/grind/valgrind-3.11.patch/memcheck/tests' leak-autofreepool-0: valgrind --leak-check=full --show-possibly-lost=no --track-origins=yes ./leak-autofreepool 0 leak-autofreepool-1: valgrind --leak-check=full --show-possibly-lost=no --track-origins=yes ./leak-autofreepool 1 leak-autofreepool-2: valgrind --leak-check=full --show-possibly-lost=no --track-origins=yes ./leak-autofreepool 2 leak-autofreepool-3: valgrind --leak-check=full --show-possibly-lost=no --track-origins=yes ./leak-autofreepool 3 leak-autofreepool-4: valgrind --leak-check=full --show-possibly-lost=no --track-origins=yes ./leak-autofreepool 4 leak-autofreepool-5: valgrind --leak-check=full --show-possibly-lost=no --track-origins=yes ./leak-autofreepool 5 == 6 tests, 0 stderr failures, 0 stdout failures, 0 stderrB failures, 0 stdoutB failures, 0 post failures == Works for me (again :-) Hope this version of the patch is acceptable, Regards, Ruurd Beerstra -- You are receiving this mail because: You are watching all bug changes.
