|
From: Florian K. <fl...@ei...> - 2015-04-17 09:32:21
|
In coregrind/m_sigframe we currently have for the linux platform 9
versions of a function called 'extend' which extends the stack segment
-- all alike (modulo white space).
I'm factoring that out and while doing so I noticed that the 2
incarnations of that function for Darwin are subtly different.
The difference is:
x86-darwin:
VG_TRACK( new_mem_stack_signal,
addr - VG_STACK_REDZONE_SZB, size, tid );
amd64-darwin:
VG_TRACK( new_mem_stack_signal, addr - VG_STACK_REDZONE_SZB,
size + VG_STACK_REDZONE_SZB, tid );
It used to be that the amd64-darwin version of 'extend' was identical to
the x86-darwin version. In r13320 Bart changed the amd64-darwin version
to what it is today. The rationale is not the strongest one:
Darwin: Make stack growth tracking consistent with other architectures
I'm curious...... Why was x86-darwin not changed the same way?
Or the other way round: Was r13320 perhaps not the right thing to do?
Florian
|
|
From: Tom H. <to...@co...> - 2015-04-17 09:49:57
|
On 17/04/15 10:32, Florian Krohm wrote: > In coregrind/m_sigframe we currently have for the linux platform 9 > versions of a function called 'extend' which extends the stack segment > -- all alike (modulo white space). > I'm factoring that out and while doing so I noticed that the 2 > incarnations of that function for Darwin are subtly different. > The difference is: > > x86-darwin: > > VG_TRACK( new_mem_stack_signal, > addr - VG_STACK_REDZONE_SZB, size, tid ); > > amd64-darwin: > > VG_TRACK( new_mem_stack_signal, addr - VG_STACK_REDZONE_SZB, > size + VG_STACK_REDZONE_SZB, tid ); > > It used to be that the amd64-darwin version of 'extend' was identical to > the x86-darwin version. In r13320 Bart changed the amd64-darwin version > to what it is today. The rationale is not the strongest one: > Darwin: Make stack growth tracking consistent with other architectures > > I'm curious...... Why was x86-darwin not changed the same way? > Or the other way round: Was r13320 perhaps not the right thing to do? Well if Darwin is like other platforms then amd64 has a redzone and x86 doesn't - maybe that is the reason for the difference? Tom -- Tom Hughes (to...@co...) http://compton.nu/ |
|
From: Julian S. <js...@ac...> - 2015-04-17 10:19:19
|
>> In coregrind/m_sigframe we currently have for the linux platform 9
>> versions of a function called 'extend' which extends the stack segment
>> -- all alike (modulo white space).
>> I'm factoring that out
Excellent.
>> incarnations of that function for Darwin are subtly different.
>> The difference is:
>>
>> x86-darwin:
>>
>> VG_TRACK( new_mem_stack_signal,
>> addr - VG_STACK_REDZONE_SZB, size, tid );
>>
>> amd64-darwin:
>>
>> VG_TRACK( new_mem_stack_signal, addr - VG_STACK_REDZONE_SZB,
>> size + VG_STACK_REDZONE_SZB, tid );
My 2 euro-cents worth: on x86-darwin (and x86-linux), VG_STACK_REDZONE_SZB
is zero, so
VG_TRACK( new_mem_stack_signal,
addr - VG_STACK_REDZONE_SZB,
size, tid );
and
VG_TRACK( new_mem_stack_signal,
addr - VG_STACK_REDZONE_SZB,
size + VG_STACK_REDZONE_SZB, tid );
are equivalent.
On the whole I'd guess that the first version is actually correct.
Imagine, on amd64-linux, where the redzone size is 128 (bytes). That is,
the area up to 128 below %rsp is accessible. If we now want to allocate a
new block on the stack for delivering signals, with size |size|, the area
that we want to mark as "new" is new_rsp-128 .. old_rsp-128. So I'd say
that we don't want to extend the marked area by 128 (as in the second
version) since that will paint the pre-signal-delivery redzone as
addressible but uninitialised. And so if, after the signal frame is
cleared, the thread pulls a value out of the redzone and uses it, it
will be incorrectly marked as uninitialised.
This is just me guessing on the meaning of |size| here.
Commoning up the extend functions is great .. it means there's only one
place we have to prove correct :)
J
|
|
From: Florian K. <fl...@ei...> - 2015-04-18 10:35:36
|
On 17.04.2015 12:57, Julian Seward wrote: > On 17/04/15 12:47, Florian Krohm wrote: > >> Now, looking at the linux side of things: >> >> VG_TRACK( new_mem_stack_signal, addr - VG_STACK_REDZONE_SZB, >> size + VG_STACK_REDZONE_SZB, tid ); >> >> With your above argument (which is platform neutral), this does not look >> right either. > > I agree. It might be one of those things which has always been wrong, but > which nobody really noticed until now. > > If you're amenable to it, I'd suggest to use simply |size| in the new > merged-up version. If we get it wrong somehow, the worst that can happen > is that we'll get flooded with false positive errors in signal handlers > and we'll soon know something isn't right. So it's a low-risk change IMO. > r15109. Regtested on x86-64, s390, ppc64 Florian |
|
From: Rhys K. <rhy...@gm...> - 2015-05-01 00:03:03
|
No regressions seen on OS X for r15109. The change looks fine for this platform, good pickup. On 18 April 2015 at 20:35, Florian Krohm <fl...@ei...> wrote: > On 17.04.2015 12:57, Julian Seward wrote: > > On 17/04/15 12:47, Florian Krohm wrote: > > > >> Now, looking at the linux side of things: > >> > >> VG_TRACK( new_mem_stack_signal, addr - VG_STACK_REDZONE_SZB, > >> size + VG_STACK_REDZONE_SZB, tid ); > >> > >> With your above argument (which is platform neutral), this does not look > >> right either. > > > > I agree. It might be one of those things which has always been wrong, > but > > which nobody really noticed until now. > > > > If you're amenable to it, I'd suggest to use simply |size| in the new > > merged-up version. If we get it wrong somehow, the worst that can happen > > is that we'll get flooded with false positive errors in signal handlers > > and we'll soon know something isn't right. So it's a low-risk change > IMO. > > > > r15109. Regtested on x86-64, s390, ppc64 > > Florian > > > > ------------------------------------------------------------------------------ > 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: Florian K. <fl...@ei...> - 2015-04-17 10:47:47
|
>>> incarnations of that function for Darwin are subtly different.
>>> The difference is:
>>>
>>> x86-darwin:
>>>
>>> VG_TRACK( new_mem_stack_signal,
>>> addr - VG_STACK_REDZONE_SZB, size, tid );
>>>
>>> amd64-darwin:
>>>
>>> VG_TRACK( new_mem_stack_signal, addr - VG_STACK_REDZONE_SZB,
>>> size + VG_STACK_REDZONE_SZB, tid );
>
>
> On the whole I'd guess that the first version is actually correct.
>
> Imagine, on amd64-linux, where the redzone size is 128 (bytes). That is,
> the area up to 128 below %rsp is accessible. If we now want to allocate a
> new block on the stack for delivering signals, with size |size|, the area
> that we want to mark as "new" is new_rsp-128 .. old_rsp-128. So I'd say
> that we don't want to extend the marked area by 128 (as in the second
> version) since that will paint the pre-signal-delivery redzone as
> addressible but uninitialised. And so if, after the signal frame is
> cleared, the thread pulls a value out of the redzone and uses it, it
> will be incorrectly marked as uninitialised.
>
That makes sense to me.
> This is just me guessing on the meaning of |size| here.
|size| is either sizeof(struct sigframe) or sizeof(struct rt_sigframe).
You guess was excellent! :)
Now, looking at the linux side of things:
VG_TRACK( new_mem_stack_signal, addr - VG_STACK_REDZONE_SZB,
size + VG_STACK_REDZONE_SZB, tid );
With your above argument (which is platform neutral), this does not look
right either.
Florian
|
|
From: Julian S. <js...@ac...> - 2015-04-17 10:57:16
|
On 17/04/15 12:47, Florian Krohm wrote: > Now, looking at the linux side of things: > > VG_TRACK( new_mem_stack_signal, addr - VG_STACK_REDZONE_SZB, > size + VG_STACK_REDZONE_SZB, tid ); > > With your above argument (which is platform neutral), this does not look > right either. I agree. It might be one of those things which has always been wrong, but which nobody really noticed until now. If you're amenable to it, I'd suggest to use simply |size| in the new merged-up version. If we get it wrong somehow, the worst that can happen is that we'll get flooded with false positive errors in signal handlers and we'll soon know something isn't right. So it's a low-risk change IMO. J |