Quick note: I think I can clarify the "For an unclear reason, the introduction of the meta pool functionality has changed" below.
Without that fix, our allocator that uses the metapool feature (allocate large blocks from which smaller blocks are allocated) would always describe the address of the large outer block, and that is totally uninteresting (triggered by the need for a new block).
The description of the smaller, inner block is the one to show.
So I did not *remove* the search for a mempool block, I moved it to the end of the function, with a comment (see below) that attempted to describe the why...  This way, the description prefers the inner block over the mempool block.

Hope that helps - Ruurd.

memcheck/mc_errors.c:1106:

   /* -- Perhaps it's in a meta mempool block? -- */
   /* This test is done last, because metapool blocks overlap with blocks
      handed out to the application. That makes every heap address part of
      a metapool block, so the interesting cases are handled first.
      This final search is a last-ditch attempt. When found, it is probably
      an error in the custom allocator itself. */
   if (mempool_block_maybe_describe( a, /*is_metapool*/ True, ai )) {
      return;
   }

(In reply to Ruurd Beerstra from comment #1)
First, thanks for providing feedback: providing further feedback/follow-up
after commit of a provided patch is well appreciated.

> Quick note: I think I can clarify the "For an unclear reason, the
> introduction of the meta pool functionality has changed" below.
> Without that fix, our allocator that uses the metapool feature (allocate
> large blocks from which smaller blocks are allocated) would always describe
> the address of the large outer block, and that is totally uninteresting
> (triggered by the need for a new block).
> The description of the smaller, inner block is the one to show.
Yes, the reason why we have 2 scans of the mempools blocks:
   one for the non meta pool first
   one for the meta pool last
is very clear/understood.

Rather, what I do not understand is why we need the new sub-condition:
"&& !MC_(is_mempool_block)(mc)"
in the below:
   mc = MC_(get_freed_block_bracketting)( a );
   if (mc && !MC_(is_mempool_block)(mc)) {
With this new sub-condition, unless I missed something, a (small inner) mempool
block freed will never be used to describe an error.

> So I did not *remove* the search for a mempool block,
True. But as I understand; what you have removed is that a mempool (inner for
meta pool) block is never landing in an error address description.
And *that* is not clear to me.

> I moved it to the end
> of the function, with a comment (see below) that attempted to describe the
> why...  This way, the description prefers the inner block over the mempool
> block.
> 
> Hope that helps - Ruurd.
> 
> memcheck/mc_errors.c:1106:
> 
>    /* -- Perhaps it's in a meta mempool block? -- */
>    /* This test is done last, because metapool blocks overlap with blocks
>       handed out to the application. That makes every heap address part of
>       a metapool block, so the interesting cases are handled first.
>       This final search is a last-ditch attempt. When found, it is probably
>       an error in the custom allocator itself. */
>    if (mempool_block_maybe_describe( a, /*is_metapool*/ True, ai )) {
>       return;
>    }

Ruurd, can you please comment?

Sorry for the delay, I'm doing lots of stuff on other projects and valgrind is on the back burner for me. It "just works" as part of our regression framework, doing several thousand runs every night and catches newly introduced memory errors every so often.
So I hope I can still make sense here :-)

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.

Point 2: The not-is-mempool test: Unless I'm completely wrong here: 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).
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. 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.

(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";

Hi,

I was completely wrong :-)
I took out the extra !MC_(is_mempool_block)(mc) and it still reports the proper, usable locations. So it can be removed.

	Thanks,
	Ruurd

-----Original Message-----
From: Philippe Waroquiers [mailto:bugzilla_noreply@kde.org] 
Sent: Monday, May 15, 2017 22:27
To: Ruurd Beerstra <Ruurd.Beerstra@infor.com>
Subject: [valgrind] [Bug 375415] free list of blocks, mempool blocks and describe addr do not work properly together

https://bugs.kde.org/show_bug.cgi?id=375415

--- Comment #5 from Philippe Waroquiers <philippe.waroquiers@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 on the CC list for the bug.

The useless condition was removed in revision 16402.