[valgrind] [Bug 375415] free list of blocks, mempool blocks and describe addr do not work properly together

[Philippe Waroquiers]

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

--- Comment #7 from Philippe Waroquiers  ---
The useless condition was removed in revision 16402.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 375415] free list of blocks, mempool blocks and describe addr do not work properly together

[Ruurd Beerstra]

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

--- Comment #6 from Ruurd Beerstra  ---
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_nore...@kde.org] 
Sent: Monday, May 15, 2017 22:27
To: Ruurd Beerstra 
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  --- (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 

[valgrind] [Bug 375415] free list of blocks, mempool blocks and describe addr do not work properly together

[Philippe Waroquiers]

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

--- Comment #5 from Philippe Waroquiers  ---
(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.

[valgrind] [Bug 375415] free list of blocks, mempool blocks and describe addr do not work properly together

[Ruurd Beerstra]

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

--- Comment #4 from Ruurd Beerstra  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 375415] free list of blocks, mempool blocks and describe addr do not work properly together

[Ivo Raisr]

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

Ivo Raisr  changed:

   What|Removed |Added

 CC||iv...@ivosh.net

--- Comment #3 from Ivo Raisr  ---
Ruurd, can you please comment?

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 375415] free list of blocks, mempool blocks and describe addr do not work properly together

[Philippe Waroquiers]

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

Philippe Waroquiers  changed:

   What|Removed |Added

 CC||philippe.waroquiers@skynet.
   ||be

--- Comment #2 from Philippe Waroquiers  ---
(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;
>}

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 375415] free list of blocks, mempool blocks and describe addr do not work properly together

[Ruurd Beerstra]

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

Ruurd Beerstra  changed:

   What|Removed |Added

 CC||ruurd.beers...@infor.com

--- Comment #1 from Ruurd Beerstra  ---
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;
   }

-- 
You are receiving this mail because:
You are watching all bug changes.