|
From: Florian K. <fl...@ei...> - 2015-04-18 18:30:24
|
I noticed that the do_clone function in syswrap-tilegx-linux.c
does its own stack guessing and registration instead of using
the generic ML_(guess_and_register_stack) function which is
used by all other archs for that purpose. If at all possible
we should strive to use the generic code unless there is a *really*
good reason not to..
Looking at your code I see some subtle differences and am wondering
whether those are correct:
tilegx code:
ctst->client_stack_highest_byte = (Addr) VG_PGROUNDUP (sp);
ctst->client_stack_szB = ctst->client_stack_highest_byte - seg->start;
generic code:
tst->client_stack_highest_byte = (Addr)VG_PGROUNDUP(sp)-1;
tst->client_stack_szB = tst->client_stack_highest_byte - seg->start + 1;
That looks like an off-by-one error to me. Could you elaborate?
I'm thinking about the patch below.
Florian
Index: coregrind/m_syswrap/syswrap-tilegx-linux.c
===================================================================
--- coregrind/m_syswrap/syswrap-tilegx-linux.c (revision 15111)
+++ coregrind/m_syswrap/syswrap-tilegx-linux.c (working copy)
@@ -337,7 +337,6 @@
ThreadState * ctst = VG_ (get_ThreadState) (ctid);
Long ret = 0;
Long * stack;
- NSegment const *seg;
SysRes res;
vki_sigset_t blockall, savedmask;
@@ -372,23 +371,8 @@
See #226116. */
ctst->os_state.threadgroup = ptst->os_state.threadgroup;
- seg = VG_ (am_find_nsegment) ((Addr) sp);
+ ML_(guess_and_register_stack) (sp, ctst);
- if (seg && seg->kind != SkResvn) {
- ctst->client_stack_highest_byte = (Addr) VG_PGROUNDUP (sp);
- ctst->client_stack_szB = ctst->client_stack_highest_byte - seg->start;
- VG_ (register_stack) (seg->start, ctst->client_stack_highest_byte);
- if (debug)
- VG_ (printf) ("tid %d: guessed client stack range %#lx-%#lx\n",
-
- ctid, seg->start, VG_PGROUNDUP (sp));
- } else {
- VG_ (message) (Vg_UserMsg,
- "!? New thread %d starts with sp+%#lx) unmapped\n",
- ctid, sp);
- ctst->client_stack_szB = 0;
- }
-
VG_TRACK (pre_thread_ll_create, ptid, ctid);
if (flags & VKI_CLONE_SETTLS) {
if (debug)
|
|
From: Philippe W. <phi...@sk...> - 2015-04-19 12:29:25
|
On Sat, 2015-04-18 at 20:30 +0200, Florian Krohm wrote:
> I noticed that the do_clone function in syswrap-tilegx-linux.c
> does its own stack guessing and registration instead of using
> the generic ML_(guess_and_register_stack) function which is
> used by all other archs for that purpose. If at all possible
> we should strive to use the generic code unless there is a *really*
> good reason not to..
As I understood, the tilegx arch development was done
in the era of valgrind 3.7 or so. At that time
the semantic of stack boundary (bound included or not)
was not consistent/documented.
The semantic was documented and made systematic in r14392 in 2014.
I guess this change was not tracked in the tilegx.
The common/consistent/documented semantic should also be used
for tilegx.
So, the patch below looks a good idea.
Philippe
>
> Looking at your code I see some subtle differences and am wondering
> whether those are correct:
>
> tilegx code:
>
> ctst->client_stack_highest_byte = (Addr) VG_PGROUNDUP (sp);
> ctst->client_stack_szB = ctst->client_stack_highest_byte - seg->start;
>
> generic code:
>
> tst->client_stack_highest_byte = (Addr)VG_PGROUNDUP(sp)-1;
> tst->client_stack_szB = tst->client_stack_highest_byte - seg->start + 1;
>
> That looks like an off-by-one error to me. Could you elaborate?
>
> I'm thinking about the patch below.
>
> Florian
>
> Index: coregrind/m_syswrap/syswrap-tilegx-linux.c
> ===================================================================
> --- coregrind/m_syswrap/syswrap-tilegx-linux.c (revision 15111)
> +++ coregrind/m_syswrap/syswrap-tilegx-linux.c (working copy)
> @@ -337,7 +337,6 @@
> ThreadState * ctst = VG_ (get_ThreadState) (ctid);
> Long ret = 0;
> Long * stack;
> - NSegment const *seg;
> SysRes res;
> vki_sigset_t blockall, savedmask;
>
> @@ -372,23 +371,8 @@
> See #226116. */
>
> ctst->os_state.threadgroup = ptst->os_state.threadgroup;
> - seg = VG_ (am_find_nsegment) ((Addr) sp);
> + ML_(guess_and_register_stack) (sp, ctst);
>
> - if (seg && seg->kind != SkResvn) {
> - ctst->client_stack_highest_byte = (Addr) VG_PGROUNDUP (sp);
> - ctst->client_stack_szB = ctst->client_stack_highest_byte - seg->start;
> - VG_ (register_stack) (seg->start, ctst->client_stack_highest_byte);
> - if (debug)
> - VG_ (printf) ("tid %d: guessed client stack range %#lx-%#lx\n",
> -
> - ctid, seg->start, VG_PGROUNDUP (sp));
> - } else {
> - VG_ (message) (Vg_UserMsg,
> - "!? New thread %d starts with sp+%#lx) unmapped\n",
> - ctid, sp);
> - ctst->client_stack_szB = 0;
> - }
> -
> VG_TRACK (pre_thread_ll_create, ptid, ctid);
> if (flags & VKI_CLONE_SETTLS) {
> if (debug)
>
> ------------------------------------------------------------------------------
> BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT
> Develop your own process in accordance with the BPMN 2 standard
> Learn Process modeling best practices with Bonita BPM through live exercises
> http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_
> source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF
> _______________________________________________
> Valgrind-developers mailing list
> Val...@li...
> https://lists.sourceforge.net/lists/listinfo/valgrind-developers
|
|
From: Liming S. <ls...@ez...> - 2015-04-20 01:01:01
|
I ran a quick test. The regression result is about the same as before. It looks safe to check in the patch...
Thanks,
Liming
________________________________________
From: Florian Krohm <fl...@ei...>
Sent: Saturday, April 18, 2015 2:30 PM
To: Zhigang Liu
Cc: Valgrind Developers
Subject: [Valgrind-developers] tilegx: do_clone() stack guessing and registration
I noticed that the do_clone function in syswrap-tilegx-linux.c
does its own stack guessing and registration instead of using
the generic ML_(guess_and_register_stack) function which is
used by all other archs for that purpose. If at all possible
we should strive to use the generic code unless there is a *really*
good reason not to..
Looking at your code I see some subtle differences and am wondering
whether those are correct:
tilegx code:
ctst->client_stack_highest_byte = (Addr) VG_PGROUNDUP (sp);
ctst->client_stack_szB = ctst->client_stack_highest_byte - seg->start;
generic code:
tst->client_stack_highest_byte = (Addr)VG_PGROUNDUP(sp)-1;
tst->client_stack_szB = tst->client_stack_highest_byte - seg->start + 1;
That looks like an off-by-one error to me. Could you elaborate?
I'm thinking about the patch below.
Florian
Index: coregrind/m_syswrap/syswrap-tilegx-linux.c
===================================================================
--- coregrind/m_syswrap/syswrap-tilegx-linux.c (revision 15111)
+++ coregrind/m_syswrap/syswrap-tilegx-linux.c (working copy)
@@ -337,7 +337,6 @@
ThreadState * ctst = VG_ (get_ThreadState) (ctid);
Long ret = 0;
Long * stack;
- NSegment const *seg;
SysRes res;
vki_sigset_t blockall, savedmask;
@@ -372,23 +371,8 @@
See #226116. */
ctst->os_state.threadgroup = ptst->os_state.threadgroup;
- seg = VG_ (am_find_nsegment) ((Addr) sp);
+ ML_(guess_and_register_stack) (sp, ctst);
- if (seg && seg->kind != SkResvn) {
- ctst->client_stack_highest_byte = (Addr) VG_PGROUNDUP (sp);
- ctst->client_stack_szB = ctst->client_stack_highest_byte - seg->start;
- VG_ (register_stack) (seg->start, ctst->client_stack_highest_byte);
- if (debug)
- VG_ (printf) ("tid %d: guessed client stack range %#lx-%#lx\n",
-
- ctid, seg->start, VG_PGROUNDUP (sp));
- } else {
- VG_ (message) (Vg_UserMsg,
- "!? New thread %d starts with sp+%#lx) unmapped\n",
- ctid, sp);
- ctst->client_stack_szB = 0;
- }
-
VG_TRACK (pre_thread_ll_create, ptid, ctid);
if (flags & VKI_CLONE_SETTLS) {
if (debug)
------------------------------------------------------------------------------
BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT
Develop your own process in accordance with the BPMN 2 standard
Learn Process modeling best practices with Bonita BPM through live exercises
http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_
source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF
_______________________________________________
Valgrind-developers mailing list
Val...@li...
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
|