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

[Mark Wielaard <mark at klomp dot org>]

Hi,

On Fri, 2006-09-08 at 20:54 +0800, Yao Qi wrote:
> Some items are listed here to present what I did,
> 
> 1. Move syscallList from  Syscall.java to Linux<ISA>.java.
> syscallList is relative to OS+ISA, so I put this array in
> Linux<ISA>.java.

Personally I would place the actual lists on their own class files to
keep the rest of the class files smaller. But that depends on whether
you like all code to be together as much as possible, or you want code
as much as possible in separate files.

As already mentioned the OS + Isa setup is a little odd at the moment.
It seems part of the design works around the fact that a Isa isn't a
SyscallEventDecoder, only OS + Isa is. But there is no way to know which
Isa instance is a OS + Isa, and which are only an Isa.

If we aren't going to support other OSes than GNU/Linux soon I would
propose to collapse the Isa/Os classes together and make Isa implement
SyscallEventDecoder. Currently we have an extra layer of abstraction
that we aren't using at all.

>   However, I do not change the spec in syscallList.
> It would take me a whole day to look up system call numbers for
> different architectures. :)

As soon as the setup is good enough for IA32 we should probably divide
the task between people and make sure the tables are correct for the
various primary platforms that people use. I would love to see ftrace
work on x86_64 and strace and systemtap seem to have the appropriate
lists already so I can help with that arch.

> 2. New methods to Syscall.
> Tom submitted a patch about part of this, and I integrate his patch in
> my changes.  Add equals() in Syscall.
> 
> 3. New methods in SyscallEventInfo.
> a) getSyscall (Task task) return a Syscall object when a syscall happens.

Should document what happens when not in a syscall.

> b) getSyscallList() and getUnknownSyscalls() return the
> architecture-dependent syscallList and unknown syscall hash map from
> Linux<ISA>.java
> c) syscallByNum() and syscallByName() are also from Tom's patch.

I wonder whether you might want a map from names to syscalls. Depending
on how much syscallByName() is used. It currently does a linear search
through the array. This might be premature micro-optimizing though.

Would it make sense to extend Syscall with a getArguments() that returns
the number of arguments of the syscall? And maybe a way to access the
arguments with their correct type? It seems we encode that info in the
Syscall, but don't actually use it at all. That would be really handy
for inspecting the actual arguments and return values. But maybe this is
for later.

> 4. Setup three syscallList for Ia32 to index syscall object by syscall
> number and first argument directly.
> I discussed this with Mark here,
> http://sources.redhat.com/ml/frysk/2006-q3/msg00409.html
> 
> One syscallList is for common syscalls.(indexed by syscall number)
> One syscalLList is for socket "subcall".(indexed by first argument)
> One syscallList is for ipc "subcall".(indexed by first argument)

Where are these ipc subcalls documented?

> This patch could setup a framework for multi-arch syscall list, and now
> ftrace could recognize syscall "bind" or "send" as a standard system
> call, but there are something we could go on if this patch is on a
> right track.
> 1) Code syscallList spec for every architecture except Ia32.  I did not
> code syscall spec for ipc "subcalls".
> 2) fill in argument for these "subcalls" on ia32.
> On ia32, these "subcalls" share the same system call number, but with
> different name, and these "subcalls" could be distinguished by %ebx,
> and their arguments are in memory addressed by %ecx.
> 
> In my patch, I just let it return 0 now, so the output from ftrace is
> 0 or NULL.

Are the arguments not just shifted by one? if so then just make an
explicit SubSyscall class for them which just does
arg(Task t, int n) { return super.arg(t, n + 1); } 

> 
> Any comments are welcome!
> 
> Here is an output from ftrace on x86,
> (I input Ctrl-C to kill accept)

Nice output!
BTW, it should output the port it is listening on to stdin. Then you can
connect to that port from some other terminal and see it continue.

Cheers,

Mark