Re: [patch review] Syscall for multi-arch support

[Yao Qi <qiyaoltc at cn dot ibm dot com>]

On Fri, Sep 08, 2006 at 05:51:25PM -0600, Tom Tromey wrote:
> >>>>> "Yao" == Yao Qi <qiyaoltc@cn.ibm.com> writes:
> 
> I generally like this patch.  I've got a few comments about it.
> 
> Yao> It would take me a whole day to look up system call numbers for
> Yao> different architectures. :)
> 
> Automation would be good... too bad there's no way to do this
> dynamically.
Syscall spec of the same name is the same on all the architectures, so
most of the work is copy-paste.  Luckily, this work is
once-done-for-ever. :)
> 
> Yao> b) getSyscallList() and getUnknownSyscalls() return the
> Yao> architecture-dependent syscallList and unknown syscall hash map from
> Yao> Linux<ISA>.java
> 
> Ok, I think I didn't understand something important (and fundamental
> and obvious :-) about frysk.  If it is possible to request Syscall
> objects from different ISAs in the same frysk process, then I think
> either syscallByNum and syscallByName should be methods on the ISA
> object, or they should take the ISA as an argument.
> 
syscallByNum and syscallByName in class SyscallEventInfo takes the
task as an argument, and get ISA via task.getIsa().

syscallByNum and syscallByName in class SyscallEventInfo are
interfaces to "users" outside system calls, and they could get
information about syscall calls for a certain task.

> This makes the strace '-e' argument look a little weird now too.  If
> the user 'ftrace's all 'open' calls, should it be every open call from
> every ISA?  I suppose we could try to enumerate every ISA supported by
> the host platform...
Actually, I have modified this patch for some times in order to
support "ftrace -e", and "ftrace -e" makes some troubles to this
design. :)

If the user 'ftrace's all 'open' calls, frsk core
(SyscallEventInfo.getSyscall(task))could detect the ISA type of task we
currently ftraced, and *only* find syscall object in syscallList for
this ISA type.  This is to say, if we run "ftrace -e open ls" on our
pc(x86), frysk core fork and execv "/bin/ls", and then get the ISA
type(x86 here) of this task, and only find syscall object in
LinuxIa32.syscallList.

If a 32-bit program fork and exec a 64-bit program, the ISA type of 
this child task is 64-bit, and frysk core could find syscall object
in LinuxEMT64(or LinuxPPC64).syscallList.

> 
> Yao>  public interface Isa
> Yao>  {
> Yao> +
> Yao> +  String toString();
> 
> Not really necessary (since Object has toString) but I suppose it
> could be nice if you plan to document it differently.
I add this method in order to know what the current ISA is, and not it
seems that it is not necessary since the following code is "anti-OO".
> 
> Yao> +    if (isaName.equals("ia32"))
> Yao> +      {
> Yao> +	syscallList = LinuxIa32.syscallList;
> Yao> +      }
> Yao> +    else if (isaName.equals("x86-64"))
> Yao> +      {
> Yao> +	syscallList = LinuxEMT64.syscallList;
> Yao> +      }
> Yao> +    else if (isaName.equals("ppc64"))
> Yao> +      {
> Yao> +	syscallList = LinuxPPC64.syscallList;
> Yao> +      }
> Yao> +    else
> Yao> +      {
> Yao> +	syscallList = LinuxPPC.syscallList;
> Yao> +      }
> 
> Code like this is "anti-OO" -- every time you add an ISA you will have
> to find every such switch and add a case.
> 
> It is much better, IMO, to add a method to the ISA to return the
> desired attribute.
That is a good idea.
System call is an OS+ISA specific thing, so I do not put them in ISA
at first.  Currently, frysk is only supported on Linux, and assume
"OS" as Linux, and I could put getSyscallList() or syscallByNum() in ISA.
> 
> Yao> +  public static Syscall syscallByName (String name, Task task)
> [...]
> Yao> +    if (isaName.equals("ia32"))
> Yao> +      {
> Yao> +	syscall = Syscall.syscallByName (name, LinuxIa32.syscallList);
> Yao> +	if (syscall != null)
> Yao> +	  return syscall;
> 
> Likewise, this could simply delegate to the ISA and the ISA-specific
> oddities could be hidden there, where they (again IMO) belong.

-- 
Yao Qi