From: <sv...@va...> - 2010-09-02 14:50:49
|
Author: bart Date: 2010-09-02 15:50:41 +0100 (Thu, 02 Sep 2010) New Revision: 11329 Log: Made sure that DRD processes client programs that use SA_ONSTACK correctly (e.g. Wine). Modified: trunk/drd/drd_main.c Modified: trunk/drd/drd_main.c =================================================================== --- trunk/drd/drd_main.c 2010-09-02 14:44:17 UTC (rev 11328) +++ trunk/drd/drd_main.c 2010-09-02 14:50:41 UTC (rev 11329) @@ -326,9 +326,6 @@ { const Addr a2 = a1 + len; - if (len == 0) - return; - tl_assert(a1 < a2); if (UNLIKELY(DRD_(any_address_is_traced)())) @@ -451,7 +448,75 @@ True); } +static +Bool on_alt_stack(const Addr a) +{ + ThreadId vg_tid; + Addr alt_min; + SizeT alt_size; + + vg_tid = VG_(get_running_tid)(); + alt_min = VG_(thread_get_altstack_min)(vg_tid); + alt_size = VG_(thread_get_altstack_size)(vg_tid); + return (SizeT)(a - alt_min) < alt_size; +} + +static +void drd_start_using_mem_alt_stack(const Addr a, const SizeT len) +{ + if (!on_alt_stack(a)) + drd_start_using_mem_stack(a, len); +} + +static +void drd_stop_using_mem_alt_stack(const Addr a, const SizeT len) +{ + if (!on_alt_stack(a)) + drd_stop_using_mem_stack(a, len); +} + /** + * Callback function invoked by the Valgrind core before a signal is delivered. + */ +static +void drd_pre_deliver_signal(const ThreadId vg_tid, const Int sigNo, + const Bool alt_stack) +{ + DrdThreadId drd_tid; + + drd_tid = DRD_(VgThreadIdToDrdThreadId)(vg_tid); + DRD_(thread_set_on_alt_stack)(drd_tid, alt_stack); + if (alt_stack) + { + /* + * As soon a signal handler has been invoked on the alternate stack, + * switch to stack memory handling functions that can handle the + * alternate stack. + */ + VG_(track_new_mem_stack)(drd_start_using_mem_alt_stack); + VG_(track_die_mem_stack)(drd_stop_using_mem_alt_stack); + } +} + +/** + * Callback function invoked by the Valgrind core after a signal is delivered, + * at least if the signal handler did not longjmp(). + */ +static +void drd_post_deliver_signal(const ThreadId vg_tid, const Int sigNo) +{ + DrdThreadId drd_tid; + + drd_tid = DRD_(VgThreadIdToDrdThreadId)(vg_tid); + DRD_(thread_set_on_alt_stack)(drd_tid, False); + if (DRD_(thread_get_threads_on_alt_stack)() == 0) + { + VG_(track_new_mem_stack)(drd_start_using_mem_stack); + VG_(track_die_mem_stack)(drd_stop_using_mem_stack); + } +} + +/** * Callback function called by the Valgrind core before a stack area is * being used by a signal handler. * @@ -683,6 +748,8 @@ VG_(track_die_mem_munmap) (drd_stop_using_nonstack_mem); VG_(track_die_mem_stack) (drd_stop_using_mem_stack); VG_(track_die_mem_stack_signal) (drd_stop_using_mem_stack_signal); + VG_(track_pre_deliver_signal) (drd_pre_deliver_signal); + VG_(track_post_deliver_signal) (drd_post_deliver_signal); VG_(track_start_client_code) (drd_start_client_code); VG_(track_pre_thread_ll_create) (drd_pre_thread_create); VG_(track_pre_thread_first_insn)(drd_post_thread_create); |
From: Christian B. <bor...@de...> - 2010-09-07 10:59:41
|
Am 02.09.2010 16:50, schrieb sv...@va...: > Author: bart > Date: 2010-09-02 15:50:41 +0100 (Thu, 02 Sep 2010) > New Revision: 11329 > > Log: > Made sure that DRD processes client programs that use SA_ONSTACK > correctly (e.g. Wine). > > > Modified: > trunk/drd/drd_main.c > > > Modified: trunk/drd/drd_main.c > =================================================================== > --- trunk/drd/drd_main.c 2010-09-02 14:44:17 UTC (rev 11328) > +++ trunk/drd/drd_main.c 2010-09-02 14:50:41 UTC (rev 11329) > @@ -326,9 +326,6 @@ > { > const Addr a2 = a1 + len; > > - if (len == 0) > - return; > - > tl_assert(a1 < a2); > > if (UNLIKELY(DRD_(any_address_is_traced)())) Bart, this basically removes r11304. Was this an oversight? len=0 can really happen in real code,e.g. on startup if the stack pointer is exactly on a page boundary and valgrind_main does initial stack permissions. (client_SP == seg->start) Please consider readding this check Christian |
From: Bart V. A. <bva...@ac...> - 2010-09-07 11:04:23
|
On Tue, Sep 7, 2010 at 12:59 PM, Christian Borntraeger <bor...@de...> wrote: > Am 02.09.2010 16:50, schrieb sv...@va...: >> Author: bart >> Date: 2010-09-02 15:50:41 +0100 (Thu, 02 Sep 2010) >> New Revision: 11329 >> >> Log: >> Made sure that DRD processes client programs that use SA_ONSTACK >> correctly (e.g. Wine). >> >> >> Modified: >> trunk/drd/drd_main.c >> >> >> Modified: trunk/drd/drd_main.c >> =================================================================== >> --- trunk/drd/drd_main.c 2010-09-02 14:44:17 UTC (rev 11328) >> +++ trunk/drd/drd_main.c 2010-09-02 14:50:41 UTC (rev 11329) >> @@ -326,9 +326,6 @@ >> { >> const Addr a2 = a1 + len; >> >> - if (len == 0) >> - return; >> - >> tl_assert(a1 < a2); >> >> if (UNLIKELY(DRD_(any_address_is_traced)())) > > Bart, > > this basically removes r11304. Was this an oversight? > len=0 can really happen in real code,e.g. on startup > if the stack pointer is exactly on a page boundary > and valgrind_main does initial stack permissions. > (client_SP == seg->start) The above commit indeed removes r11304. Revision 11304 was committed too early - it was committed while I was searching for the cause of an assertion failure triggered by a signal handler that was invoked on an alternate stack. None of the callers of the modified function should invoke it with len == 0. Bart. |
From: Christian B. <bor...@de...> - 2010-09-07 11:10:56
|
Am 07.09.2010 13:04, schrieb Bart Van Assche: > The above commit indeed removes r11304. Revision 11304 was committed > too early - it was committed while I was searching for the cause of an > assertion failure triggered by a signal handler that was invoked on an > alternate stack. > > None of the callers of the modified function should invoke it with len == 0. Look at coregrind/m_main.c:2188 VG_TRACK( die_mem_stack, seg->start, the_iifii.initial_client_SP - VG_STACK_REDZONE_SZB - seg->start ); If the environment happens to be the right size, (the_iifii.initial_client_SP - VG_STACK_REDZONE_SZB) might be exactly on a page start and therefore identical to seg-start. This would result in len=0, no? (coincidentially seen here on my system). |
From: Bart V. A. <bva...@ac...> - 2010-09-07 16:48:41
|
On Tue, Sep 7, 2010 at 1:10 PM, Christian Borntraeger <bor...@de...> wrote: > > Am 07.09.2010 13:04, schrieb Bart Van Assche: > > The above commit indeed removes r11304. Revision 11304 was committed > > too early - it was committed while I was searching for the cause of an > > assertion failure triggered by a signal handler that was invoked on an > > alternate stack. > > > > None of the callers of the modified function should invoke it with len == 0. > > > Look at coregrind/m_main.c:2188 > > VG_TRACK( die_mem_stack, > seg->start, > the_iifii.initial_client_SP - VG_STACK_REDZONE_SZB > - seg->start ); > > If the environment happens to be the right size, > (the_iifii.initial_client_SP - VG_STACK_REDZONE_SZB) might be exactly > on a page start and therefore identical to seg-start. This would > result in len=0, no? > > (coincidentally seen here on my system). Thanks for the feedback. Does r11342 (or later) work on your system without triggering an assertion failure ? Bart. |
From: Christian B. <bor...@de...> - 2010-09-08 06:43:39
|
Am 07.09.2010 18:48, schrieb Bart Van Assche: >> If the environment happens to be the right size, >> (the_iifii.initial_client_SP - VG_STACK_REDZONE_SZB) might be exactly >> on a page start and therefore identical to seg-start. This would >> result in len=0, no? >> >> (coincidentally seen here on my system). > > Thanks for the feedback. Does r11342 (or later) work on your system > without triggering an assertion failure ? Yes, that patch also solves my problem. Thanks Christian |
From: Julian S. <js...@ac...> - 2010-09-08 10:40:53
|
Umm, I must say, I don't think this is a good solution. Yes, it means that this specific call of VG_TRACK( die_mem_stack, ... ) does not send a zero length to drd_stop_using_mem. But there are other places where die_mem_stack and related functions (new_mem_*) are sent to tools, and we would have to audit all of those call sites too. The interface never promised that such functions would not be called with len == 0. It would I think be better for all the places that such a notification could be sent to (eg, drd_stop_using_mem) to be robust to a len == 0 value. I can see you possibly did this for performance related reasons, to remove a conditional branch. I think you can achieve the same without making the assumption "len != 0" by doing this void drd_stop_using_mem(const Addr a1, const SizeT len, const Bool is_stack_mem) { const Addr a2 = a1 + len; if (UNLIKELY(a1 >= a2)) { if (len == 0) return; tl_assert(0); } ... code as before .. } You still have only one conditional branch on the fast path, and the slow path differentiates the len == 0 case (harmless) from the wraparound case (we must assert). J On Wednesday, September 08, 2010, Christian Borntraeger wrote: > Am 07.09.2010 18:48, schrieb Bart Van Assche: > >> If the environment happens to be the right size, > >> (the_iifii.initial_client_SP - VG_STACK_REDZONE_SZB) might be exactly > >> on a page start and therefore identical to seg-start. This would > >> result in len=0, no? > >> > >> (coincidentally seen here on my system). > > > > Thanks for the feedback. Does r11342 (or later) work on your system > > without triggering an assertion failure ? > > Yes, that patch also solves my problem. Thanks > > Christian > > > --------------------------------------------------------------------------- > --- This SF.net Dev2Dev email is sponsored by: > > Show off your parallel programming skills. > Enter the Intel(R) Threading Challenge 2010. > http://p.sf.net/sfu/intel-thread-sfd > _______________________________________________ > Valgrind-developers mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-developers |
From: Bart V. A. <bva...@ac...> - 2010-09-08 16:43:32
|
On Wed, Sep 8, 2010 at 12:39 PM, Julian Seward <js...@ac...> wrote: > > Umm, I must say, I don't think this is a good solution. Yes, it means > that this specific call of VG_TRACK( die_mem_stack, ... ) does not send > a zero length to drd_stop_using_mem. But there are other places where > die_mem_stack and related functions (new_mem_*) are sent to tools, and > we would have to audit all of those call sites too. The interface never > promised that such functions would not be called with len == 0. > > It would I think be better for all the places that such a notification > could be sent to (eg, drd_stop_using_mem) to be robust to a len == 0 > value. The reason DRD did complain about empty address ranges was exactly to catch invocations of tracking functions by the core with empty address ranges. Since all code in DRD that did complain about empty address ranges works fine with empty address ranges, I have replaced the "tl_assert(a1 < a2)" statements by "tl_assert(a1 <= a2)" in r11346. Bart. |