|
From: Nicholas N. <nj...@cs...> - 2005-03-27 16:37:06
|
Hi, I just spent some time learning how SK_(foo) functions are called. It's a lot more complex than I realised. In v2.0.0 and earlier, we had default definitions of all these functions, such as SK_(instrument)(), in vg_default.c. They were defined with the 'weak' attribute. If the tool had a function called SK_(instrument)(), that would override the one in vg_default.c due to the tool.so being put in LD_PRELOAD. If a tool forgot to provide a function it should have, the default version would end up getting called and it just aborted with an error message. Since v2.1.0, things have gotten more complex. Now, main() calls VG_(tool_init_dlsym)(). It uses dlsym() to find all the functions like vgSkin_instrument() that the core provides. It puts pointers to these functions into a struct called VG_(tool_interface)(). vg_toolint.c then provides functions such as vgSkinInternal_instrument(), which just call vgSkin_instrument() via the pointer in VG_(tool_interface). This involves redefining the meaning of "SK_()" in core.h to prefix "vgSkinInternal_", instead of "vgSkin_". There is a lot of other stuff in vg_toolint.c for providing default definitions which immediately abort, etc. The files vg_toolint.[ch] are auto-generated. This all took me a while to figure out. There were a few aspects that puzzled me. - The two different meanings of SK_() were really confusing, and probably doubled the amount of time it took me to work this all out. - We're relying heavily on dlsym(), which the 2.0.0 approach did not. Is there a good reason why dlsym() must be used rather than LD_PRELOAD? I ask because we use LD_PRELOAD for the vgpreload_foo.so files -- can we not use it for the normal vgtool_foo.so files also? The use of one mechanism (LD_PRELOAD) would be preferable to the use of two (dlsym + LD_PRELOAD), no? - It seems like a lot of the stuff in vg_toolint.c and vg_toolint.h is global when it doesn't need to be. For example, the VG_(missing_*)() and VG_(defined_*)() functions can be made local to vg_toolint.c. This is probably just a matter of tweaking the auto-generation script gen_toolint.pl. - Jeremy, in vg_pthreadmodel.c you call SK_(malloc)() -- why? I'm not sure what you're doing there, and I think the SK_(malloc)() in vg_default.c might not be getting called, because it relied on the 2.0.0-style use of the 'weak' attribute. But I'm really not sure. How does that vg_pthreadmodel.c code work? I'd really like to remove SK_(malloc)() and SK_(free)() from vg_default.c, now that libpthread isn't around to use them. I'm sure there are good reasons for all this, they just weren't obvious to me from the code. Any explanations would be appreciated! Thanks. N |
|
From: Jeremy F. <je...@go...> - 2005-03-27 17:05:07
|
Nicholas Nethercote wrote:
> This involves redefining the meaning of "SK_()" in core.h to prefix
> "vgSkinInternal_", instead of "vgSkin_". There is a lot of other
> stuff in vg_toolint.c for providing default definitions which
> immediately abort, etc. The files vg_toolint.[ch] are auto-generated.
>
> This all took me a while to figure out. There were a few aspects that
> puzzled me.
>
> - The two different meanings of SK_() were really confusing, and
> probably doubled the amount of time it took me to work this all out.
Yeah, that seems a bit bogus. I guess it could have the same
definitions in both tools and core, and you'd simply get a pair of
functions for each tool interface - one in the tool, and one in the
core. Or changing all the core SK_() references to some other prefix macro.
> - We're relying heavily on dlsym(), which the 2.0.0 approach did not.
> Is there a good reason why dlsym() must be used rather than
> LD_PRELOAD? I ask because we use LD_PRELOAD for the vgpreload_foo.so
> files -- can we not use it for the normal vgtool_foo.so files also?
> The use of one mechanism (LD_PRELOAD) would be preferable to the use
> of two (dlsym + LD_PRELOAD), no?
I was hoping to drop all the magic symbol stuff altogether. I would
prefer it if each tool .so had only a single special symbol to an init
function; that init function would then call a core function passing a
structure full of pointers to functions for all its tool functions. I
don't really like all this implicit behaviour based on what symbols
happen to exist. I intended the dlsym() stuff to be a backwards
compatibility thing which would go away once all the tools had been
converted.
LD_PRELOAD won't work for loading the tools because we'd need to set it
before starting valgrind in the first place, but we don't know what tool
to use at that point. You could probably do it by shifting more
command-line parsing into stage1 and have it hack on the environment,
but I'd prefer to keep stage1 as simple as possible.
> - It seems like a lot of the stuff in vg_toolint.c and vg_toolint.h is
> global when it doesn't need to be. For example, the VG_(missing_*)()
> and VG_(defined_*)() functions can be made local to vg_toolint.c.
> This is probably just a matter of tweaking the auto-generation script
> gen_toolint.pl.
The defined_* ones are used all over the place from its use in the
VG_TRACK() macro. missing_* need to be global because they're weak
symbols which can be overridden. The default missing_* functions are in
vg_toolint.c, but vg_default.c defines some replacements. This should
probably be implemented by replacing pointers to functions rather than
using linker tricks.
> - Jeremy, in vg_pthreadmodel.c you call SK_(malloc)() -- why? I'm not
> sure what you're doing there, and I think the SK_(malloc)() in
> vg_default.c might not be getting called, because it relied on the
> 2.0.0-style use of the 'weak' attribute. But I'm really not sure.
> How does that vg_pthreadmodel.c code work? I'd really like to remove
> SK_(malloc)() and SK_(free)() from vg_default.c, now that libpthread
> isn't around to use them.
The pthread_create code wants to allocate some memory in the client
which the client can free via VG_USERREQ_FREE. I had to make a couple
of small changes so this would work for both malloc-replacing and
non-malloc-replacing tools.
J
|
|
From: Nicholas N. <nj...@cs...> - 2005-03-27 18:09:48
|
On Sun, 27 Mar 2005, Jeremy Fitzhardinge wrote:
>> - We're relying heavily on dlsym(), which the 2.0.0 approach did not.
>
> I was hoping to drop all the magic symbol stuff altogether. I would prefer
> it if each tool .so had only a single special symbol to an init function;
> that init function would then call a core function passing a structure full
> of pointers to functions for all its tool functions. I don't really like all
> this implicit behaviour based on what symbols happen to exist. I intended
> the dlsym() stuff to be a backwards compatibility thing which would go away
> once all the tools had been converted.
I see. So in the same way tools register their 'tracking' functions, eg:
VG_(init_new_mem_startup) ( & ac_new_mem_startup );
VG_(init_new_mem_stack_signal) ( & ac_make_accessible );
they could do the same things for all the other functions, eg:
VG_(init_post_clo_init) ( & ac_post_clo_init );
VG_(init_instrument) ( & ac_instrument );
VG_(init_handle_client_request)( & ac_handle_client_request );
(That's slightly different your suggestion of passing a struct of function
pointers, but very similar.)
It's marginally more verbose, but you're right that it is better than all
the dlsym stuff. Good idea.
(Actually, we have an awkward mix now -- most of the SK_ functions are
done with dlsym, but the 'tracking' ones are done with function pointers.
Yuk.)
>> - It seems like a lot of the stuff in vg_toolint.c and vg_toolint.h is
>> global when it doesn't need to be. For example, the VG_(missing_*)() and
>> VG_(defined_*)() functions can be made local to vg_toolint.c. This is
>> probably just a matter of tweaking the auto-generation script
>> gen_toolint.pl.
>
> The defined_* ones are used all over the place from its use in the VG_TRACK()
> macro. missing_* need to be global because they're weak symbols which can be
> overridden. The default missing_* functions are in vg_toolint.c, but
> vg_default.c defines some replacements. This should probably be implemented
> by replacing pointers to functions rather than using linker tricks.
You're right about the defined_* ones, but the missing_* ones can be
removed. They don't need to be weak either. But it's all irrelevant if
we switch to using function pointers.
>> - Jeremy, in vg_pthreadmodel.c you call SK_(malloc)() -- why?
>
> The pthread_create code wants to allocate some memory in the client which the
> client can free via VG_USERREQ_FREE. I had to make a couple of small changes
> so this would work for both malloc-replacing and non-malloc-replacing tools.
Ah crap, I recently removed VG_USERREQ_{MALLOC,FREE} from the SVN repo
because they weren't being used.
But what is this memory that's being allocated by pthread_create() for?
Will the client have to use VG_USERREQ_FREE for the thread-modelling to
not leak memory? That would not be nice.
N
|
|
From: Jeremy F. <je...@go...> - 2005-03-28 00:22:16
|
Nicholas Nethercote wrote:
>>
>> I was hoping to drop all the magic symbol stuff altogether. I would
>> prefer it if each tool .so had only a single special symbol to an
>> init function; that init function would then call a core function
>> passing a structure full of pointers to functions for all its tool
>> functions. I don't really like all this implicit behaviour based on
>> what symbols happen to exist. I intended the dlsym() stuff to be a
>> backwards compatibility thing which would go away once all the tools
>> had been converted.
>
>
> I see. So in the same way tools register their 'tracking' functions, eg:
>
> VG_(init_new_mem_startup) ( & ac_new_mem_startup );
> VG_(init_new_mem_stack_signal) ( & ac_make_accessible );
>
> they could do the same things for all the other functions, eg:
>
> VG_(init_post_clo_init) ( & ac_post_clo_init );
> VG_(init_instrument) ( & ac_instrument );
> VG_(init_handle_client_request)( & ac_handle_client_request );
>
> (That's slightly different your suggestion of passing a struct of
> function pointers, but very similar.)
Yes, I think that's better because it removes the fragility of having a
structure as part of the interface.
[
As an aside, it's one of the things which makes OpenGL one of my
favorite APIs in terms of design. The API/ABI has no types more
complex than an array of scalars, which makes it very easy to have
multiple ABI-compatible implementations, and easy to maintain ABI
compatibility over many API revisions. OpenGL has lots of state,
but it is managed by lots of simple entrypoints which handle only
one aspect at a time (though some of those functions are generic
glGet(value_enum) form).
The tool interface is going to be a bit more complex, but the fewer
complex types the better if we actually want to achieve any kind of
long-term ABI stability.
]
> You're right about the defined_* ones, but the missing_* ones can be
> removed. They don't need to be weak either.
That's true if you've removed the USERREQ_MALLOC/FREE requests. The
missing_* weak-override stuff was used so that there is some
implementation for the MALLOC/FREE requests if the tool doesn't handle it.
>> The pthread_create code wants to allocate some memory in the client
>> which the client can free via VG_USERREQ_FREE. I had to make a
>> couple of small changes so this would work for both malloc-replacing
>> and non-malloc-replacing tools.
>
>
> Ah crap, I recently removed VG_USERREQ_{MALLOC,FREE} from the SVN repo
> because they weren't being used.
>
> But what is this memory that's being allocated by pthread_create()
> for? Will the client have to use VG_USERREQ_FREE for the
> thread-modelling to not leak memory? That would not be nice.
The basic problem is that the thread modelling code needs to know the
mapping between a libpthread thread identifier (pthread_t) and the
underlying Valgrind thread ID. The way I implement this is by
repointing the thread-start function to an intercept function in
vg_intercept.c which does a pthread_self() call; the pthread modelling
code wraps this pthread_self call and looks at the return value.
Because it knows what ThreadID is making the call, it can create the
mapping from pthread_t -> ThreadID.
This is implemented by allocating a small piece of memory (wrapper info,
known as vg_pthread_newthread_data) in the pthread_create "before"
wrapper function, which stores the original (start_func, arg) pair. It
then replaces the arguments to pthread_create()-proper with a
(start_func_wrapper, wrapper_info_ptr) pair. When the start_func_wrapper
gets called in the new thread, it calls pthread_self(), extracts the
original function and argument from the wrapper info, frees the wrapper
info and calls the original (*start_func)(arg).
By necessesity, the memory needs to be allocated within Valgrind, but
freed by the client code. We could add a special-case client callback
to free this specific piece of memory, but USERREQ_FREE seemed like the
right thing to use since it was there. Also, since this is allocated
memory being used by the VCPU, all its uses are subject to inspection by
the tool, so it has to all look OK to the tool (or suppressed around,
which definitely isn't nice).
J
|
|
From: Nicholas N. <nj...@cs...> - 2005-03-28 00:53:55
|
On Sun, 27 Mar 2005, Jeremy Fitzhardinge wrote: > The tool interface is going to be a bit more complex, but the fewer > complex types the better if we actually want to achieve any kind of > long-term ABI stability. Yep. > The basic problem is that the thread modelling code needs to know the mapping > between a libpthread thread identifier (pthread_t) and the underlying > Valgrind thread ID. The way I implement this is by repointing the > thread-start function to an intercept function in vg_intercept.c which does a > pthread_self() call; the pthread modelling code wraps this pthread_self call > and looks at the return value. Because it knows what ThreadID is making the > call, it can create the mapping from pthread_t -> ThreadID. > > This is implemented by allocating a small piece of memory (wrapper info, > known as vg_pthread_newthread_data) in the pthread_create "before" wrapper > function, which stores the original (start_func, arg) pair. It then replaces > the arguments to pthread_create()-proper with a (start_func_wrapper, > wrapper_info_ptr) pair. When the start_func_wrapper gets called in the new > thread, it calls pthread_self(), extracts the original function and argument > from the wrapper info, frees the wrapper info and calls the original > (*start_func)(arg). > > By necessesity, the memory needs to be allocated within Valgrind, but freed > by the client code. We could add a special-case client callback to free this > specific piece of memory, but USERREQ_FREE seemed like the right thing to use > since it was there. Also, since this is allocated memory being used by the > VCPU, all its uses are subject to inspection by the tool, so it has to all > look OK to the tool (or suppressed around, which definitely isn't nice). Hmm, I didn't really follow that... I'm not clear on what functions/bits of memory are from the client, what are from the pthreads implementation, and what are in Valgrind/pthreadmodel. Where would the USERREQ_FREE requrest be embedded in the client? Would every client have to have such a request in it? N |
|
From: Nicholas N. <nj...@cs...> - 2005-03-28 03:26:03
|
On Sun, 27 Mar 2005, Jeremy Fitzhardinge wrote:
>> VG_(init_post_clo_init) ( & ac_post_clo_init );
>> VG_(init_instrument) ( & ac_instrument );
>> VG_(init_handle_client_request)( & ac_handle_client_request );
>>
>> (That's slightly different your suggestion of passing a struct of function
>> pointers, but very similar.)
>
> Yes, I think that's better because it removes the fragility of having a
> structure as part of the interface.
The below patch against the current SVN HEAD (with changelog) is a first
step towards this. Comments are welcome -- Julian, how do you think it
looks doing it with functions rather than naked structs?
N
-----------------------------------------------------------------------------
This change reduces the number of calls to dlsym() when loading tools from a
lot to one. This required two basic changes:
1. Tools are responsible for telling the tool about any functions they
provide that the tool may call. This includes basic functions like
TL_(instrument)(), functions that assist core services such as
TL_(pp_Error)(), and malloc-replacement-related functions like
TL_(malloc)().
2. Tools that replace malloc now specify the size of the heap block redzones
through the VG_DETERMINE_INTERFACE_VERSION macro, rather than with a
variable VG_(vg_malloc_redzone_szB).
One consequence of these changes is that VG_(tool_init_dlsym)() no longer
needs to be generated by gen_toolint.pl.
There are a number of further improvements that could follow on from this one.
- Avoid the confusingly different definitions of the TL_() macro in the
core vs. for tools. Indeed, the functions provided by the tools now don't
need to use the TL_() macro at all, as they can have arbitrary names.
- Remove a lot of the auto-generated stuff in vg_toolint.c and vg_toolint.h
(indeed, it might be possible to not auto-generate these at all, which
would be nice).
- The handling of VgToolInterface is currently split across vg_needs.c and
vg_toolint.c, which isn't nice.
-----------------------------------------------------------------------------
Index: corecheck/cc_main.c
===================================================================
--- corecheck/cc_main.c (revision 3466)
+++ corecheck/cc_main.c (working copy)
@@ -40,13 +40,15 @@
"Copyright (C) 2002-2005, and GNU GPL'd, by Nicholas Nethercote.");
VG_(details_bug_reports_to) (VG_BUGS_TO);
+ VG_(basic_tool_funcs) (TL_(post_clo_init),
+ TL_(instrument),
+ TL_(fini));
+
VG_(needs_core_errors)();
/* No core events to track */
}
-VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 0)
-
void TL_(post_clo_init)(void)
{
}
@@ -61,6 +63,8 @@
{
}
+VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 0, 0)
+
/*--------------------------------------------------------------------*/
/*--- end cc_main.c ---*/
/*--------------------------------------------------------------------*/
Index: memcheck/mc_main.c
===================================================================
--- memcheck/mc_main.c (revision 3466)
+++ memcheck/mc_main.c (working copy)
@@ -1873,14 +1873,38 @@
VG_(details_bug_reports_to) (VG_BUGS_TO);
VG_(details_avg_translation_sizeB) ( 370 );
+ VG_(basic_tool_funcs) (TL_(post_clo_init),
+ TL_(instrument),
+ TL_(fini));
+
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));
VG_(needs_libc_freeres) ();
- VG_(needs_command_line_options)();
- VG_(needs_client_requests) ();
- VG_(needs_sanity_checks) ();
+ VG_(needs_command_line_options)(TL_(process_cmd_line_option),
+ TL_(print_usage),
+ TL_(print_debug_usage));
+ VG_(needs_client_requests) (TL_(handle_client_request));
+ VG_(needs_sanity_checks) (TL_(cheap_sanity_check),
+ TL_(expensive_sanity_check));
VG_(needs_shadow_memory) ();
+ VG_(malloc_funcs) (TL_(malloc),
+ TL_(__builtin_new),
+ TL_(__builtin_vec_new),
+ TL_(memalign),
+ TL_(calloc),
+ TL_(free),
+ TL_(__builtin_delete),
+ TL_(__builtin_vec_delete),
+ TL_(realloc) );
+
MAC_( new_mem_heap) = & mc_new_mem_heap;
MAC_( ban_mem_heap) = & mc_make_noaccess;
MAC_(copy_mem_heap) = & mc_copy_address_range_state;
@@ -1951,7 +1975,7 @@
}
}
-VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 9./8)
+VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 9./8, MALLOC_REDZONE_SZB)
/*--------------------------------------------------------------------*/
/*--- end mc_main.c ---*/
Index: memcheck/mac_malloc_wrappers.c
===================================================================
--- memcheck/mac_malloc_wrappers.c (revision 3466)
+++ memcheck/mac_malloc_wrappers.c (working copy)
@@ -41,9 +41,6 @@
static SizeT cmalloc_n_frees = 0;
static SizeT cmalloc_bs_mallocd = 0;
-/* We want a 16B redzone on heap blocks for Addrcheck and Memcheck */
-SizeT VG_(vg_malloc_redzone_szB) = 16;
-
/* Function pointers for the two tools to track interesting events. */
void (*MAC_(new_mem_heap)) ( Addr a, SizeT len, Bool is_inited ) = NULL;
void (*MAC_(ban_mem_heap)) ( Addr a, SizeT len ) = NULL;
@@ -221,7 +218,7 @@
return NULL;
} else {
return MAC_(new_block) ( tid, 0, n, VG_(clo_alignment),
- VG_(vg_malloc_redzone_szB), /*is_zeroed*/False, MAC_AllocMalloc,
+ MALLOC_REDZONE_SZB, /*is_zeroed*/False, MAC_AllocMalloc,
MAC_(malloc_list));
}
}
@@ -232,7 +229,7 @@
return NULL;
} else {
return MAC_(new_block) ( tid, 0, n, VG_(clo_alignment),
- VG_(vg_malloc_redzone_szB), /*is_zeroed*/False, MAC_AllocNew,
+ MALLOC_REDZONE_SZB, /*is_zeroed*/False, MAC_AllocNew,
MAC_(malloc_list));
}
}
@@ -243,7 +240,7 @@
return NULL;
} else {
return MAC_(new_block) ( tid, 0, n, VG_(clo_alignment),
- VG_(vg_malloc_redzone_szB), /*is_zeroed*/False, MAC_AllocNewVec,
+ MALLOC_REDZONE_SZB, /*is_zeroed*/False, MAC_AllocNewVec,
MAC_(malloc_list));
}
}
@@ -254,7 +251,7 @@
return NULL;
} else {
return MAC_(new_block) ( tid, 0, n, align,
- VG_(vg_malloc_redzone_szB), /*is_zeroed*/False, MAC_AllocMalloc,
+ MALLOC_REDZONE_SZB, /*is_zeroed*/False, MAC_AllocMalloc,
MAC_(malloc_list));
}
}
@@ -265,7 +262,7 @@
return NULL;
} else {
return MAC_(new_block) ( tid, 0, nmemb*size1, VG_(clo_alignment),
- VG_(vg_malloc_redzone_szB), /*is_zeroed*/True, MAC_AllocMalloc,
+ MALLOC_REDZONE_SZB, /*is_zeroed*/True, MAC_AllocMalloc,
MAC_(malloc_list));
}
}
@@ -326,19 +323,19 @@
void TL_(free) ( ThreadId tid, void* p )
{
MAC_(handle_free)(
- tid, (Addr)p, VG_(vg_malloc_redzone_szB), MAC_AllocMalloc );
+ tid, (Addr)p, MALLOC_REDZONE_SZB, MAC_AllocMalloc );
}
void TL_(__builtin_delete) ( ThreadId tid, void* p )
{
MAC_(handle_free)(
- tid, (Addr)p, VG_(vg_malloc_redzone_szB), MAC_AllocNew);
+ tid, (Addr)p, MALLOC_REDZONE_SZB, MAC_AllocNew);
}
void TL_(__builtin_vec_delete) ( ThreadId tid, void* p )
{
MAC_(handle_free)(
- tid, (Addr)p, VG_(vg_malloc_redzone_szB), MAC_AllocNewVec);
+ tid, (Addr)p, MALLOC_REDZONE_SZB, MAC_AllocNewVec);
}
void* TL_(realloc) ( ThreadId tid, void* p, SizeT new_size )
@@ -397,19 +394,17 @@
/* First half kept and copied, second half new,
red zones as normal */
- MAC_(ban_mem_heap) ( p_new-VG_(vg_malloc_redzone_szB),
- VG_(vg_malloc_redzone_szB) );
+ MAC_(ban_mem_heap) ( p_new-MALLOC_REDZONE_SZB, MALLOC_REDZONE_SZB );
MAC_(copy_mem_heap)( (Addr)p, p_new, mc->size );
MAC_(new_mem_heap) ( p_new+mc->size, new_size-mc->size, /*inited*/False );
- MAC_(ban_mem_heap) ( p_new+new_size, VG_(vg_malloc_redzone_szB) );
+ MAC_(ban_mem_heap) ( p_new+new_size, MALLOC_REDZONE_SZB );
/* Copy from old to new */
for (i = 0; i < mc->size; i++)
((UChar*)p_new)[i] = ((UChar*)p)[i];
/* Free old memory */
- die_and_free_mem ( tid, mc, prev_chunks_next_ptr,
- VG_(vg_malloc_redzone_szB) );
+ die_and_free_mem ( tid, mc, prev_chunks_next_ptr, MALLOC_REDZONE_SZB );
/* this has to be after die_and_free_mem, otherwise the
former succeeds in shorting out the new block, not the
Index: memcheck/mac_shared.h
===================================================================
--- memcheck/mac_shared.h (revision 3466)
+++ memcheck/mac_shared.h (working copy)
@@ -290,6 +290,8 @@
extern void MAC_(print_common_usage) ( void );
extern void MAC_(print_common_debug_usage) ( void );
+/* We want a 16B redzone on heap blocks for Addrcheck and Memcheck */
+#define MALLOC_REDZONE_SZB 16
/*------------------------------------------------------------*/
/*--- Variables ---*/
Index: massif/ms_main.c
===================================================================
--- massif/ms_main.c (revision 3466)
+++ massif/ms_main.c (working copy)
@@ -1159,8 +1159,6 @@
// Current directory at startup.
static Char* base_dir;
-SizeT VG_(vg_malloc_redzone_szB) = 0;
-
void TL_(pre_clo_init)()
{
VG_(details_name) ("Massif");
@@ -1169,11 +1167,29 @@
VG_(details_copyright_author)("Copyright (C) 2003, Nicholas Nethercote");
VG_(details_bug_reports_to) (VG_BUGS_TO);
+ // Basic functions
+ VG_(basic_tool_funcs) (TL_(post_clo_init),
+ TL_(instrument),
+ TL_(fini));
+
// Needs
VG_(needs_libc_freeres)();
- VG_(needs_command_line_options)();
- VG_(needs_client_requests) ();
+ VG_(needs_command_line_options)(TL_(process_cmd_line_option),
+ TL_(print_usage),
+ TL_(print_debug_usage));
+ VG_(needs_client_requests) (TL_(handle_client_request));
+ // Malloc replacement
+ VG_(malloc_funcs) (TL_(malloc),
+ TL_(__builtin_new),
+ TL_(__builtin_vec_new),
+ TL_(memalign),
+ TL_(calloc),
+ TL_(free),
+ TL_(__builtin_delete),
+ TL_(__builtin_vec_delete),
+ TL_(realloc) );
+
// Events to track
VG_(init_new_mem_stack_signal) ( new_mem_stack_signal );
VG_(init_die_mem_stack_signal) ( die_mem_stack_signal );
@@ -1812,7 +1828,7 @@
print_summary ( total_ST, heap_ST, heap_admin_ST, stack_ST );
}
-VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 0)
+VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 0, 0)
/*--------------------------------------------------------------------*/
/*--- end ms_main.c ---*/
Index: include/tool.h.base
===================================================================
--- include/tool.h.base (revision 3466)
+++ include/tool.h.base (working copy)
@@ -107,16 +107,21 @@
/* Specifies how big the shadow segment should be as a ratio to the
client address space. 0 for no shadow segment. */
float shadow_ratio;
+
+ /* Specifies how big the redzone surrounding each heap block should be.
+ Only relevant for tools that replace malloc() et al. */
+ UInt malloc_redzone_szB;
} ToolInfo;
/* Every tool must include this macro somewhere, exactly once. */
-#define VG_DETERMINE_INTERFACE_VERSION(pre_clo_init, shadow) \
+#define VG_DETERMINE_INTERFACE_VERSION(pre_clo_init, ratio, rz_szB) \
const ToolInfo TL_(tool_info) = { \
.sizeof_ToolInfo = sizeof(ToolInfo), \
.interface_major_version = VG_CORE_INTERFACE_MAJOR_VERSION, \
.interface_minor_version = VG_CORE_INTERFACE_MINOR_VERSION, \
.tl_pre_clo_init = pre_clo_init, \
- .shadow_ratio = shadow, \
+ .shadow_ratio = ratio, \
+ .malloc_redzone_szB = rz_szB, \
};
/*====================================================================*/
@@ -908,15 +913,6 @@
follow the following instructions. You can do it from scratch,
though, if you enjoy that sort of thing. */
-/* Arena size for valgrind's own malloc(); default value is 0, but can
- be overridden by tool -- but must be done so *statically*, eg:
-
- SizeT VG_(vg_malloc_redzone_szB) = 4;
-
- It can't be done from a function like TL_(pre_clo_init)(). So it can't,
- for example, be controlled with a command line option, unfortunately. */
-extern SizeT VG_(vg_malloc_redzone_szB);
-
/* Can be called from TL_(malloc) et al to do the actual alloc/freeing. */
extern void* VG_(cli_malloc) ( SizeT align, SizeT nbytes );
extern void VG_(cli_free) ( void* p );
@@ -947,6 +943,16 @@
/*====================================================================*/
/* ------------------------------------------------------------------ */
+/* Basic tool functions */
+
+extern void VG_(basic_tool_funcs)(
+ void(*post_clo_init)(void),
+ IRBB*(*instrument)(IRBB* bb_in, VexGuestLayout* layout,
+ IRType gWordTy, IRType hWordTy ),
+ void(*fini)(Int)
+);
+
+/* ------------------------------------------------------------------ */
/* Details */
/* Default value for avg_translations_sizeB (in bytes), indicating typical
@@ -995,27 +1001,110 @@
lot like being a member of a type class. */
/* Want to report errors from tool? This implies use of suppressions, too. */
-extern void VG_(needs_tool_errors) ( void );
+extern void VG_(needs_tool_errors) (
+ // Identify if two errors are equal, or equal enough. `res' indicates how
+ // close is "close enough". `res' should be passed on as necessary, eg. if
+ // the Error's `extra' part contains an ExeContext, `res' should be
+ // passed to VG_(eq_ExeContext)() if the ExeContexts are considered. Other
+ // than that, probably don't worry about it unless you have lots of very
+ // similar errors occurring.
+ Bool (*eq_Error)(VgRes res, Error* e1, Error* e2),
+ // Print error context.
+ void (*pp_Error)(Error* err),
+
+ // Should fill in any details that could be postponed until after the
+ // decision whether to ignore the error (ie. details not affecting the
+ // result of TL_(eq_Error)()). This saves time when errors are ignored.
+ // Yuk.
+ // Return value: must be the size of the `extra' part in bytes -- used by
+ // the core to make a copy.
+ UInt (*update_extra)(Error* err),
+
+ // Return value indicates recognition. If recognised, must set skind using
+ // VG_(set_supp_kind)().
+ Bool (*recognised_suppression)(Char* name, Supp* su),
+
+ // Read any extra info for this suppression kind. Most likely for filling
+ // in the `extra' and `string' parts (with VG_(set_supp_{extra, string})())
+ // of a suppression if necessary. Should return False if a syntax error
+ // occurred, True otherwise.
+ Bool (*read_extra_suppression_info)(Int fd, Char* buf, Int nBuf, Supp* su),
+
+ // This should just check the kinds match and maybe some stuff in the
+ // `string' and `extra' field if appropriate (using VG_(get_supp_*)() to
+ // get the relevant suppression parts).
+ Bool (*error_matches_suppression)(Error* err, Supp* su),
+
+ // This should return the suppression name, for --gen-suppressions, or NULL
+ // if that error type cannot be suppressed. This is the inverse of
+ // TL_(recognised_suppression)().
+ Char* (*get_error_name)(Error* err),
+
+ // This should print any extra info for the error, for --gen-suppressions,
+ // including the newline. This is the inverse of
+ // TL_(read_extra_suppression_info)().
+ void (*print_extra_suppression_info)(Error* err)
+);
+
+
/* Is information kept about specific individual basic blocks? (Eg. for
cachegrind there are cost-centres for every instruction, stored at a
basic block level.) If so, it sometimes has to be discarded, because
.so mmap/munmap-ping or self-modifying code (informed by the
DISCARD_TRANSLATIONS user request) can cause one instruction address
to be used for more than one instruction in one program run... */
-extern void VG_(needs_basic_block_discards) ( void );
+extern void VG_(needs_basic_block_discards) (
+ // Should discard any information that pertains to specific basic blocks
+ // or instructions within the address range given.
+ void (*discard_basic_block_info)(Addr a, SizeT size)
+);
/* Tool defines its own command line options? */
-extern void VG_(needs_command_line_options) ( void );
+extern void VG_(needs_command_line_options) (
+ // Return True if option was recognised. Presumably sets some state to
+ // record the option as well.
+ Bool (*process_cmd_line_option)(Char* argv),
+ // Print out command line usage for options for normal tool operation.
+ void (*print_usage)(void),
+
+ // Print out command line usage for options for debugging the tool.
+ void (*print_debug_usage)(void)
+);
+
/* Tool defines its own client requests? */
-extern void VG_(needs_client_requests) ( void );
+extern void VG_(needs_client_requests) (
+ // If using client requests, the number of the first request should be equal
+ // to VG_USERREQ_TOOL_BASE('X', 'Y'), where 'X' and 'Y' form a suitable two
+ // character identification for the string. The second and subsequent
+ // requests should follow.
+ //
+ // This function should use the VG_IS_TOOL_USERREQ macro (in
+ // include/valgrind.h) to first check if it's a request for this tool. Then
+ // should handle it if it's recognised (and return True), or return False if
+ // not recognised. arg_block[0] holds the request number, any further args
+ // from the request are in arg_block[1..]. 'ret' is for the return value...
+ // it should probably be filled, if only with 0.
+ Bool (*handle_client_request)(ThreadId tid, UWord* arg_block, UWord* ret)
+);
/* Tool does stuff before and/or after system calls? */
-extern void VG_(needs_syscall_wrapper) ( void );
+// Nb: If either of the pre_ functions malloc() something to return, the
+// corresponding post_ function had better free() it!
+extern void VG_(needs_syscall_wrapper) (
+ void (* pre_syscall)(ThreadId tid, UInt syscallno),
+ void (*post_syscall)(ThreadId tid, UInt syscallno, Int res)
+);
/* Are tool-state sanity checks performed? */
-extern void VG_(needs_sanity_checks) ( void );
+// Can be useful for ensuring a tool's correctness. TL_(cheap_sanity_check)
+// is called very frequently; TL_(expensive_sanity_check) is called less
+// frequently and can be more involved.
+extern void VG_(needs_sanity_checks) (
+ Bool(*cheap_sanity_check)(void),
+ Bool(*expensive_sanity_check)(void)
+);
/* Do we need to see data symbols? */
extern void VG_(needs_data_syms) ( void );
@@ -1028,6 +1117,22 @@
extern float TL_(shadow_ratio);
/* ------------------------------------------------------------------ */
+/* Malloc replacement */
+
+// The 'v' prefix avoids GCC complaints about overshadowing global names.
+extern void VG_(malloc_funcs)(
+ void* (*vmalloc) ( ThreadId tid, SizeT n ),
+ void* (*v__builtin_new) ( ThreadId tid, SizeT n ),
+ void* (*v__builtin_vec_new) ( ThreadId tid, SizeT n ),
+ void* (*vmemalign) ( ThreadId tid, SizeT align, SizeT n ),
+ void* (*vcalloc) ( ThreadId tid, SizeT nmemb, SizeT size1 ),
+ void (*vfree) ( ThreadId tid, void* p ),
+ void (*v__builtin_delete) ( ThreadId tid, void* p ),
+ void (*v__builtin_vec_delete) ( ThreadId tid, void* p ),
+ void* (*vrealloc) ( ThreadId tid, void* p, SizeT new_size )
+);
+
+/* ------------------------------------------------------------------ */
/* Core events to track */
/* Part of the core from which this call was made. Useful for determining
Index: cachegrind/cg_main.c
===================================================================
--- cachegrind/cg_main.c (revision 3466)
+++ cachegrind/cg_main.c (working copy)
@@ -1132,9 +1132,15 @@
VG_(details_bug_reports_to) (VG_BUGS_TO);
VG_(details_avg_translation_sizeB) ( 155 );
- VG_(needs_basic_block_discards)();
- VG_(needs_command_line_options)();
+ VG_(basic_tool_funcs) (TL_(post_clo_init),
+ TL_(instrument),
+ TL_(fini));
+ VG_(needs_basic_block_discards)(TL_(discard_basic_block_info));
+ VG_(needs_command_line_options)(TL_(process_cmd_line_option),
+ TL_(print_usage),
+ TL_(print_debug_usage));
+
/* Get working directory */
tl_assert( VG_(getcwd_alloc)(&base_dir) );
@@ -1162,7 +1168,7 @@
VG_(register_profile_event)(VgpCacheResults, "cache-results");
}
-VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 0)
+VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 0, 0)
/*--------------------------------------------------------------------*/
/*--- end cg_main.c ---*/
Index: none/nl_main.c
===================================================================
--- none/nl_main.c (revision 3466)
+++ none/nl_main.c (working copy)
@@ -39,6 +39,10 @@
"Copyright (C) 2002-2005, and GNU GPL'd, by Nicholas Nethercote.");
VG_(details_bug_reports_to) (VG_BUGS_TO);
+ VG_(basic_tool_funcs) (TL_(post_clo_init),
+ TL_(instrument),
+ TL_(fini));
+
/* No needs, no core events to track */
}
@@ -56,7 +60,7 @@
{
}
-VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 0)
+VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 0, 0)
/*--------------------------------------------------------------------*/
/*--- end ---*/
Index: addrcheck/ac_main.c
===================================================================
--- addrcheck/ac_main.c (revision 3466)
+++ addrcheck/ac_main.c (working copy)
@@ -1318,14 +1318,38 @@
VG_(details_bug_reports_to) (VG_BUGS_TO);
VG_(details_avg_translation_sizeB) ( 135 );
+ VG_(basic_tool_funcs) (TL_(post_clo_init),
+ TL_(instrument),
+ TL_(fini));
+
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));
VG_(needs_libc_freeres) ();
- VG_(needs_command_line_options)();
- VG_(needs_client_requests) ();
- VG_(needs_sanity_checks) ();
+ VG_(needs_command_line_options)(TL_(process_cmd_line_option),
+ TL_(print_usage),
+ TL_(print_debug_usage));
+ VG_(needs_client_requests) (TL_(handle_client_request));
+ VG_(needs_sanity_checks) (TL_(cheap_sanity_check),
+ TL_(expensive_sanity_check));
VG_(needs_shadow_memory) ();
+ VG_(malloc_funcs) (TL_(malloc),
+ TL_(__builtin_new),
+ TL_(__builtin_vec_new),
+ TL_(memalign),
+ TL_(calloc),
+ TL_(free),
+ TL_(__builtin_delete),
+ TL_(__builtin_vec_delete),
+ TL_(realloc) );
+
MAC_( new_mem_heap) = & ac_new_mem_heap;
MAC_( ban_mem_heap) = & ac_make_noaccess;
MAC_(copy_mem_heap) = & ac_copy_address_range_state;
@@ -1381,7 +1405,7 @@
MAC_(common_fini)( ac_detect_memory_leaks );
}
-VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 1./8)
+VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 1./8, MALLOC_REDZONE_SZB)
/*--------------------------------------------------------------------*/
Index: lackey/lk_main.c
===================================================================
--- lackey/lk_main.c (revision 3466)
+++ lackey/lk_main.c (working copy)
@@ -81,6 +81,10 @@
"Copyright (C) 2002-2005, and GNU GPL'd, by Nicholas Nethercote.");
VG_(details_bug_reports_to) (VG_BUGS_TO);
VG_(details_avg_translation_sizeB) ( 175 );
+
+ VG_(basic_tool_funcs) (TL_(post_clo_init),
+ TL_(instrument),
+ TL_(fini));
}
void TL_(post_clo_init)(void)
@@ -278,7 +282,7 @@
VG_(message)(Vg_UserMsg, "Exit code: %d", exitcode);
}
-VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 0)
+VG_DETERMINE_INTERFACE_VERSION(TL_(pre_clo_init), 0, 0)
/*--------------------------------------------------------------------*/
Index: coregrind/gen_toolint.pl
===================================================================
--- coregrind/gen_toolint.pl (revision 3466)
+++ coregrind/gen_toolint.pl (working copy)
@@ -166,32 +166,6 @@
print "void VG_(init_$func)($ret (*func)($args));\n";
};
$headerguard=$output;
-} elsif ($output eq "initdlsym") {
- $pre = sub () {
- print <<EOF;
-#include <dlfcn.h>
-void VG_(tool_init_dlsym)(void *dlhandle)
-{
- void *ret;
-
-EOF
- };
- $post = sub () {
- print "}\n";
- };
- $generate = sub ($$$@) {
- my ($pfx, $ret, $func, @args) = @_;
- my $args = join ", ", getargtypes(@args);
-
- print <<EOF;
- ret = dlsym(dlhandle, "vgTool_$func");
- if (ret != NULL)
- VG_(init_$func)(($ret (*)($args))ret);
-
-EOF
- };
-
- $passcomment = 0;
}
die "Unknown output format \"$output\"" unless defined $generate;
Index: coregrind/vg_default.c
===================================================================
--- coregrind/vg_default.c (revision 3466)
+++ coregrind/vg_default.c (working copy)
@@ -66,10 +66,6 @@
/*--- Replacing malloc et al ---*/
/*------------------------------------------------------------*/
-/* Default redzone size for CLIENT arena of Valgrind's malloc() */
-__attribute__ ((weak))
-SizeT VG_(vg_malloc_redzone_szB) = 8;
-
Bool VG_(tl_malloc_called_deliberately) = False;
/* If the tool hasn't replaced malloc(), this one can be called
Index: coregrind/vg_main.c
===================================================================
--- coregrind/vg_main.c (revision 3466)
+++ coregrind/vg_main.c (working copy)
@@ -1175,7 +1175,7 @@
/* Find and load a tool, and check it looks ok. Also looks to see if there's
* a matching vgpreload_*.so file, and returns its name in *preloadpath. */
-static void load_tool( const char *toolname, void** handle_out,
+static void load_tool( const char *toolname,
ToolInfo** toolinfo_out, char **preloadpath_out )
{
Bool ok;
@@ -1184,7 +1184,6 @@
void* handle;
ToolInfo* toolinfo;
char* preloadpath = NULL;
- Int* vg_malloc_redzonep;
// XXX: allowing full paths for --tool option -- does it make sense?
// Doesn't allow for vgpreload_<tool>.so.
@@ -1244,13 +1243,9 @@
}
// Set redzone size for V's allocator
- vg_malloc_redzonep = dlsym(handle, VG_STRINGIFY(VG_(vg_malloc_redzone_szB)));
- if ( NULL != vg_malloc_redzonep ) {
- VG_(vg_malloc_redzone_szB) = *vg_malloc_redzonep;
- }
+ VG_(vg_malloc_redzone_szB) = toolinfo->malloc_redzone_szB;
- vg_assert(NULL != handle && NULL != toolinfo);
- *handle_out = handle;
+ vg_assert(NULL != toolinfo);
*toolinfo_out = toolinfo;
*preloadpath_out = preloadpath;
return;
@@ -2422,7 +2417,6 @@
Int need_help = 0; // 0 = no, 1 = --help, 2 = --help-debug
struct exeinfo info;
ToolInfo *toolinfo = NULL;
- void *tool_dlhandle;
Addr client_eip;
Addr sp_at_startup; /* client's SP at the point we gained control. */
UInt * client_auxv;
@@ -2495,7 +2489,7 @@
// p: set-libdir [for VG_(libdir)]
// p: pre_process_cmd_line_options() [for 'tool']
//--------------------------------------------------------------
- load_tool(tool, &tool_dlhandle, &toolinfo, &preload);
+ load_tool(tool, &toolinfo, &preload);
//==============================================================
// Can use VG_(malloc)() and VG_(arena_malloc)() only after load_tool()
@@ -2572,13 +2566,12 @@
//--------------------------------------------------------------
// Init tool: pre_clo_init, process cmd line, post_clo_init
// p: setup_client_stack() [for 'VG_(client_arg[cv]']
- // p: load_tool() [for 'tool']
+ // p: load_tool() [for 'toolinfo']
// p: setup_file_descriptors() [for 'VG_(fd_xxx_limit)']
// p: parse_procselfmaps [so VG segments are setup so tool can
// call VG_(malloc)]
//--------------------------------------------------------------
(*toolinfo->tl_pre_clo_init)();
- VG_(tool_init_dlsym)(tool_dlhandle);
VG_(sanity_check_needs)();
// If --tool and --help/--help-debug was given, now give the core+tool
Index: coregrind/vg_malloc2.c
===================================================================
--- coregrind/vg_malloc2.c (revision 3466)
+++ coregrind/vg_malloc2.c (working copy)
@@ -329,6 +329,9 @@
// The arena structures themselves.
static Arena vg_arena[VG_N_ARENAS];
+// Redzone size for CLIENT arena of Valgrind's malloc(). Set in vg_main.c.
+SizeT VG_(vg_malloc_redzone_szB);
+
// Functions external to this module identify arenas using ArenaIds,
// not Arena*s. This fn converts the former to the latter.
static Arena* arenaId_to_ArenaP ( ArenaId arena )
Index: coregrind/vg_needs.c
===================================================================
--- coregrind/vg_needs.c (revision 3466)
+++ coregrind/vg_needs.c (working copy)
@@ -36,6 +36,23 @@
Tool data structure initialisation
------------------------------------------------------------------ */
+/*--------------------------------------------------------------------*/
+/* Setting basic functions */
+
+void VG_(basic_tool_funcs)(
+ void(*post_clo_init)(void),
+ IRBB*(*instrument)(IRBB*, VexGuestLayout*, IRType, IRType ),
+ void(*fini)(Int)
+)
+{
+ VG_(tool_interface).tool_post_clo_init = post_clo_init;
+ VG_(tool_interface).tool_instrument = instrument;
+ VG_(tool_interface).tool_fini = fini;
+}
+
+/*--------------------------------------------------------------------*/
+/* Setting details */
+
/* Init with default values. */
VgDetails VG_(details) = {
.name = NULL,
@@ -46,6 +63,23 @@
.avg_translation_sizeB = VG_DEFAULT_TRANS_SIZEB,
};
+/* Use macro because they're so repetitive */
+#define DETAILS(type, detail) \
+ extern void VG_(details_##detail)(type detail) \
+ { \
+ VG_(details).detail = detail; \
+ }
+
+DETAILS(Char*, name)
+DETAILS(Char*, version)
+DETAILS(Char*, description)
+DETAILS(Char*, copyright_author)
+DETAILS(Char*, bug_reports_to)
+DETAILS(UInt, avg_translation_sizeB)
+
+/*--------------------------------------------------------------------*/
+/* Setting needs */
+
VgNeeds VG_(needs) = {
.core_errors = False,
.tool_errors = False,
@@ -115,44 +149,115 @@
#undef CHECK_NOT
}
-/*--------------------------------------------------------------------*/
-/* Setting details */
-
/* Use macro because they're so repetitive */
-#define DETAILS(type, detail) \
- extern void VG_(details_##detail)(type detail) \
- { \
- VG_(details).detail = detail; \
- }
-
-DETAILS(Char*, name)
-DETAILS(Char*, version)
-DETAILS(Char*, description)
-DETAILS(Char*, copyright_author)
-DETAILS(Char*, bug_reports_to)
-DETAILS(UInt, avg_translation_sizeB)
-
-/*--------------------------------------------------------------------*/
-/* Setting needs */
-
-/* Use macro because they're so repetitive */
#define NEEDS(need) \
extern void VG_(needs_##need)(void) \
{ \
VG_(needs).need = True; \
}
+// These ones don't require any tool-supplied functions
NEEDS(libc_freeres)
NEEDS(core_errors)
-NEEDS(tool_errors)
-NEEDS(basic_block_discards)
-NEEDS(command_line_options)
-NEEDS(client_requests)
-NEEDS(syscall_wrapper)
-NEEDS(sanity_checks)
NEEDS(data_syms)
NEEDS(shadow_memory)
+void VG_(needs_basic_block_discards)(
+ void (*discard)(Addr, SizeT)
+)
+{
+ VG_(needs).basic_block_discards = True;
+ VG_(tool_interface).tool_discard_basic_block_info = discard;
+}
+
+void VG_(needs_tool_errors)(
+ Bool (*eq) (VgRes, Error*, Error*),
+ void (*pp) (Error*),
+ UInt (*update) (Error*),
+ Bool (*recog) (Char*, Supp*),
+ Bool (*read_extra) (Int, Char*, Int, Supp*),
+ Bool (*matches) (Error*, Supp*),
+ Char* (*name) (Error*),
+ void (*print_extra)(Error*)
+)
+{
+ VG_(needs).tool_errors = True;
+ VG_(tool_interface).tool_eq_Error = eq;
+ VG_(tool_interface).tool_pp_Error = pp;
+ VG_(tool_interface).tool_update_extra = update;
+ VG_(tool_interface).tool_recognised_suppression = recog;
+ VG_(tool_interface).tool_read_extra_suppression_info = read_extra;
+ VG_(tool_interface).tool_error_matches_suppression = matches;
+ VG_(tool_interface).tool_get_error_name = name;
+ VG_(tool_interface).tool_print_extra_suppression_info = print_extra;
+}
+
+void VG_(needs_command_line_options)(
+ Bool (*process)(Char*),
+ void (*usage)(void),
+ void (*debug_usage)(void)
+)
+{
+ VG_(needs).command_line_options = True;
+ VG_(tool_interface).tool_process_cmd_line_option = process;
+ VG_(tool_interface).tool_print_usage = usage;
+ VG_(tool_interface).tool_print_debug_usage = debug_usage;
+}
+
+void VG_(needs_client_requests)(
+ Bool (*handle)(ThreadId, UWord*, UWord*)
+)
+{
+ VG_(needs).client_requests = True;
+ VG_(tool_interface).tool_handle_client_request = handle;
+}
+
+void VG_(needs_syscall_wrapper)(
+ void(*pre) (ThreadId, UInt),
+ void(*post)(ThreadId, UInt, Int res)
+)
+{
+ VG_(needs).syscall_wrapper = True;
+ VG_(tool_interface).tool_pre_syscall = pre;
+ VG_(tool_interface).tool_post_syscall = post;
+}
+
+void VG_(needs_sanity_checks)(
+ Bool(*cheap)(void),
+ Bool(*expen)(void)
+)
+{
+ VG_(needs).sanity_checks = True;
+ VG_(tool_interface).tool_cheap_sanity_check = cheap;
+ VG_(tool_interface).tool_expensive_sanity_check = expen;
+}
+
+
+/* Replacing malloc() */
+
+extern void VG_(malloc_funcs)(
+ void* (*malloc) ( ThreadId, SizeT ),
+ void* (*__builtin_new) ( ThreadId, SizeT ),
+ void* (*__builtin_vec_new) ( ThreadId, SizeT ),
+ void* (*memalign) ( ThreadId, SizeT, SizeT ),
+ void* (*calloc) ( ThreadId, SizeT, SizeT ),
+ void (*free) ( ThreadId, void* ),
+ void (*__builtin_delete) ( ThreadId, void* ),
+ void (*__builtin_vec_delete) ( ThreadId, void* ),
+ void* (*realloc) ( ThreadId, void*, SizeT )
+)
+{
+ VG_(tool_interface).malloc_malloc = malloc;
+ VG_(tool_interface).malloc___builtin_new = __builtin_new;
+ VG_(tool_interface).malloc___builtin_vec_new = __builtin_vec_new;
+ VG_(tool_interface).malloc_memalign = memalign;
+ VG_(tool_interface).malloc_calloc = calloc;
+ VG_(tool_interface).malloc_free = free;
+ VG_(tool_interface).malloc___builtin_delete = __builtin_delete;
+ VG_(tool_interface).malloc___builtin_vec_delete = __builtin_vec_delete;
+ VG_(tool_interface).malloc_realloc = realloc;
+}
+
/*--------------------------------------------------------------------*/
/*--- end vg_needs.c ---*/
/*--------------------------------------------------------------------*/
Index: coregrind/core.h
===================================================================
--- coregrind/core.h (revision 3466)
+++ coregrind/core.h (working copy)
@@ -337,8 +337,6 @@
extern VgNeeds VG_(needs);
-extern void VG_(tool_init_dlsym)(void *dlhandle);
-
#include "vg_toolint.h"
@@ -386,6 +384,12 @@
// Round-up size for --sloppy-malloc=yes.
#define VG_SLOPPY_MALLOC_SZB 4
+// Arena size for valgrind's own malloc(). Gets set by the 3rd parameter
+// to the VG_DETERMINE_INTERFACE_VERSION macro, because it has to be set before
+// any of the tool's functions get called. So it can't, for example, be
+// controlled with a command line option, unfortunately. */
+extern SizeT VG_(vg_malloc_redzone_szB);
+
extern void* VG_(arena_malloc) ( ArenaId arena, SizeT nbytes );
extern void VG_(arena_free) ( ArenaId arena, void* ptr );
extern void* VG_(arena_calloc) ( ArenaId arena,
Index: coregrind/Makefile.am
===================================================================
--- coregrind/Makefile.am (revision 3466)
+++ coregrind/Makefile.am (working copy)
@@ -124,7 +124,6 @@
$(PERL) $(srcdir)/gen_toolint.pl callwrap < $(srcdir)/toolfuncs.def > $@ || rm -f $@
$(PERL) $(srcdir)/gen_toolint.pl missingfuncs < $(srcdir)/toolfuncs.def >> $@ || rm -f $@
$(PERL) $(srcdir)/gen_toolint.pl initfunc < $(srcdir)/toolfuncs.def >> $@ || rm -f $@
- $(PERL) $(srcdir)/gen_toolint.pl initdlsym < $(srcdir)/toolfuncs.def >> $@ || rm -f $@
$(PERL) $(srcdir)/gen_toolint.pl structdef < $(srcdir)/toolfuncs.def >> $@ || rm -f $@
vg_toolint.h: $(srcdir)/gen_toolint.pl $(srcdir)/toolfuncs.def ./Makefile
|
|
From: Jeremy F. <je...@go...> - 2005-03-28 08:59:43
|
Nicholas Nethercote wrote:
> 2. Tools that replace malloc now specify the size of the heap block
> redzones
> through the VG_DETERMINE_INTERFACE_VERSION macro, rather than with a
> variable VG_(vg_malloc_redzone_szB).
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.
> + 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. 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);
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).
J
|
|
From: Jeremy F. <je...@go...> - 2005-03-28 12:03:57
|
Jeremy Fitzhardinge wrote:
> 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.
(Particularly when one's mail program mangles the formatting. But you
get the point.)
J
|
|
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
|
|
From: Julian S. <js...@ac...> - 2005-03-27 19:10:28
|
> > I was hoping to drop all the magic symbol stuff altogether. I would > > prefer it if each tool .so had only a single special symbol to an init > > function; that init function would then call a core function passing a > > structure full of pointers to functions for all its tool functions. That sounds great to me. > I see. So in the same way tools register their 'tracking' functions, eg: > > VG_(init_new_mem_startup) ( & ac_new_mem_startup ); > VG_(init_new_mem_stack_signal) ( & ac_make_accessible ); > > they could do the same things for all the other functions, eg: > > VG_(init_post_clo_init) ( & ac_post_clo_init ); > VG_(init_instrument) ( & ac_instrument ); > VG_(init_handle_client_request)( & ac_handle_client_request ); > > (That's slightly different your suggestion of passing a struct of function > pointers, but very similar.) Jeremy's approach sounds cleaner: just call the tool's init function and you get back a pointer to a "dictionary" -- a struct full of fn pointers, with NULLs where the tool doesn't want to override the core. The "register" approach means (if I understand correctly) that the tool's init function is called, resulting in a bunch of calls back to the core to register functions; eventually the tool init fn returns. That sounds more complex (mutable state; more complex control flow) and I'm not sure what the benefit is. So I vote for the dictionary approach -- assuming I understand this right. J |
|
From: Julian S. <js...@ac...> - 2005-03-27 19:20:14
|
> Jeremy's approach sounds cleaner: just call the tool's init function > and you get back a pointer to a "dictionary" -- a struct full of > fn pointers, with NULLs where the tool doesn't want to override the > core. In fact, couldn't this idea be used to almost completely decouple the tool and core, by passing dictionaries in both directions? At startup, the core finds the tool's init fn using dlsym, and calls it, handing it a dictionary of all core fns which the tool can have access to. And the returned value is a pointer to the tool's core-visible dictionary, as already discussed. J |
|
From: Nicholas N. <nj...@cs...> - 2005-03-27 19:25:32
|
On Sun, 27 Mar 2005, Julian Seward wrote: >> VG_(init_post_clo_init) ( & ac_post_clo_init ); >> VG_(init_instrument) ( & ac_instrument ); >> VG_(init_handle_client_request)( & ac_handle_client_request ); >> >> (That's slightly different your suggestion of passing a struct of function >> pointers, but very similar.) > > Jeremy's approach sounds cleaner: just call the tool's init function > and you get back a pointer to a "dictionary" -- a struct full of > fn pointers, with NULLs where the tool doesn't want to override the > core. > > The "register" approach means (if I understand correctly) that > the tool's init function is called, resulting in a bunch of calls > back to the core to register functions; eventually the tool init > fn returns. That sounds more complex (mutable state; more complex > control flow) and I'm not sure what the benefit is. One problem with the dictionary approach is that you are exposing the internals of the struct to tools. I avoided this approach with the 'needs' and 'details' structs for just this reason. > In fact, couldn't this idea be used to almost completely decouple > the tool and core, by passing dictionaries in both directions? > > At startup, the core finds the tool's init fn using dlsym, and > calls it, handing it a dictionary of all core fns which the tool > can have access to. And the returned value is a pointer to the > tool's core-visible dictionary, as already discussed. You certainly could. The tool would still have to import a lot of types and macros from the core. N |
|
From: Julian S. <js...@ac...> - 2005-03-28 18:41:05
|
Replying to several messages at once, up to and including discussion
of the proposed patch.
Simplifying the tool interface is definitely a Good Thing To Do. However
I'm not convinced by the objections raised thus far to the struct passing
approach, and I like its simplicity. Reasons below.
> > The "register" approach means (if I understand correctly) that
> > the tool's init function is called, resulting in a bunch of calls
> > back to the core to register functions; eventually the tool init
> > fn returns. That sounds more complex (mutable state; more complex
> > control flow) and I'm not sure what the benefit is.
>
> One problem with the dictionary approach is that you are exposing the
> internals of the struct to tools. I avoided this approach with the
> 'needs' and 'details' structs for just this reason.
Confused. Each tool would assemble a struct and pass a pointer
to it to the core as the return value of the tool's initialisation
function. So inevitably the internals of the struct are visible
to the tool anyway since it created the struct.
> Yes, I think that's better because it removes the fragility of having a
> structure as part of the interface.
How does having a struct as part of the interface make it fragile?
I am thinking of a very simple struct which contains only function
pointers; no other data.
If you are really worried about version mismatches in the struct,
make the first field (viz, offset 0) be an int which carries a
version identifier. Then we can safely say
"if (struct->version != ...) barf()"
This at least allows detection of ABI-level mismatches at startup
time, providing the version number is bumped each time the API changes.
> 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).
I agree. Especially if the struct has a version field at the start.
------
The registration-based scheme feels cluttered to me, because there are
more names and storage locations (global state) to keep track of. For
example, the tool makes a registration call:
+ VG_(basic_tool_funcs) (TL_(post_clo_init),
+ TL_(instrument),
+ TL_(fini));
Now in the core, if I want to call one of these, first I have to find the
global variables that those pointers got stored in -- probably using
grep. Then I have to be convinced that they've been initialised, and
only then can I make the call. Also I have to figure out precisely
which groups of functions make up the interface, and whether I've
forgotten to consider any.
By comparison, if I have a struct "tdict" from the tool, then there are
no banks of global variables to be found, no questions about initialisation,
and if I want assurance I'm seeing the complete interface, I only have to
read the struct definition -- nothing else. Then I can do:
assert(tdict->instrument); // we're hosed if this fails
bbi = tdict->instrument(bb);
or
if (tdict->some_optional_function)
some_optional_function(args);
------
Re-reading that, my real point is that the struct scheme is easier for
human readers of the code to read and reason about. And as Jeremy says,
it's not really any less robust against API changes relative to the
registration-scheme.
J
|
|
From: Nicholas N. <nj...@cs...> - 2005-03-28 14:35:38
|
On Mon, 28 Mar 2005, Julian Seward wrote:
>> One problem with the dictionary approach is that you are exposing the
>> internals of the struct to tools. I avoided this approach with the
>> 'needs' and 'details' structs for just this reason.
>
> Confused. Each tool would assemble a struct and pass a pointer
> to it to the core as the return value of the tool's initialisation
> function. So inevitably the internals of the struct are visible
> to the tool anyway since it created the struct.
In the current approach the struct is defined and allocated within the
core. It's called VG_(tool_interface), it has type "VgToolInterface.
The tool doesn't know what this struct looks like.
> How does having a struct as part of the interface make it fragile?
> I am thinking of a very simple struct which contains only function
> pointers; no other data.
I think the concern is if (when) fields get added or removed. Doing it
via functions can help, eg. if a field is removed you can make the
function that sets that field just do nothing. Just a general
abstract-types-are-nice thing. Although in this case it doesn't help that
much, as I agree below.
> If you are really worried about version mismatches in the struct,
> make the first field (viz, offset 0) be an int which carries a
> version identifier. Then we can safely say
>
> "if (struct->version != ...) barf()"
>
> This at least allows detection of ABI-level mismatches at startup
> time, providing the version number is bumped each time the API changes.
I don't like version numbers much, they're easy to forget to change.
Although I guess if you tied it into the existing MAJOR_VERSION_NUMBER (or
whatever it's called) that would help.
> ------
>
> The registration-based scheme feels cluttered to me, because there are
> more names and storage locations (global state) to keep track of. For
> example, the tool makes a registration call:
>
> + VG_(basic_tool_funcs) (TL_(post_clo_init),
> + TL_(instrument),
> + TL_(fini));
>
> Now in the core, if I want to call one of these, first I have to find the
> global variables that those pointers got stored in -- probably using
> grep.
Global variables? There are no global variables, the function pointers
will all get stored in the single VgToolInterface struct.
> Then I have to be convinced that they've been initialised, and only then
> can I make the call.
I don't see how this would be different if the tool does the struct
initialisation? (More below.)
> Also I have to figure out precisely which groups of functions make up
> the interface, and whether I've forgotten to consider any.
>
> By comparison, if I have a struct "tdict" from the tool, then there are
> no banks of global variables to be found,
As I've done it, we still have the single struct, it's just on the core
side. There are no banks of global variables. Perhaps it's confusing
because the names above TL_(post_clo_init)(), TL_(instrument)(), etc,
still have the TL_ prefix (which here means "vgTool_"). They don't need
to, I just haven't changed that in the patch. They could be eg.
mc_post_clo_init(), mc_instrument(), etc.
> no questions about initialisation,
> and if I want assurance I'm seeing the complete interface, I only have to
> read the struct definition -- nothing else.
That's true of how I've done it -- just look at VgToolInterface.
(Actually, it was already like that, I don't mean to take credit for it.)
> Then I can do:
>
> assert(tdict->instrument); // we're hosed if this fails
> bbi = tdict->instrument(bb);
>
> or
>
> if (tdict->some_optional_function)
> some_optional_function(args);
That's how it is with my patch. VG_(tool_interface) is the name of the
"tdict" struct. A shorter name might be better.
Another confusing aspect currently is that vg_toolint.c defines lots of
TL_ functions like this:
void TL_(fini)(Int exitcode)
{
return (*VG_(tool_interface).tool_fini)(exitcode);
}
where TL_ here means "vgToolInternal_" and so is different to the TL_
within the tools. So this is just a short-cut for your "tdict->fini(x)"
example which the core can call. We could get rid of these, and just
always use "tdict->fini(x)" to make it explicit. And then there are the
VG_(defined_*) and VG_(missing_*) functions which further clutter things,
and could possibly be removed.
> ------
>
> Re-reading that, my real point is that the struct scheme is easier for
> human readers of the code to read and reason about. And as Jeremy says,
> it's not really any less robust against API changes relative to the
> registration-scheme.
I think the patch is a lot closer to what you described and want than you
realise. Perhaps it's unclear because it's only a first step and there's
more cruft to remove (eg. the double-meaning of TL_). But it has a single
struct, which I think is the main thing you want.
As for the robustness against API changes, I agree that the way I've done
it isn't any more robust against API changes. But the advantage it does
have over a naked struct is robustness against one possible tool screwup:
the cases where the tool has to provide multiple functions together, as I
explained in my other response to Jeremy's comments.
--
Thanks for the feedback, really! I'm defending my patch, because there
are constraints involved that aren't obvious. But I won't refuse to
change it if the consensus is still against it after those constraints
have been made clearer. I'll be away until Thursday night, and I'm not
sure how much email contact I'll have, so if I don't reply it's not
because I'm sulking :)
N
|
|
From: Jeremy F. <je...@go...> - 2005-03-28 17:30:28
|
Julian Seward wrote:
>How does having a struct as part of the interface make it fragile?
>I am thinking of a very simple struct which contains only function
>pointers; no other data.
>
>If you are really worried about version mismatches in the struct,
>make the first field (viz, offset 0) be an int which carries a
>version identifier. Then we can safely say
>
> "if (struct->version != ...) barf()"
>
>This at least allows detection of ABI-level mismatches at startup
>time, providing the version number is bumped each time the API changes.
>
>
The trouble with this is that you break the ABI with every function you
add to the interface, even if it doesn't affect the semantics of any
other function, and most tools don't care about it. That's what is
fragile - you break the interface very easily. If you were to have a
registration function per tool entrypoint, it means you can add new
entrypoints without breaking the binaries of any existing tool. That
way you can reserve interface version numbers to mean semantic changes
to the interface.
J
|
|
From: Oswald B. <os...@kd...> - 2005-03-28 18:34:17
|
On Mon, Mar 28, 2005 at 09:30:23AM -0800, Jeremy Fitzhardinge wrote: > If you were to have a registration function per tool entrypoint, it > means you can add new entrypoints without breaking the binaries of any > existing tool. That way you can reserve interface version numbers to > mean semantic changes to the interface. > one can also use a real dictionary (name -> address) ... which is sort of exactly what the current dlsym solution does, only a bit more explicit. -- Hi! I'm a .signature virus! Copy me into your ~/.signature, please! -- Chaos, panic, and disorder - my work here is done. |
|
From: Nicholas N. <nj...@cs...> - 2005-03-30 05:05:44
Attachments:
diff
|
On Mon, 28 Mar 2005, Julian Seward wrote: > Simplifying the tool interface is definitely a Good Thing To Do. However > I'm not convinced by the objections raised thus far to the struct passing > approach, and I like its simplicity. Reasons below. Jeremy, Julian, I attach a revised version of my patch, which has a small improvement in the way that the size of redzones in the client arena is handled -- the size is now specified as an argument to VG_(malloc_funcs) rather than in the VG_DETERMINE_INTERFACE_VERSION macro, which is much better. Otherwise, it's unchanged. What do you both think about the way tool functions are registered, given the discussion we've had about the constraints involved? As I said before, if I committed this it would only be the first step of several. Or, I could do more changes and submit a larger patch, so you could see what things would look like further along that road. Let me know what you both think. N |
|
From: Jeremy F. <je...@go...> - 2005-03-30 08:50:44
|
Nicholas Nethercote wrote:
> I attach a revised version of my patch, which has a small improvement
> in the way that the size of redzones in the client arena is handled --
> the size is now specified as an argument to VG_(malloc_funcs) rather
> than in the VG_DETERMINE_INTERFACE_VERSION macro, which is much better.
Yes, that's good.
> Otherwise, it's unchanged. What do you both think about the way tool
> functions are registered, given the discussion we've had about the
> constraints involved? As I said before, if I committed this it would
> only be the first step of several. Or, I could do more changes and
> submit a larger patch, so you could see what things would look like
> further along that road. Let me know what you both think.
Even assuming the "all-or-none" condition is really invarient for all
these VG_(*_funcs)() functions, which I'm unconvinced about, I think
functions with lots of arguments are inherently unreadable and hard to
maintain. I'd really prefer to go with structures of pointers to
functions: at least you can name the fields and order isn't important.
(At the very very least, the redzone arg to VG_(malloc_funcs)() should
be first, so that new allocators can be added without having to move it,
or embed it in the middle of unrelated arguments.)
J
|
|
From: Nicholas N. <nj...@cs...> - 2005-03-30 15:02:48
|
On Wed, 30 Mar 2005, Jeremy Fitzhardinge wrote: > Even assuming the "all-or-none" condition is really invarient for all these > VG_(*_funcs)() functions, which I'm unconvinced about, I think functions with > lots of arguments are inherently unreadable and hard to maintain. I'd really > prefer to go with structures of pointers to functions: at least you can name > the fields and order isn't important. (At the very very least, the redzone > arg to VG_(malloc_funcs)() should be first, so that new allocators can be > added without having to move it, or embed it in the middle of unrelated > arguments.) Giving the tool direct access to the VG_(tool_interface) struct doesn't seem like a good idea -- it exposes it to more than it needs to see. So this would simply separate structs corresponding to each of the multi-arg functions in the patch now. Only two of those functions are really bad -- VG_(needs_tool_errors)() has 8 args, VG_(malloc_funcs)() has 10. Do you think we need structs for the ones with only 1, 2 or 3 args? N |
|
From: Jeremy F. <je...@go...> - 2005-03-30 23:18:57
|
Nicholas Nethercote wrote:
> Giving the tool direct access to the VG_(tool_interface) struct
> doesn't seem like a good idea -- it exposes it to more than it needs
> to see.
Agreed.
> So this would simply separate structs corresponding to each of the
> multi-arg functions in the patch now. Only two of those functions are
> really bad -- VG_(needs_tool_errors)() has 8 args, VG_(malloc_funcs)()
> has 10. Do you think we need structs for the ones with only 1, 2 or 3
> args?
It would seem silly to have lots of little structures, but having two
calling styles isn't terribly pretty either (and one presumes that at
some point you'd need to convert a called-with-pointers function to a
called-with-a-struct function when the number of functions reaches a
certain point). I guess structures for the biggies and literal pointers
for the 1,2,3 cases, but I still think a simple one registry function
per function model is cleaner and more robust in the long term.
J
|
|
From: Jeremy F. <je...@go...> - 2005-03-28 08:41:19
|
Nicholas Nethercote wrote:
> Hmm, I didn't really follow that... I'm not clear on what
> functions/bits of memory are from the client, what are from the
> pthreads implementation, and what are in Valgrind/pthreadmodel.
before_pthread_create() allocates an instance of
vg_pthread_newthread_data; this function is run as part of the core. The
pointer to this instance ends up being passed to the real
pthread_create, and is used then freed in
vg_intercept.c:VG_WRAPPER(pthread_startfunc_wrapper)(), which is run as
part of the client code.
> Where would the USERREQ_FREE requrest be embedded in the client?
> Would every client have to have such a request in it?
No client directly calls USERREQ_FREE. Any client which calls
pthread_create ends up calling it via code in vg_inject.so (compiled
from vg_intercept.c).
J
|
|
From: Nicholas N. <nj...@cs...> - 2005-03-28 15:27:54
|
On Mon, 28 Mar 2005, Jeremy Fitzhardinge wrote: >> Where would the USERREQ_FREE requrest be embedded in the client? Would >> every client have to have such a request in it? > > No client directly calls USERREQ_FREE. Any client which calls pthread_create > ends up calling it via code in vg_inject.so (compiled from vg_intercept.c). Ah, that's the key idea I was missing. Thanks. N |
|
From: Julian S. <js...@ac...> - 2005-03-28 15:24:31
|
> > The registration-based scheme feels cluttered to me, because there are
> > more names and storage locations (global state) to keep track of. For
> > example, the tool makes a registration call:
> >
> > + VG_(basic_tool_funcs) (TL_(post_clo_init),
> > + TL_(instrument),
> > + TL_(fini));
> >
> > Now in the core, if I want to call one of these, first I have to find the
> > global variables that those pointers got stored in -- probably using
> > grep.
>
> Global variables? There are no global variables, the function pointers
> will all get stored in the single VgToolInterface struct.
Ah. I didn't realise that the struct exists on the core side; I thought
it only existed on the core side. (== I didn't understand that). That
zaps some of my objections.
> I think the patch is a lot closer to what you described and want than you
> realise. Perhaps it's unclear because it's only a first step and there's
> more cruft to remove (eg. the double-meaning of TL_). But it has a single
> struct, which I think is the main thing you want.
Yes, you could be right.
> Another confusing aspect currently is that vg_toolint.c defines lots of
> TL_ functions like this:
>
> void TL_(fini)(Int exitcode)
> {
> return (*VG_(tool_interface).tool_fini)(exitcode);
> }
>
> where TL_ here means "vgToolInternal_" and so is different to the TL_
> within the tools. So this is just a short-cut for your "tdict->fini(x)"
> example which the core can call. We could get rid of these, and just
> always use "tdict->fini(x)" to make it explicit. And then there are the
> VG_(defined_*) and VG_(missing_*) functions which further clutter things,
> and could possibly be removed.
That would be a nice clarification. I'd prefer to directly call
(*VG_(tool_interface).tool_fini)(exitcode)
rather than
TL_(fini)(Int exitcode)
even though it's more verbose -- less layers of abstraction is good.
Part of the reason I didn't even realise there was a tool_interface
struct on the core side was as a result of this hiding.
J
|
|
From: Nicholas N. <nj...@cs...> - 2005-03-28 15:33:03
|
On Mon, 28 Mar 2005, Julian Seward wrote: > Ah. I didn't realise that the struct exists on the core side; I thought > it only existed on the core side. Do you mean "only existed on the tool side?" > That would be a nice clarification. I'd prefer to directly call > > (*VG_(tool_interface).tool_fini)(exitcode) > > rather than > > TL_(fini)(Int exitcode) > > even though it's more verbose -- less layers of abstraction is good. Yes. > Part of the reason I didn't even realise there was a tool_interface > struct on the core side was as a result of this hiding. I think that was the point :) Jeremy implemented it in a way that made it look just like the 2.0.0 system, so the core could still call TL_(fini)(), even though the underlying details were quite different. N |
|
From: Jeremy F. <je...@go...> - 2005-03-28 17:42:42
|
Nicholas Nethercote wrote:
> 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?
Well, a function with 15 (or even 8) arguments is always going to be
hard to read, and hard to write or modify (it's impossible to remember
what order they should go in, and very easy to stuff it up by adding or
deleting an argument - in either case you need to carefully check the
call against the function prototype).
Are you saying that the needs functions are, by definition, all-or-none,
so that if you want to add a new function with is related but not
essential (say, get_extra_address_info) would actually be a different
need? Or that a tool would be always required to implement a function
because of the all-or-none policy, even if its a nul implementation?
I'm all for grouping the interface callback registrations into
functionally related pieces, but it seems like a stretch to further
assert that functional grouping == all-or-none.
J
|