|
From: Vince W. <vi...@cs...> - 2008-02-21 17:38:48
|
Hello
I was looking at extending my Basic Block Vector generating plugin so that
it can handle multi-threaded apps.
To do this properly, it would be nice if it were possible to be notified
whenever a thread-change happened. That way the statistics from the
various threads could be kept separately.
I looked through the code, and it didn't look like functionality existed
for this already.
I've attached a patch that implements registering a thread callback, but
I'm not sure if this is the best way to accomplish this. Also, it does
make some intrusive changes to the core so I wanted to see what opinions
people have on if this is useful or not.
Thanks,
Vince
--- ./include/pub_tool_tooliface.h.orig 2008-02-20 17:22:26.000000000 -0500
+++ ./include/pub_tool_tooliface.h 2008-02-20 17:40:22.000000000 -0500
@@ -446,6 +446,8 @@
function here. */
extern void VG_(needs_final_IR_tidy_pass) ( IRSB*(*final_tidy)(IRSB*) );
+extern void VG_(needs_thread_callback) (void(*thread_switch) (ThreadId));
+
/* ------------------------------------------------------------------ */
/* Core events to track */
--- ./coregrind/m_scheduler/scheduler.c.orig 2008-02-20 17:29:34.000000000 -0500
+++ ./coregrind/m_scheduler/scheduler.c 2008-02-20 17:47:42.000000000 -0500
@@ -945,6 +945,9 @@
VG_(message)(Vg_DebugMsg, "thread %d: running for %d bbs",
tid, VG_(dispatch_ctr) - 1 );
+ if (VG_(needs).thread_callback)
+ (VG_(tdict).tool_thread_callback) ( tid );
+
trc = run_thread_for_a_while ( tid );
if (VG_(clo_trace_sched) && VG_(clo_verbosity) > 2) {
--- ./coregrind/m_tooliface.c.orig 2008-02-20 16:33:45.000000000 -0500
+++ ./coregrind/m_tooliface.c 2008-02-20 17:37:08.000000000 -0500
@@ -94,7 +94,8 @@
.data_syms = False,
.malloc_replacement = False,
.xml_output = False,
- .final_IR_tidy_pass = False
+ .final_IR_tidy_pass = False,
+ .thread_callback = False,
};
/* static */
@@ -217,6 +218,14 @@
VG_(tdict).tool_handle_client_request = handle;
}
+void VG_(needs_thread_callback)(
+ void(*thread_switch) (ThreadID)
+)
+{
+ VG_(needs).thread_callback = True;
+ VG_(tdict).tool_thread_callback=thread_switch;
+}
+
void VG_(needs_syscall_wrapper)(
void(*pre) (ThreadId, UInt),
void(*post)(ThreadId, UInt, SysRes res)
--- ./coregrind/pub_core_tooliface.h.orig 2008-02-20 17:08:00.000000000 -0500
+++ ./coregrind/pub_core_tooliface.h 2008-02-20 17:39:47.000000000 -0500
@@ -92,6 +92,7 @@
Bool malloc_replacement;
Bool xml_output;
Bool final_IR_tidy_pass;
+ Bool thread_callback;
}
VgNeeds;
@@ -160,6 +161,9 @@
// VG_(needs).final_IR_tidy_pass
IRSB* (*tool_final_IR_tidy_pass) (IRSB*);
+ // VG_(needs).thread_callback
+ void (*tool_thread_callback) (ThreadId);
+
// -- Event tracking functions ------------------------------------
void (*track_new_mem_startup) (Addr, SizeT, Bool, Bool, Bool);
void (*track_new_mem_stack_signal)(Addr, SizeT);
|
|
From: Bart V. A. <bar...@gm...> - 2008-02-21 19:01:13
|
On Thu, Feb 21, 2008 at 6:38 PM, Vince Weaver <vi...@cs...> wrote: > To do this properly, it would be nice if it were possible to be notified > whenever a thread-change happened. That way the statistics from the > various threads could be kept separately. This would be useful for exp-drd too. What I do now is to compare the result of VG_(get_running_tid)() with a cached value in order to detect when Valgrind scheduled another thread. This test is performed upon every load, every store, and every time the stack pointer is modified. Bart. |
|
From: Nicholas N. <nj...@cs...> - 2008-02-21 21:20:28
|
On Thu, 21 Feb 2008, Bart Van Assche wrote: > On Thu, Feb 21, 2008 at 6:38 PM, Vince Weaver <vi...@cs...> wrote: >> To do this properly, it would be nice if it were possible to be notified >> whenever a thread-change happened. That way the statistics from the >> various threads could be kept separately. > > This would be useful for exp-drd too. What I do now is to compare the > result of VG_(get_running_tid)() with a cached value in order to > detect when Valgrind scheduled another thread. This test is performed > upon every load, every store, and every time the stack pointer is > modified. I thought we had such an event -- I thought Josef used it in Callgrind. In include/pub_tool_tooliface.h I see: track_start_client_code track_stop_client_code track_pre_thread_ll_create track_pre_thread_ll_create track_pre_thread_ll_exit Are any of these good enough? (I really thought we had a track_thread_switch event...) Nick |
|
From: Julian S. <js...@ac...> - 2008-02-21 23:33:44
|
> I thought we had such an event -- I thought Josef used it in Callgrind.
> In include/pub_tool_tooliface.h I see:
>
> track_start_client_code
> track_stop_client_code
> track_pre_thread_ll_create
> track_pre_thread_ll_create
> track_pre_thread_ll_exit
>
> Are any of these good enough? (I really thought we had a
> track_thread_switch event...)
Indeed we do; track_{start,stop}_client code should provide the
required info.
J
|
|
From: Nicholas N. <nj...@cs...> - 2008-02-22 00:58:34
|
On Fri, 22 Feb 2008, Julian Seward wrote:
>> I thought we had such an event -- I thought Josef used it in Callgrind.
>> In include/pub_tool_tooliface.h I see:
>>
>> track_start_client_code
>> track_stop_client_code
>> track_pre_thread_ll_create
>> track_pre_thread_ll_create
>> track_pre_thread_ll_exit
>>
>> Are any of these good enough? (I really thought we had a
>> track_thread_switch event...)
>
> Indeed we do; track_{start,stop}_client code should provide the
> required info.
Perhaps the comment in pub_tool_tooliface.h could be clarified -- it wasn't
clear to me (and presumably others) that this means "thread switch has
occurred".
Nick
|
|
From: Bart V. A. <bar...@gm...> - 2008-02-22 07:15:24
|
On Thu, Feb 21, 2008 at 10:19 PM, Nicholas Nethercote <nj...@cs...> wrote: > On Thu, 21 Feb 2008, Bart Van Assche wrote: > > On Thu, Feb 21, 2008 at 6:38 PM, Vince Weaver <vi...@cs...> wrote: > > > To do this properly, it would be nice if it were possible to be notified > > > whenever a thread-change happened. That way the statistics from the > > > various threads could be kept separately. > > > > This would be useful for exp-drd too. What I do now is to compare the > > result of VG_(get_running_tid)() with a cached value in order to > > detect when Valgrind scheduled another thread. This test is performed > > upon every load, every store, and every time the stack pointer is > > modified. > > I thought we had such an event -- I thought Josef used it in Callgrind. > In include/pub_tool_tooliface.h I see: > > track_start_client_code > track_stop_client_code > track_pre_thread_ll_create > track_pre_thread_ll_create > track_pre_thread_ll_exit > > Are any of these good enough? (I really thought we had a > track_thread_switch event...) The above is not sufficient. I have tried in exp-drd to only update the cached thread ID when one of the above events was generated. The result was that for several test cases the cached thread ID was not equal to VG_(get_running_tid()). An example of such a test case is exp-drd/tests/sigalrm.c. Bart. |
|
From: Josef W. <Jos...@gm...> - 2008-02-22 12:35:30
|
Hi, On Friday 22 February 2008, Bart Van Assche wrote: > > > This would be useful for exp-drd too. What I do now is to compare the > > > result of VG_(get_running_tid)() with a cached value in order to > > > detect when Valgrind scheduled another thread. This test is performed > > > upon every load, every store, and every time the stack pointer is > > > modified. > > > > I thought we had such an event -- I thought Josef used it in Callgrind. > > In include/pub_tool_tooliface.h I see: > > > > track_start_client_code Actually, I use this one. But I do not fully rely on it. Callgrind has the "nice" property that it instruments a callback at begin of every basic block. And there, I adjust to VG_(get_running_tid)(). In Callgrind, I have the additional requirement the reliable track enter/leave of signal handlers. This is not possible with the hooks provided by valgrind, e.g. in the case of a longjmp from signal handler into regular code. > > track_stop_client_code > > track_pre_thread_ll_create > > track_pre_thread_ll_create > > track_pre_thread_ll_exit > > > > Are any of these good enough? (I really thought we had a > > track_thread_switch event...) > > The above is not sufficient. I have tried in exp-drd to only update > the cached thread ID when one of the above events was generated. The > result was that for several test cases the cached thread ID was not > equal to VG_(get_running_tid()). An example of such a test case is > exp-drd/tests/sigalrm.c. Did you research the reason for this? I have this quite old comment before my "TID adjustment" (bbcc.c:536): /* This is needed because thread switches can not reliable be tracked * with callback CLG_(run_thread) only: we have otherwise no way to get * the thread ID after a signal handler returns. * This could be removed again if that bug is fixed in Valgrind. * This is in the hot path but hopefully not to costly. */ Josef |
|
From: Julian S. <js...@ac...> - 2008-02-22 13:46:38
|
> > > trace_start_client_code
> > > track_stop_client_code
> > >
> > > Are any of these good enough? (I really thought we had a
> > > track_thread_switch event...)
> >
> > The above is not sufficient. I have tried in exp-drd to only update
> > the cached thread ID when one of the above events was generated. The
> > result was that for several test cases the cached thread ID was not
> > equal to VG_(get_running_tid()). An example of such a test case is
> > exp-drd/tests/sigalrm.c.
>
> Did you research the reason for this?
Yes, that is a good question.
I am (very) surprised to hear that track_{start,stop}_client_code
give wrong results. The reason is that (translations of) client code
(and I mean *all* client code, including client signal handlers)
can only be reached through the following functions
coregrind/m_scheduler/scheduler.c: run_thread_for_a_while
coregrind/m_scheduler/scheduler.c: run_noredir_translation
There is no other way for a translation to be run.
In both functions, the call to the generated code is bracketed
by these:
// Tell the tool this thread is about to run client code
VG_TRACK( start_client_code, tid, bbs_done );
// Tell the tool this thread has stopped running client code
VG_TRACK( stop_client_code, tid, bbs_done );
So perhaps tid is wrong here? But it can't be: the tid
passed to run_thread_for_a_while and run_noredir_translation
is used to select which set of guest registers to use for the
run, and so if it was wrong threaded programs as a whole would
die in a very obvious way.
J
|
|
From: Josef W. <Jos...@gm...> - 2008-02-22 14:39:56
|
On Friday 22 February 2008, Julian Seward wrote:
> > > > trace_start_client_code
> > > > track_stop_client_code
> ...
> > > The above is not sufficient.
> ...
> I am (very) surprised to hear that track_{start,stop}_client_code
> give wrong results. The reason is that (translations of) client code
> (and I mean *all* client code, including client signal handlers)
<OT>
"Signal handler exit via longjmp" is offtopic here, but how should valgrind
core be able to detect this, as the signal frame is silently discarded by
longjmp? This is also clearly stated in "pub_tool_tooliface.h":
/* Called after a signal is delivered. Nb: unfortunately, if the signal
handler longjmps, this won't be called. */
void VG_(track_post_deliver_signal)(void(*f)(ThreadId tid, Int sigNo));
However, this is not important for Callgrind as with the regular resync.
of the shadow stack against the real one, this case still is catched
without any help from VG core.
</OT>
> So perhaps tid is wrong here?
For Callgrind, it is a non-issue, as setup_bbcc is called at beginning
of every BB. So I wanted to play safe and did the adjustment against
VG_(get_running_tid) there. I could skip this and make an assertion instead.
Josef
|
|
From: Bart V. A. <bar...@gm...> - 2008-02-22 14:24:22
|
On Fri, Feb 22, 2008 at 2:38 PM, Julian Seward <js...@ac...> wrote:
> I am (very) surprised to hear that track_{start,stop}_client_code
> give wrong results.
As soon as I have the time I will look further into this issue. But
please note that I have raised this issue more than once in the past
-- see e.g. http://bugs.kde.org/show_bug.cgi?id=152728.
Bart.
|
|
From: Julian S. <js...@ac...> - 2008-02-22 14:40:55
|
On Friday 22 February 2008 15:24, Bart Van Assche wrote:
> On Fri, Feb 22, 2008 at 2:38 PM, Julian Seward <js...@ac...> wrote:
> > I am (very) surprised to hear that track_{start,stop}_client_code
> > give wrong results.
>
> As soon as I have the time I will look further into this issue. But
> please note that I have raised this issue more than once in the past
> -- see e.g. http://bugs.kde.org/show_bug.cgi?id=152728.
Yes. I'm only saying that track_{start,stop}_client_code look correct
to me, and so the problem is more likely to be elsewhere, perhaps:
VG_(get_running_tid)() produces wrong results, under some circumstances,
or
the assertion that VG_(get_running_tid)() and track_{start,stop}_client_code
agree is not always valid, for some subtle reason.
I am a bit unclear about the precise semantics of VG_(get_running_tid)
(was it ever specified, exactly? I don't know). That probably doesn't
help. A first step might be to specify exactly the behaviour of this fn.
J
|
|
From: Bart V. A. <bar...@gm...> - 2008-02-22 17:53:12
|
On Fri, Feb 22, 2008 at 3:57 PM, Julian Seward <js...@ac...> wrote: > track_start/stop_client_code does only show you when the CPU is > running translated code blocks. However, it may be that a thread > has the CPU but is not running translated code, but instead doing > one of many other things (including building signal frames). > That's the difference: get_running_tid shows you what thread has > the CPU. In exp-drd I need the value of VG_(get_running_tid)() upon *every* client memory access, not just for the memory accesses generated by translated code. I would appreciate it very much that the Valgrind core would announce changes in VG_(get_running_tid)() instead of having to verify the value of VG_(get_running_tid)() in the tool. > Are you intercepting both track_start_client_code and track_stop_client_code, > or only the _start ? Only _start. Even if I would track _stop, I still would need code in the drd_trace_load() / drd_trace_store() functions for verifying whether VG_(get_running_tid)() changed. Bart. |
|
From: Julian S. <js...@ac...> - 2008-02-25 12:46:06
|
> In exp-drd I need the value of VG_(get_running_tid)() upon *every* > client memory access, not just for the memory accesses generated by > translated code. [...] Yes. And the same is true in Helgrind. > Only _start. Even if I would track _stop, I still would need code in > the drd_trace_load() / drd_trace_store() functions for verifying > whether VG_(get_running_tid)() changed. You really need to intercept the _stop function. * when notified with _start, cache the thread ID provided. * when notified with _stop, discard the cached thread ID. * when you need to know the thread ID, first see if you have a cached ID. If not, call VG_(get_running_tid) (but don't cache the result). J |
|
From: Bart V. A. <bar...@gm...> - 2008-02-25 16:37:33
|
On Mon, Feb 25, 2008 at 1:42 PM, Julian Seward <js...@ac...> wrote:
>
> > Only _start. Even if I would track _stop, I still would need code in
> > the drd_trace_load() / drd_trace_store() functions for verifying
> > whether VG_(get_running_tid)() changed.
>
> You really need to intercept the _stop function.
>
> * when notified with _start, cache the thread ID provided.
>
> * when notified with _stop, discard the cached thread ID.
>
> * when you need to know the thread ID, first see if you have
> a cached ID. If not, call VG_(get_running_tid) (but don't
> cache the result).
The code that is currently executed by exp-drd at the start of every
memory load and store is about as follows:
if (VG_(get_running_tid) != cached_tid)
{ cached_tid = VG_(get_running_tid); /* update danger set */; }
When tracking both _start and _stop, the code at the start of every
memory load and store will look as follows:
if (tracked_thread_id == VG_INVALID_THREADID && VG_(get_running_tid)
!= cached_tid)
{ cached_tid = VG_(get_running_tid); /* update danger set */; }
Is this an improvement ? This would save one function call in the
common path, but there is also an alternative solution for this --
declaring VG_(get_running_tid)() inline. And the code for checking the
thread ID still has to be executed for every client memory load and
for every client memory store.
I still prefer that the Valgrind core calls a tracking function to
notify a tool about any thread ID changes.
Bart.
|