https://bugs.kde.org/show_bug.cgi?id=367995
Ivo Raisr <iv...@ivosh.net> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|UNCONFIRMED |CONFIRMED
Ever confirmed|0 |1
--- Comment #1 from Ivo Raisr <iv...@ivosh.net> ---
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.
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.
3. describe_addr()
Why it is necessary to check for meta mempool almost at the end?
Please add some commentary.
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.
5. memcheck/mc_include.h
+ int is_mempool_block;
Use 'Bool' instead and 'True'/'False' for values.
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.
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.
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.
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.
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?
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)"
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.
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)().
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?
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.
16. free_mallocs_in_mempool_block()
+ int found;
Use 'Bool' and 'True'/'False'.
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'?
+struct pool _PlainPool, *PlainPool = &_PlainPool;
+struct pool _MetaPool, *MetaPool = &_MetaPool;
Use static.
+static int MetaPoolFlags = 0;
For consideration: You can use arguments to create_meta_pool() and remove this
unnecessary variable.
+ /* A pool-block is expected to have overhead, and the core of
'overhead' is usually called 'metadata' in allocator's lingo. Just for
clarification.
--
You are receiving this mail because:
You are watching all bug changes.
