https://bugs.kde.org/show_bug.cgi?id=375415
--- Comment #5 from Philippe Waroquiers <philippe.waroqui...@skynet.be> ---
(In reply to Ruurd Beerstra from comment #4)
Thanks for the feedback, a first positive result of this feedback is
that a close examination of the test mempool2.c revealed the test
was not properly testing what it was supposed to test
(see revision 16369).
>
> Point 1 (bad redzone): My bad, caused by being all new to valgrind when I
> took a stab at modifying it. Our code doesn't use redzones, so both are zero
> and I didn't look closely enough. Needs a fix.
Yes, it needs a fix. But do not worry, you did not create this bug.
As far as I can see, this bug was already there since a very very long time
(even before I implemented --redzone-size).
It is however not easy to fix, at least not without having a list of freed
blocks per memory pool, which then implies probably to heavily rework
the current implementation of free blocks.
>
> Point 2: The not-is-mempool test: Unless I'm completely wrong here:
I believe you are completely wrong here :).
As I understand, in the below test, no way mc can even be a mempool block
mc = MC_(get_freed_block_bracketting)( a );
if (mc && !MC_(is_mempool_block)(mc)) {
The reasoning: is_mempool_block checks that mc is a mempool block by
scanning the chunks in all mempools.
But if a block has been freed, then it is removed from the mp->chunks.
So, as I understand, this test is useless : if mc is found, it will never
be a free mempool block.
> If I
> take that test out, many application-malloced blocks that lie within a
> mempool block will be described as lying within a mempool block, with the
> allocation stack being the one that triggered the allocation of that mempool
> block (some random moment when the pool is exhausted).
I think that there is confusion between 2 different conditions:
* the condition that I am discussing just above, which is useless as
I understand (at least, removing it makes no difference in the
regression tests, and reading the code, as explained above, it looks
useless).
* the other condition is what you have added in the calls to
mempool_block_maybe_describe, which is to check for metapool False,
and then True. Yes, these 2 calls are necessary, otherwise, effectively,
a freed inner block will always be described by the first call
to mempool_block_maybe_describe.
So, I am tempted to just remove this useless condition.
It would be nice however if you could just first check that effectively
in your application, it also makes no difference.
(see patch below)
> That is not what I want: I want the description of the malloc that was done
> *from* the mempool block, with the proper allocation stack.
> This is because our allocator uses itself to allocate the mempool blocks, so
> both mempool-block allocs and plain application allocs are on the same
> malloc list. The test for a mempool block at the end was added to catch the
> case where an application-malloc is NOT within a mempool, which should not
> happen in our custom-allocator case, but the case needs handling.
> Your remark: "what you have removed is that a mempool (inner for
> meta pool) block is never landing in an error address description" is
> correct: I consider that address description an internal and uninteresting
> affair of our custom allocator and there should always be a more
> interesting, application malloc stack that I want to show.
> Does that make sense?
>
> Point 3: About the order: I think you sort of echo there what I try to
> describe above. This is hard stuff, trying to get the best address
> description in all cases.
Yes, we need a rework of this. E.g. the msg
"recently re-allocated block" is sometimes plain wrong, when a outer block
was split in inner blocks, and an inner block was freed. The outer block
is described as recently reallocated, which is not true.
> All I can say: The current version works for me,
> and I gather there are few custom allocators these days that use the mempool
> features. But if my amateur changes require rework: feel free to improve
> upon it. I'll (test) run it in our test environment.
If you could check the below patch, that would be nice.
Thanks
Index: memcheck/mc_errors.c
===================================================================
--- memcheck/mc_errors.c (revision 16378)
+++ memcheck/mc_errors.c (working copy)
@@ -1098,7 +1098,7 @@
}
/* -- Search for a recently freed block which might bracket it. -- */
mc = MC_(get_freed_block_bracketting)( a );
- if (mc && !MC_(is_mempool_block)(mc)) {
+ if (mc) {
ai->tag = Addr_Block;
ai->Addr.Block.block_kind = Block_Freed;
ai->Addr.Block.block_desc = "block";
--
You are receiving this mail because:
You are watching all bug changes.
