Bug 346411 - MIPS: SysRes::_valEx handling is incorrect
Summary: MIPS: SysRes::_valEx handling is incorrect
Status: REPORTED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (show other bugs)
Version: 3.10 SVN
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-20 21:05 UTC by Florian Krohm
Modified: 2016-04-09 12:46 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
A possible fix (10.76 KB, patch)
2015-05-02 20:15 UTC, Julian Seward
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Krohm 2015-04-20 21:05:24 UTC
The thread entitled SysRes::_valEx on valgrind-developers makes a good warm-up reading.

Function VG_(sr_as_string) may also need adjustment.
Comment 1 Julian Seward 2015-05-02 20:15:13 UTC
Created attachment 92378 [details]
A possible fix

This fixes it.  The basic idea is to pass the system call number to
sr_EQ (the comparison function for SysRes) so that it can decide
whether or not .valEx needs to be considered.  A previous attempt that
encoded a boolean in the SysRes to indicate validity/non-validity of
.valEx failed because there is a place where a SysRes is constructed
without the creating syscall number being available.

As part of this I made a new version of the SysRes type specifically
for mips{32,64}-linux rather than cluttering up the other linux
variants with a not-used field.  So there is some tidying up too.

Tested and believed working on a mips64-linux install on QEMU -- a
very slow experience.
Comment 2 Petar Jovanovic 2015-05-03 23:50:22 UTC
(In reply to Julian Seward from comment #1)
> Created attachment 92378 [details]
> A possible fix
> 
> This fixes it.  The basic idea is to pass the system call number to
> sr_EQ (the comparison function for SysRes) so that it can decide
> whether or not .valEx needs to be considered.  A previous attempt that
> encoded a boolean in the SysRes to indicate validity/non-validity of
> .valEx failed because there is a place where a SysRes is constructed
> without the creating syscall number being available.
> 
> As part of this I made a new version of the SysRes type specifically
> for mips{32,64}-linux rather than cluttering up the other linux
> variants with a not-used field.  So there is some tidying up too.
> 
> Tested and believed working on a mips64-linux install on QEMU -- a
> very slow experience.

Looks good to me. Thank you for working on this.
Comment 3 Florian Krohm 2015-05-04 09:18:56 UTC
In pub_tool_basics.h (and likewise in m_syscall.c):
I would change the #ifdeffery to have this structure:

#ifdef VGO_linux
// common code here, e.g. sr_Res etc.
// let's keep in common what can be kept in common and not replicate it
#if defined(VGP_mips32_linux) || defined(VGP_mips64_linux)
// mips stuff here
# else
// non-mips linux stuff here
#endif
#elif defined(VGO_darwin)
...
#endif

That seems like a more natural hierarchy to me. 
I really do not like to hard code the syscall numbers. This is a bit messy.
Instead make sr_EQ  forward to, say, VG_(eq_mips_SysRes)  which is defined
in m_syscall.c
Comment 4 Julian Seward 2015-07-08 17:10:55 UTC
(In reply to Julian Seward from comment #1)
> Created attachment 92378 [details]
> A possible fix

Committed, r15404, with some extra STATIC_ASSERTS to make the hardwired
syscall numbers safe.

Leaving open so that a better fix can later be landed, if anyone is inspired to do so.