|
From: Nicholas N. <nj...@cs...> - 2005-03-28 14:09:01
|
On Mon, 28 Mar 2005, Jeremy Fitzhardinge wrote:
> At present, there's a somewhat artificial dependency between loading the
> tool.so and initializing the allocator. If the allocator could stand to have
> the redzone size set after the rest of the allocator is initialized (but
> before the first client arena allocation), we could initialize and use the
> allocator much earlier. I don't know if this change would help or not, but
> it looks like it might.
Yes, I realised after sending the patch that there's a better way.
vg_malloc2.c could export a tool-visible function
VG_(set_malloc_redzone_szB)(). There would be a restriction that tools
can only call it before calling VG_(malloc)() for the first time. It
would have a little check in it to enforce this. That avoids the global
variable VG_(malloc_redzone_szB)() and keeps it all neatly within
vg_malloc2.c.
>> + VG_(basic_tool_funcs) (TL_(post_clo_init),
>> + TL_(instrument),
>> + TL_(fini));
>
> I would either go with a registry function per function, or pass a struct.
> Having registry functions which takes lots of args doesn't seem like much of
> an improvement. Though a bit more verbose, something like:
>
> VG_(register_clo_init)(my_clo_init);
> VG_(register_instrument)(my_instrument);
> VG_(register_fini)(my_fini);
>
> seems more maintainable and robust in the face of interface change to me.
>
>> +
>> VG_(needs_core_errors) ();
>> - VG_(needs_tool_errors) ();
>> + VG_(needs_tool_errors) (TL_(eq_Error),
>> + TL_(pp_Error),
>> + TL_(update_extra),
>> + TL_(recognised_suppression),
>> + TL_(read_extra_suppression_info),
>> + TL_(error_matches_suppression),
>> + TL_(get_error_name),
>> + TL_(print_extra_suppression_info));
>
>
> Particularly compared to this kind of thing. What if you don't want all
> these callbacks? You end up with a call like this (totally made up example):
>
> VG_(needs_tool_errors)(NULL, NULL, NULL, NULL,
> TL_(read_extra_suppression_info), NULL,
> TL_(get_error_name), NULL);
>
> which doesn't seem very readable or maintainable.
The reason I did it this way is that with the ones I grouped together, the
tool has to provide none of them or all of them. All tools have to
provide pre_clo_init(), instrument() and fini(), so they're grouped
together. If the tool wants to report errors (ie. needs_tool_errors), it
has to provide all eight of those functions: eq_Error(), pp_Error(), etc.
If it doesn't provide all of them the core will bomb out at some point
with a "missing tool function" error.
Does that make a difference to how you feel about it?
> At least with a struct you can name the elements you're initializing:
>
> static const struct tool_error_funcs my_tool_errors = {
> .read_extra_suppression = my_read_extra_suppression,
> .get_error_name = my_get_error_name,
> };
> VG_(needs_tool_errors)(&my_tool_errors);
Having the name on the LHS is nice.
> And this isn't any more robust than passing a structure against adding new
> interface functions (it's worse, because adding a new function would require
> every caller to VG_(needs_tool_errors)() be changed, even if they don't care
> about the new function; a static struct will automatically initialize
> otherwise uninitialized members to NULL).
If new functions are added to any of the groups, all tools that use those
groups would have to be changed anyway, due to the all-or-nothing
property.
N
|