|
From: Bart V. A. <bar...@gm...> - 2007-12-20 19:43:13
Attachments:
valgrind-2007-12-20.patch
|
Hello Julian, There is a small inconsistency in the Valgrind core with regard to the value of VG_(running_tid) concerning client memory accesses: this variable contains a valid thread ID for all client memory accesses, except for some accesses triggered from coregrind/m_main.c. The attached patch fixes this and also contains some exp-drd simplifications that became possible because of the fix in the core. Can you please evaluate this patch and apply it if you consider it acceptable ? Regression test results are unaffected by the patch -- the results below are for the x86_64 platform. == 357 tests, 7 stderr failures, 2 stdout failures, 0 post failures == memcheck/tests/malloc_free_fill (stderr) memcheck/tests/pointer-trace (stderr) memcheck/tests/vcpu_fnfns (stdout) memcheck/tests/writev (stderr) memcheck/tests/x86/scalar (stderr) memcheck/tests/x86/scalar_exit_group (stderr) memcheck/tests/x86/scalar_supp (stderr) none/tests/mremap (stderr) none/tests/mremap2 (stdout) Regards, Bart Van Assche. |
|
From: Tom H. <to...@co...> - 2007-12-20 21:31:25
|
In message <e2e...@ma...>
"Bart Van Assche" <bar...@gm...> wrote:
> There is a small inconsistency in the Valgrind core with regard to the
> value of VG_(running_tid) concerning client memory accesses: this
> variable contains a valid thread ID for all client memory accesses,
> except for some accesses triggered from coregrind/m_main.c. The
> attached patch fixes this and also contains some exp-drd
> simplifications that became possible because of the fix in the core.
> Can you please evaluate this patch and apply it if you consider it
> acceptable ? Regression test results are unaffected by the patch --
> the results below are for the x86_64 platform.
Looks good to me.
Are you sure that's everything? My memory was that the problems
were with signal handlers and things, but maybe those have all
been fixed now?
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
|
From: Bart V. A. <bar...@gm...> - 2007-12-21 07:07:33
|
On Dec 20, 2007 10:31 PM, Tom Hughes <to...@co...> wrote: > > Looks good to me. > > Are you sure that's everything? My memory was that the problems > were with signal handlers and things, but maybe those have all > been fixed now? > > Tom > What I had noticed for signal handlers is that VG_(running_tid) was correct, but that VG_(running_tid) was changed before VG_TRACK(start_client_code)() was called. Instead of caching VG_(running_tid) upon VG_TRACK(start_client_code)(), exp-drd now queries VG_(running_tid) upon every memory access. This solved the issues I encountered with memory accesses from within signal handlers. Regards, Bart. |
|
From: Tom H. <to...@co...> - 2007-12-21 09:05:01
|
In message <e2e...@ma...>
Bart Van Assche <bar...@gm...> wrote:
> On Dec 20, 2007 10:31 PM, Tom Hughes <to...@co...> wrote:
>
>> Are you sure that's everything? My memory was that the problems
>> were with signal handlers and things, but maybe those have all
>> been fixed now?
>
> What I had noticed for signal handlers is that VG_(running_tid) was correct,
> but that VG_(running_tid) was changed before VG_TRACK(start_client_code)()
> was called. Instead of caching VG_(running_tid) upon
> VG_TRACK(start_client_code)(), exp-drd now queries VG_(running_tid) upon
> every memory access. This solved the issues I encountered with memory
> accesses from within signal handlers.
Surely if it was changed before VG_TRACK(start_client_code)() then it
would have been fine for it to cache it?
Did you mean that it was changed after that tracking call but before
the code in the handler was executed?
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
|
From: Bart V. A. <bar...@gm...> - 2007-12-21 15:35:46
|
On Dec 21, 2007 10:04 AM, Tom Hughes <to...@co...> wrote: > > Surely if it was changed before VG_TRACK(start_client_code)() then it > would have been fine for it to cache it? > > Did you mean that it was changed after that tracking call but before > the code in the handler was executed? What happens is the following: * Core changes VG_(running_tid). * Core notifies tool about client memory accesses. * Core calls VG_TRACK(start_client_code)(). This is why it is not sufficient to cache VG_(running_tid) upon VG_TRACK(start_client_code)() notifications. Regards, Bart Van Assche. |
|
From: Tom H. <to...@co...> - 2007-12-21 15:47:16
|
In message <e2e...@ma...>
Bart Van Assche <bar...@gm...> wrote:
> On Dec 21, 2007 10:04 AM, Tom Hughes <to...@co...> wrote:
>>
>> Surely if it was changed before VG_TRACK(start_client_code)() then it
>> would have been fine for it to cache it?
>>
>> Did you mean that it was changed after that tracking call but before
>> the code in the handler was executed?
>
> What happens is the following:
> * Core changes VG_(running_tid).
> * Core notifies tool about client memory accesses.
> * Core calls VG_TRACK(start_client_code)().
>
> This is why it is not sufficient to cache VG_(running_tid) upon
> VG_TRACK(start_client_code)() notifications.
Ah right. I guess the question there is "which thread are those
accesses made on behalf off?".
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
|
From: Bart V. A. <bar...@gm...> - 2007-12-21 15:53:18
|
On Dec 21, 2007 4:47 PM, Tom Hughes <to...@co...> wrote: > In message <e2e...@ma...> > Bart Van Assche <bar...@gm...> wrote: > > > What happens is the following: > > * Core changes VG_(running_tid). > > * Core notifies tool about client memory accesses. > > * Core calls VG_TRACK(start_client_code)(). > > > > This is why it is not sufficient to cache VG_(running_tid) upon > > VG_TRACK(start_client_code)() notifications. > > Ah right. I guess the question there is "which thread are those > accesses made on behalf off?". The client accesses are made on behalf of the thread with thread ID VG_(running_tid). Regards, Bart Van Assche. |
|
From: Tom H. <to...@co...> - 2007-12-30 13:21:09
|
On 20/12/2007, Bart Van Assche <bar...@gm...> wrote: > There is a small inconsistency in the Valgrind core with regard to the > value of VG_(running_tid) concerning client memory accesses: this > variable contains a valid thread ID for all client memory accesses, > except for some accesses triggered from coregrind/m_main.c. The > attached patch fixes this and also contains some exp-drd > simplifications that became possible because of the fix in the core. > Can you please evaluate this patch and apply it if you consider it > acceptable ? Regression test results are unaffected by the patch -- > the results below are for the x86_64 platform. I was just looking at committing this, but I've run into a bit of a problem, namely that drd segfaults on startup on my machine (with or without this patch) so it's a bit hard for me to test it before I commit it ;-) The segfault is while recording the execution context from sg_init, called as follows: VG_(get_StackTrace)() VG_(record_ExeContext)() sg_init() sg_new() thread_pre_create() drd_pre_thread_create() VG_(main)() [when creating the main thread] I think the problem is that the thread hasn't started executing yet so there is no context to unwind - the PC, FP and SP are all zero... Tom -- Tom Hughes (to...@co...) http://www.compton.nu/ |
|
From: Bart V. A. <bar...@gm...> - 2008-01-01 17:10:29
|
On Dec 30, 2007 2:21 PM, Tom Hughes <to...@co...> wrote: > I was just looking at committing this, but I've run into a bit of a > problem, namely that drd segfaults on startup on my machine (with or > without this patch) so it's a bit hard for me to test it before I > commit it ;-) The cause of this crash is that in revision 7291 the behavior of VG_(record_ExeContext)() has been changed: in revision 7290 and before it was allowed to record the exe context from within VG_TRACK(pre_thread_create)(), but from revision 7291 on this results in a crash. I'm waiting for further input from Julian to resolve this issue. See also http://www.mail-archive.com/val...@li.../msg01278.html Bart. |
|
From: Julian S. <js...@ac...> - 2008-01-01 17:54:12
|
> The cause of this crash is that in revision 7291 the behavior of > VG_(record_ExeContext)() has been changed: in revision 7290 and before > it was allowed to record the exe context from within > VG_TRACK(pre_thread_create)(), but from revision 7291 on this results > in a crash. As per http://www.mail-archive.com/valgrind-developers%40lists.sourceforge.net/msg01282.html and also http://www.mail-archive.com/valgrind-developers%40lists.sourceforge.net/msg01324.html the cause of the crash is that drd is calling VG_(record_ExeContext) for a thread which all the registers are zero, leading to the unwinder crashing. Not sure what 7291 has to do with it. 7291 simply created a copy of trunk 7290 post release. My guess is the important change is this, in 7300: --- trunk/coregrind/m_stacktrace.c 2007-12-12 11:42:33 UTC (rev 7299) +++ trunk/coregrind/m_stacktrace.c 2007-12-15 22:13:05 UTC (rev 7300) @@ -97,11 +97,9 @@ /* Assertion broken before main() is reached in pthreaded programs; the * offending stack traces only have one item. --njn, 2002-aug-16 */ /* vg_assert(fp_min <= fp_max);*/ - - if (fp_min + VG_(clo_max_stackframe) <= fp_max) { - /* If the stack is ridiculously big, don't poke around ... but - don't bomb out either. Needed to make John Regehr's - user-space threads package work. JRS 20021001 */ + if (fp_min + 512 >= fp_max) { + /* If the stack limits look bogus, don't poke around ... but + don't bomb out either. */ ips[0] = ip; return 1; } That change gets rid of an essentially bogus check and replaces it with something more reasonable. Unfortunately I think the bogus check caught the case where drd passes SP=0 (etc) and so had the unintended side effect of avoiding the segfault. Basically you can't call VG_(record_ExeContext) with a zero stack pointer as is happening now. So don't do the call -- wait till you have a valid SP and only then call it. In any case there is no loss since the previous arrangement would have gotten you only a 1-element stack trace with the top/only IP value being zero. J |
|
From: Tom H. <to...@co...> - 2008-01-02 10:13:30
|
On 20/12/2007, Bart Van Assche <bar...@gm...> wrote: > There is a small inconsistency in the Valgrind core with regard to the > value of VG_(running_tid) concerning client memory accesses: this > variable contains a valid thread ID for all client memory accesses, > except for some accesses triggered from coregrind/m_main.c. The > attached patch fixes this and also contains some exp-drd > simplifications that became possible because of the fix in the core. > Can you please evaluate this patch and apply it if you consider it > acceptable ? Regression test results are unaffected by the patch -- > the results below are for the x86_64 platform. I've committed a (slightly modified) version of this now. Tom -- Tom Hughes (to...@co...) http://www.compton.nu/ |