|
From: Philippe W. <phi...@sk...> - 2012-07-28 16:06:17
Attachments:
patch_clo_redzone.txt
|
Currently, tools which are replacing malloc (VG_(needs_malloc_replacement) can control the size of the redzone CLIENT arena, but this is all defined at compile time. Similarly, the redzone size for the core arenas (CORE, TOOL, DINFO, ...) is also defined at compile time. The attached patch implements two command line options: --client-redzone=<number> and --core-redzone=<number> The first clo can be used to either decrease the memory used by Valgrind or increase the protection or chance to discover heap overrun/underrun. The default behaviour is unchanged : if no clo is given, same as 3.7.0 (so, value of the tool provided redzone size is used). If --client-redzone is given, it overrides (at arena init) the default value provided by the tool. The decrease is particularly useful for applications doing a lot of small block allocations and using tools with a big redzone (such as helgrind or memcheck) while it is believed (e.g. as memcheck was already used) that no heap over/under-run occurs. The second option allows to (better) chase bugs inside Valgrind core and tools (by increasing the redzone size for the internal Valgrind arena), if ever it would be needed. One new test added to verify the clo has an effect on memcheck. Regression tested on x86 and amd64. Verified with --profile-heap=yes that mapped memory increases or decreases according to the clo option size. All tests also have run with --client-redzone=0 and 128 and with --core-redzone=128. A few tests are failing (for normal reasons) because the memory layout is changing Feedback welcome One question: I have currently limited the max value of the redzone to 128 bytes (similar to the assert on the tool received value). I am wondering if we should not allow more (e.g. up to a page of 4Kb ?) from the clo ? Philippe |
|
From: Florian K. <br...@ac...> - 2012-07-29 18:15:10
|
On 07/28/2012 12:06 PM, Philippe Waroquiers wrote:
> Feedback welcome
>
No objections to the patch in general as it provides more flexibility at
no cost and is backward compatible afaict.
A couple of comments:
> +SizeT VG_(malloc_effective_client_redzone)(void)
> +{
> + SizeT result;
> + ensure_mm_init (VG_AR_CLIENT);
> + vg_assert(VG_(needs).malloc_replacement);
> + if (VG_(clo_client_redzone) == -1)
> + result = VG_(tdict).tool_client_redzone_szB;
> + else
> + result = VG_(clo_client_redzone);
> + if (result < sizeof(void*)) result = sizeof(void*); <----<<
Is this test needed here? I think arena_init will take care of any
adjustments (including this specific one) that are needed on the size
of the redzone. Preferably, all such adjustments should be made in one
place only.
And 2 nitpicks:
> Bool VG_(clo_profile_heap) = False;
> +Int VG_(clo_core_redzone) = 4;
> +Int VG_(clo_client_redzone) = -1;
alignment of the variable names with preceeding one. Help prevent
eye-cancer! :)
> +// VG_(clo_client_redzone) != -1 means to override the value
> +// provided by tool in
VG_(needs_malloc_replacement).tool_client_redzone_szB
Perhaps:
// A value != -1 overrides the tool-specific value in
// VG_(needs_malloc_replacement).tool_client_redzone_szB
I regtested on s390x with no new regressions.
Florian
|
|
From: Philippe W. <phi...@sk...> - 2012-07-29 19:35:23
|
On Sun, 2012-07-29 at 14:15 -0400, Florian Krohm wrote: > > + if (result < sizeof(void*)) result = sizeof(void*); <----<< > > Is this test needed here? I think arena_init will take care of any > adjustments (including this specific one) that are needed on the size > of the redzone. Preferably, all such adjustments should be made in one > place only. VG_(malloc_effective_client_redzone) must return the effective value used so takes this condition into account. > alignment of the variable names with preceeding one. Help prevent > eye-cancer! :) Alignment done. > > > +// VG_(clo_client_redzone) != -1 means to override the value > > +// provided by tool in > VG_(needs_malloc_replacement).tool_client_redzone_szB > > Perhaps: > // A value != -1 overrides the tool-specific value in > // VG_(needs_malloc_replacement).tool_client_redzone_szB => replaced the comment by: // A value != -1 overrides the tool-specific value // VG_(needs_malloc_replacement).tool_client_redzone_szB (just removed the 'in', as the overrides does not happen in tool_client_redzone_szB) Thanks Philippe |
|
From: Julian S. <js...@ac...> - 2012-07-31 09:01:47
|
On Sunday, July 29, 2012, Florian Krohm wrote: > > Bool VG_(clo_profile_heap) = False; > > +Int VG_(clo_core_redzone) = 4; > > > > +Int VG_(clo_client_redzone) = -1; > > alignment of the variable names with preceeding one. Help prevent > eye-cancer! :) Indeed, if the source code is not aesthetically pleasing, it is unlikely to work correctly :) J |
|
From: Julian S. <js...@ac...> - 2012-07-31 09:44:31
|
On Saturday, July 28, 2012, Philippe Waroquiers wrote:
> The attached patch implements two command line options:
> --client-redzone=<number>
> and
> --core-redzone=<number>
Generally a good thing. I have two concerns about this:
* in the distant past there was complexity/many bugs/difficulties
w.r.t. using dynamic memory management before it is initialised.
I want to be sure we don't go back there. Can you add assertions
that guarantee that no dynamic memory allocation happens in any
arena before m_mallocfree is properly initialised (viz, redzone
sizes are parsed, etc)
* that redzone sizes cannot get set to stupid values. I assume
you have some check to allow them to be set only to some
integral number of host words, >= 0 ?
and some more minor comments
* I assume the defaults are unchanged? Memcheck client heap has
16 byte redzones, all other arenas == 4 byte? Is 4 bytes correct
even on 64-bit platforms? That surely causes misalignments, no?
* no bad interactions with --profile-heap?
* no bad interactions with heap related client requests (MALLOCLIKE_BLOCK etc)
?
* I'd prefer to change some of the names a bit.
--client-redzone --> --redzone-size
(eg, we just have "--alignment=" for the client, not "--client-alignment=")
Then for consistency --core-redzone ==> --core-redzone-size
+ rename VG_(clo_*) accordingly
* MC_(Malloc_Redzone_Szb) --> MC_(Malloc_Redzone_SzB)
* +" heap blocks (in bytes). [default depends on the tool]\n"
^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is really a shame -- would be much nicer to be able to print
the default in these messages. Is there no way to do that?
Maybe it is something that should be passed by the tool across the
core-tool interface at startup, when the tool requests malloc replacement?
Or maybe it is too late in the initialisation process for that.
* does this patch slow down malloc/free? Any new calls in the
allocation/free paths?
|
|
From: Philippe W. <phi...@sk...> - 2012-07-31 21:30:39
|
On Tue, 2012-07-31 at 11:37 +0200, Julian Seward wrote: > Generally a good thing. I have two concerns about this: > > * in the distant past there was complexity/many bugs/difficulties > w.r.t. using dynamic memory management before it is initialised. > I want to be sure we don't go back there. Can you add assertions > that guarantee that no dynamic memory allocation happens in any > arena before m_mallocfree is properly initialised (viz, redzone > sizes are parsed, etc) It is not clear to me how I can check that option parsing has been done but I have now regrouped the logic to compute the effective redzone size to have it only in arena_init. => Any call to malloc implies a call to ensure_mm_init for the arena, which properly sets the effective redzone size and verifies (for the client arena) that the tool provided redzone size has not been changed). I think it is pretty safe with that. > > * that redzone sizes cannot get set to stupid values. I assume > you have some check to allow them to be set only to some > integral number of host words, >= 0 ? As discussed on irc, we will allow up to 4Kb for the clo values to let 'off by one in an array of page' to be detect(able). There will still be a check that the default values are reasonable (<=128). > > and some more minor comments > > * I assume the defaults are unchanged? Memcheck client heap has > 16 byte redzones, all other arenas == 4 byte? Is 4 bytes correct > even on 64-bit platforms? That surely causes misalignments, no? Yes, default are unchanged. The current default value for core redzone is 4, even on 64-bit. It does not cause alignment problems because this is the minimum redzone size. It is increased to ensure the admin part of a block is a multiple of VG_MIN_MALLOC_SZB (8 on 32 bits except on Darwin, 16 bytes for 64 bits and Darwin). > > * no bad interactions with --profile-heap? Works ok (I used --profile-heap to check the clo are working). > * no bad interactions with heap related client requests (MALLOCLIKE_BLOCK etc) The regression tests are ok. MALLOCLIKE_BLOCK and similar are using their own redzone sizes so should not be impacted. > * I'd prefer to change some of the names a bit. > --client-redzone --> --redzone-size > (eg, we just have "--alignment=" for the client, not "--client-alignment=") > > Then for consistency --core-redzone ==> --core-redzone-size > > + rename VG_(clo_*) accordingly Names and clo names changed according to the above. > > * MC_(Malloc_Redzone_Szb) --> MC_(Malloc_Redzone_SzB) Done. > > * +" heap blocks (in bytes). [default depends on the tool]\n" > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > This is really a shame -- would be much nicer to be able to print > the default in these messages. Is there no way to do that? Done. If a tool does not replace malloc, the default values shown will be [not used by this tool] for alignment and redzone (I did not want to show a rubbish value for tools not replacing malloc). > * does this patch slow down malloc/free? Any new calls in the > allocation/free paths? The effective redzone size is computed at arena_init time, so there is no impact on performance. Will regtest and commit. Thanks for the review Philippe |