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

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Yao" == Yao Qi <qiyaoltc@cn.ibm.com> writes:

I generally like this patch.  I've got a few comments about it.

Yao> 1. Move syscallList from  Syscall.java to Linux<ISA>.java.

Sounds good.

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.

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.

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...

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.

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.

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.

Tom