https://bugs.kde.org/show_bug.cgi?id=367995
--- Comment #9 from Julian Seward <jsew...@acm.org> ---
Ivo, thank you for reviewing this; Ruurd, thank you for addressing
Ivo's comments.
I looked at the revised patch. I am generally a bit nervous about
mempool changes given that they are not much used and I am not sure
any of the Valgrind developers really understands that code any more.
But given that it looks plausible and it has some tests, I am OK with
it, provided the issues below can be cleared up.
I have some comments/questions:
(1)
When the new functionality is not in use (that is, neither
MEMPOOL_AUTO_FREE nor MEMPOOL_METAPOOL has been passed to the mempool
creation routines), is there any weakening of the sanity checking or
assertions, relative to pre-patch?
(2)
--- include/valgrind.h (revision 15958)
+++ include/valgrind.h (working copy)
+#define MEMPOOL_AUTO_FREE 1
+#define MEMPOOL_METAPOOL 2
(2a) Please call these VALGRIND_MEMPOOL_{AUTO_FREE,METAPOOL} so as
to be consistent with the rest of the file and so as to reduce
the chances of weird namespace clashes, given that this file is
#include-d into arbitrary end-user code. And update the doc diff
accordingly.
(2b) Please mention, in the comment, that the flags are intended to be
ORd together (viz, it's not an enum). This wasn't obvious to me
on first reading.
(3)
===================================================================
--- memcheck/mc_include.h (revision 15958)
+++ memcheck/mc_include.h (working copy)
@@ -67,6 +67,7 @@
Addr data; // Address of the actual block.
SizeT szB : (sizeof(SizeT)*8)-2; // Size requested; 30 or 62
bits.
MC_AllocKind allockind : 2; // Which operation did the allocation.
+ Bool is_mempool_block;
ExeContext* where[0];
/* Variable-length array. The size depends on MC_(clo_keep_stacktraces).
This array optionally stores the alloc and/or free stack trace. */
I am concerned about the addition of a Bool to struct _MC_Chunk.
There can be hundreds of thousands of these. Given that an extra Bool
will add another word to the structure, the new Bool could increase
memory use by several megabytes. There have been significant efforts
made in the past to keep these structures small, and I don't want to
undo them.
Is it absolutely necessary to add this new Bool? Is there a way to
avoid it?
--
You are receiving this mail because:
You are watching all bug changes.
