|
From: Florian K. <fl...@ei...> - 2015-03-23 19:36:37
|
r12616 (merge mips32 port) adds a new field '_valEx' to the SysRes structure. However, the function sr_EQ which tests two SysRes values for equality ignores that field. That doesn't look right. But in case it is correct there should be some verbiage as to why _valEx does not matter for equality. Florian |
|
From: Petar J. <mip...@gm...> - 2015-04-03 02:14:34
|
Hi Florian, Sorry for not responding to this earlier. Field _valEx is used on MIPS only for pipe system call when kernel can return an array of two file descriptors. Otherwise, this field is undefined (actually, it has a value that happened to be in v1 register, which can be an arbitrary value and it is not deterministic). Thus, comparing _valEx does not make sense for an arbitrary SysRes. Your yesterday's change r15060 breaks MIPS port in general, as comparing _valEx will fail immediately. Petar On Mon, Mar 23, 2015 at 8:36 PM, Florian Krohm <fl...@ei...> wrote: > r12616 (merge mips32 port) adds a new field '_valEx' to the SysRes > structure. However, the function sr_EQ which tests two SysRes values for > equality ignores that field. > That doesn't look right. But in case it is correct there should be some > verbiage as to why _valEx does not matter for equality. > > Florian > |
|
From: Florian K. <fl...@ei...> - 2015-04-03 14:30:09
|
On 03.04.2015 04:14, Petar Jovanovic wrote:
>
> Field _valEx is used on MIPS only for pipe system call when kernel can
> return an array of two file descriptors. Otherwise, this field is undefined
> (actually, it has a value that happened to be in v1 register, which can be
> an arbitrary value and it is not deterministic). Thus, comparing _valEx
> does not make sense for an arbitrary SysRes.
Yes. But comparing _valEx makes sense when it has a deterministic value
(i.e. after a pipe call). So not comparing the _valEx value at all does
not sound right either. How is this (for mips64):
Index: coregrind/m_syscall.c
===================================================================
--- coregrind/m_syscall.c (revision 15062)
+++ coregrind/m_syscall.c (working copy)
@@ -859,6 +859,7 @@
ULong V0 = do_syscall_WRK(a1,a2,a3,a4,a5,a6,sysno,v1_a3);
ULong V1 = (ULong)v1_a3[0];
ULong A3 = (ULong)v1_a3[1];
+ if (sysno != _NR_pipe) V1 = 0; // V1 is unused for this syscall
return VG_(mk_SysRes_mips64_linux)( V0, V1, A3 );
#else
Does that help?
> Your yesterday's change r15060 breaks MIPS port in general, as comparing
> _valEx will fail immediately.
I made the change because the code as is was looked just wrong.
Florian
|
|
From: Petar J. <mip...@gm...> - 2015-04-04 03:15:52
|
On Fri, Apr 3, 2015 at 4:29 PM, Florian Krohm <fl...@ei...> wrote: > Yes. But comparing _valEx makes sense when it has a deterministic value > (i.e. after a pipe call). So not comparing the _valEx value at all does > not sound right either. I agree. > How is this (for mips64): > > Index: coregrind/m_syscall.c > =================================================================== > --- coregrind/m_syscall.c (revision 15062) > +++ coregrind/m_syscall.c (working copy) > @@ -859,6 +859,7 @@ > ULong V0 = do_syscall_WRK(a1,a2,a3,a4,a5,a6,sysno,v1_a3); > ULong V1 = (ULong)v1_a3[0]; > ULong A3 = (ULong)v1_a3[1]; > + if (sysno != _NR_pipe) V1 = 0; // V1 is unused for this syscall > return VG_(mk_SysRes_mips64_linux)( V0, V1, A3 ); > > #else > > Does that help? > > I have tested a similar change right away, but it was not sufficient. I believe I need to modify at least putSyscallStatusIntoGuestState(), will see if anything else is missing after more testing. Petar |
|
From: Petar J. <mip...@gm...> - 2015-04-08 02:40:32
|
Hi Florian, I have just given a second look to this issue. It seems to me that if we want to make sure SysRes is always correctly initialized (i.e. to make sure _valEx and v1 in guest state are set to zero for non-pipe calls or for unsuccessful pipe call, we need to modify several spots for MIPS: - within VG_(do_syscall), after syscall is executed - ML_(do_syscall_for_client_WRK) in syscall-mips64-linux.S - putSyscallStatusIntoGuestState - hack around vki_ucontext and VG_UCONTEXT_SYSCALL_SYSRES And we would be doing it to satisfy condition sr_EQ introduced in r15060 that's used for assert only. Do you think it is worth adding these changes and workarounds? If you believe so, I will make the necessary changes. Regards, Petar On Sat, Apr 4, 2015 at 5:15 AM, Petar Jovanovic <mip...@gm...> wrote: > > On Fri, Apr 3, 2015 at 4:29 PM, Florian Krohm <fl...@ei...> > wrote: > >> Yes. But comparing _valEx makes sense when it has a deterministic value >> (i.e. after a pipe call). So not comparing the _valEx value at all does >> not sound right either. > > > I agree. > > >> How is this (for mips64): >> >> Index: coregrind/m_syscall.c >> =================================================================== >> --- coregrind/m_syscall.c (revision 15062) >> +++ coregrind/m_syscall.c (working copy) >> @@ -859,6 +859,7 @@ >> ULong V0 = do_syscall_WRK(a1,a2,a3,a4,a5,a6,sysno,v1_a3); >> ULong V1 = (ULong)v1_a3[0]; >> ULong A3 = (ULong)v1_a3[1]; >> + if (sysno != _NR_pipe) V1 = 0; // V1 is unused for this syscall >> return VG_(mk_SysRes_mips64_linux)( V0, V1, A3 ); >> >> #else >> >> Does that help? >> >> > I have tested a similar change right away, but it was not sufficient. > I believe I need to modify at least putSyscallStatusIntoGuestState(), > will see if anything else is missing after more testing. > > Petar > |
|
From: Julian S. <js...@ac...> - 2015-04-08 07:26:54
|
What would happen if, for mips, we added to SysRes, a boolean field that indicates whether or not _valEx contains a live value? It seems to me that at the moment we have "unclean" semantics because we can't know, from a SysRes in isolation, whether or not _valEx needs to be taken into account. It sounds, from Petar's earlier mail, that forcing _valEx to always be zero in cases where it doesn't carry a live value, breaks stuff. And that doesn't really reflect the kernel interface's semantics anyway, since that implicitly seems to require that a SysRes is interpreted in the context of whether the syscall that created it is or isn't sys_pipe. Hence my proposal to encode that information (as a bool) inside SysRes itself. I agree it would be nice to get this fixed cleanly, on the whole. J On 08/04/15 04:40, Petar Jovanovic wrote: > Hi Florian, > > I have just given a second look to this issue. > It seems to me that if we want to make sure SysRes is always correctly > initialized (i.e. to make sure _valEx and v1 in guest state are set to zero > for non-pipe calls or for unsuccessful pipe call, we need to modify several > spots for MIPS: > > - within VG_(do_syscall), after syscall is executed > - ML_(do_syscall_for_client_WRK) in syscall-mips64-linux.S > - putSyscallStatusIntoGuestState > - hack around vki_ucontext and VG_UCONTEXT_SYSCALL_SYSRES > > And we would be doing it to satisfy condition sr_EQ introduced in r15060 > that's used for assert only. Do you think it is worth adding these changes > and workarounds? If you believe so, I will make the necessary changes. > > Regards, > Petar > > > On Sat, Apr 4, 2015 at 5:15 AM, Petar Jovanovic <mip...@gm...> wrote: > >> >> On Fri, Apr 3, 2015 at 4:29 PM, Florian Krohm <fl...@ei...> >> wrote: >> >>> Yes. But comparing _valEx makes sense when it has a deterministic value >>> (i.e. after a pipe call). So not comparing the _valEx value at all does >>> not sound right either. >> >> >> I agree. >> >> >>> How is this (for mips64): >>> >>> Index: coregrind/m_syscall.c >>> =================================================================== >>> --- coregrind/m_syscall.c (revision 15062) >>> +++ coregrind/m_syscall.c (working copy) >>> @@ -859,6 +859,7 @@ >>> ULong V0 = do_syscall_WRK(a1,a2,a3,a4,a5,a6,sysno,v1_a3); >>> ULong V1 = (ULong)v1_a3[0]; >>> ULong A3 = (ULong)v1_a3[1]; >>> + if (sysno != _NR_pipe) V1 = 0; // V1 is unused for this syscall >>> return VG_(mk_SysRes_mips64_linux)( V0, V1, A3 ); >>> >>> #else >>> >>> Does that help? >>> >>> >> I have tested a similar change right away, but it was not sufficient. >> I believe I need to modify at least putSyscallStatusIntoGuestState(), >> will see if anything else is missing after more testing. >> >> Petar >> > > > > ------------------------------------------------------------------------------ > 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: Petar J. <mip...@gm...> - 2015-04-08 07:52:24
|
In order to set that boolean field correctly, at each point in which we create SysRes, we need to know for which system call SysRes was created. Similar solution - that would likely require similar number of changes - would be to have SysRes carry its system call number. Petar On Wed, Apr 8, 2015 at 9:26 AM, Julian Seward <js...@ac...> wrote: > > What would happen if, for mips, we added to SysRes, a boolean field that > indicates whether or not _valEx contains a live value? It seems to me > that at the moment we have "unclean" semantics because we can't know, from > a SysRes in isolation, whether or not _valEx needs to be taken into > account. > > It sounds, from Petar's earlier mail, that forcing _valEx to always be zero > in cases where it doesn't carry a live value, breaks stuff. And that > doesn't really reflect the kernel interface's semantics anyway, since that > implicitly seems to require that a SysRes is interpreted in the context of > whether the syscall that created it is or isn't sys_pipe. Hence my > proposal > to encode that information (as a bool) inside SysRes itself. > > I agree it would be nice to get this fixed cleanly, on the whole. > > J > > On 08/04/15 04:40, Petar Jovanovic wrote: > > Hi Florian, > > > > I have just given a second look to this issue. > > It seems to me that if we want to make sure SysRes is always correctly > > initialized (i.e. to make sure _valEx and v1 in guest state are set to > zero > > for non-pipe calls or for unsuccessful pipe call, we need to modify > several > > spots for MIPS: > > > > - within VG_(do_syscall), after syscall is executed > > - ML_(do_syscall_for_client_WRK) in syscall-mips64-linux.S > > - putSyscallStatusIntoGuestState > > - hack around vki_ucontext and VG_UCONTEXT_SYSCALL_SYSRES > > > > And we would be doing it to satisfy condition sr_EQ introduced in r15060 > > that's used for assert only. Do you think it is worth adding these > changes > > and workarounds? If you believe so, I will make the necessary changes. > > > > Regards, > > Petar > > > > > > On Sat, Apr 4, 2015 at 5:15 AM, Petar Jovanovic <mip...@gm...> > wrote: > > > >> > >> On Fri, Apr 3, 2015 at 4:29 PM, Florian Krohm <fl...@ei...> > >> wrote: > >> > >>> Yes. But comparing _valEx makes sense when it has a deterministic value > >>> (i.e. after a pipe call). So not comparing the _valEx value at all does > >>> not sound right either. > >> > >> > >> I agree. > >> > >> > >>> How is this (for mips64): > >>> > >>> Index: coregrind/m_syscall.c > >>> =================================================================== > >>> --- coregrind/m_syscall.c (revision 15062) > >>> +++ coregrind/m_syscall.c (working copy) > >>> @@ -859,6 +859,7 @@ > >>> ULong V0 = do_syscall_WRK(a1,a2,a3,a4,a5,a6,sysno,v1_a3); > >>> ULong V1 = (ULong)v1_a3[0]; > >>> ULong A3 = (ULong)v1_a3[1]; > >>> + if (sysno != _NR_pipe) V1 = 0; // V1 is unused for this syscall > >>> return VG_(mk_SysRes_mips64_linux)( V0, V1, A3 ); > >>> > >>> #else > >>> > >>> Does that help? > >>> > >>> > >> I have tested a similar change right away, but it was not sufficient. > >> I believe I need to modify at least putSyscallStatusIntoGuestState(), > >> will see if anything else is missing after more testing. > >> > >> Petar > >> > > > > > > > > > ------------------------------------------------------------------------------ > > 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: Julian S. <js...@ac...> - 2015-04-08 08:40:53
|
On 08/04/15 09:52, Petar Jovanovic wrote: > In order to set that boolean field correctly, at each point in which we > create SysRes, we need to know for which system call SysRes was created. > Similar solution - that would likely require similar number of changes - > would be to have SysRes carry its system call number. I agree. It may be however that there are not many places where we construct a SysRes. I'll have a look at it. The reason I favour this solution is that, until now, SysRes has been an object that holds the (non-memory) results of a syscall, and can answer the following questions: 1. Did the syscall fail and if so what are the error-code bits? 2. Did the syscall succeed and if so what are the result bits? 3. Are you the same as this other SysRes ? But with the mips-pipe problem, SysRes can no longer reliably answer (2) or (3), because we don't know whether _valEx is part of the result bits or not. Hence we basically break the interface to this type. FWIW, the Darwin version of SysRes already contains similar machinery so as to make (1) (2) and (3) work reliably. So I don't think it's a big problem to do something similar for mips. Are there any community machines still alive, that I can try this out on? J |
|
From: Florian K. <fl...@ei...> - 2015-04-08 09:05:14
|
On 08.04.2015 09:52, Petar Jovanovic wrote: > In order to set that boolean field correctly, at each point in which we > create SysRes, we need to know for which system call SysRes was created. > Similar solution - that would likely require similar number of changes - > would be to have SysRes carry its system call number. The advantage of the Boolean field would be that it is architecture neutral. It simply would be set to True for all architectures other than mips. If we recorded the system call number more work would be required for other architectures (or something less intuitive such as setting a fake system call number). So I'm favouring the Boolean field as well. Isn't there also a correctness issue with the status quo? You might get a false result by comparing a SysRes from a pipe call with a SysRes from a non-pipe call. If I understand correctly, such a comparison should produce a "not-equal" result whereas today it could produce an "equal" result. Florian |
|
From: Julian S. <js...@ac...> - 2015-04-08 09:15:53
|
On 08/04/15 11:05, Florian Krohm wrote: > The advantage of the Boolean field would be that it is architecture > neutral. Yes. If we did make such a change, I would prefer the Boolean version rather than embedding the syscall number. (eg, which isn't even a single number on Darwin). I'd also prefer to do it by removing _valEx from all the Linux targets except mips*-linux and having a mips-specific version that includes _valEx and the new bool. This avoids having fields with no meaning for non-mips targets, which IMO would be confusing. > Isn't there also a correctness issue with the status quo? You might get > a false result by comparing a SysRes from a pipe call with a SysRes from > a non-pipe call. That is true, but that is not a mips-specific problem -- it applies to all targets. You need to know that two SysRess came from the same syscall before comparing them. Part of the value of this discussion (I've now understood) is to more clearly define the semantics of SysRes that it so far has been. As well as fixing the problem. J |
|
From: Florian K. <fl...@ei...> - 2015-04-08 09:46:07
|
On 08.04.2015 11:15, Julian Seward wrote:
>
>> Isn't there also a correctness issue with the status quo? You might get
>> a false result by comparing a SysRes from a pipe call with a SysRes from
>> a non-pipe call.
>
> That is true, but that is not a mips-specific problem -- it applies to all
> targets. You need to know that two SysRess came from the same syscall
> before comparing them.
Oh, you are right. Is equality of sycall numbers ensured today when we
do an sr_EQ? I briefly looked at the code and the single use of sr_EQ is
in eq_SyscallStatus which again is called once. Perhaps system call
numbers in that context are guaranteed to be the same, but it is not
obvious to me.
Which then raises the question of whether the system call number should
be added to SysRes for all archs (and maintained properly) and taken
into consideration in sr_EQ.
Perhaps:
typedef
struct {
UWord _sysno;
UWord _val
#if defined(VGA_mips32) || defined(VGA_mips64)
Bool _valEx;
#endif
}
SysRes;
static inline Bool sr_EQ ( SysRes sr1, SysRes sr2 ) {
return sr1._sysno == sr2._sysno // <----<< new
&& sr1._val == sr2._val
&& sr1._isError == sr2._isError
#if defined(VGA_mips32) || defined(VGA_mips64)
&& (sr1._sysno != _NR_pipe || sr1._valEx == sr2._valEx)
#endif
;
}
Florian
|
|
From: Julian S. <js...@ac...> - 2015-05-02 20:19:46
|
I made what I think is a clean fix for this, at https://bugs.kde.org/show_bug.cgi?id=346411#c1 Florian and Petar, can you look at the patch and see if you are happy with it? I verified it works on mips64-linux. J |
|
From: Crestez D. L. <cdl...@gm...> - 2015-04-20 12:34:02
|
On 04/03/2015 05:14 AM, Petar Jovanovic wrote: > Hi Florian, > > Sorry for not responding to this earlier. > Field _valEx is used on MIPS only for pipe system call when kernel can return an array of two file descriptors. Otherwise, this field is undefined (actually, it has a value that happened to be in v1 register, which can be an arbitrary value and it is not deterministic). Thus, comparing _valEx does not make sense for an arbitrary SysRes. > > Your yesterday's change r15060 breaks MIPS port in general, as comparing _valEx will fail immediately. MIPS support has been broken in trunk for two weeks. Does anyone have a fix for this incoming? Maybe it should be posted as a bug in the tracker. It's not clear how valEx should be treated for syscalls other than pipe() since it's left undefined by the linux syscall ABI. I think that simply defining that all other syscalls return zero in valEx would be an acceptable solution. Regards, Leonard |
|
From: Julian S. <js...@ac...> - 2015-04-20 13:05:34
|
>> Your yesterday's change r15060 breaks MIPS port in general, > as comparing _valEx will fail immediately. > > MIPS support has been broken in trunk for two weeks. Does anyone > have a fix for this incoming? Maybe it should be posted as a bug > in the tracker. Perhaps it would be wise to back out 15060 until such time as it gets fixed properly, and at the same time file a bug in the tracker for the underlying problem. J |
|
From: Florian K. <fl...@ei...> - 2015-04-20 21:10:33
|
On 20.04.2015 15:05, Julian Seward wrote: > >>> Your yesterday's change r15060 breaks MIPS port in general, >> as comparing _valEx will fail immediately. >> >> MIPS support has been broken in trunk for two weeks. Does anyone >> have a fix for this incoming? Maybe it should be posted as a bug >> in the tracker. > > Perhaps it would be wise to back out 15060 until such time as it > gets fixed properly, and at the same time file a bug in the tracker > for the underlying problem. It would be even better if the issue was fixed. I've made some adjustments. In particular, I removed _valEx from the common code. We don't want it there. https://bugs.kde.org/show_bug.cgi?id=346411 Florian |