Re: [PATCH 1/5] all: s390: move wrapper infrastructure to generic headers

[Heiko Carstens]

On Tue, Feb 02, 2016 at 06:43:31PM +0300, Yury Norov wrote:
> > Well, I'd like to have some proof by the compiler or linker that nothing
> > went wrong. Which seems hard if only selected system call defines will be
> > converted to the new defines.
> > 
> > How can you tell that nothing has been forgotten?
> > 
> > Also, what happens if the prototype of a system call get's changed shortly
> > after it was merged. We might miss such changes and have bugs.
> > 
> 
> As for now, there's no such proof, and everything is OK. Syscall ABI
> is extremely conservative, and Greg KH, and other people spent a lot
> of efforts to keep it that way. This is the only reason for me to not
> worry much about it. Modification of syscall ABI is virtually
> impossible now, because it breaks binary compatibility. Even addition
> of new syscall is very difficult procedure.
> (Documentation/adding-syscalls.txt begins with section "System Call
> Alternatives".)

Well... during the years a lot of system calls have been added. And we've
also seen last-minute changes or reverts. So I don't share your optimistic
view here :)

See e.g. 485d52768685 ("sys_personality: change sys_personality() to accept
"unsigned int" instead of u_long") would have been a candidate which could
silently break architectures which need compat wrappers.

> We can invent some protection, but it will cost us in complexity and/or
> runtime delays. Because syscall ABI is so stable, I think it's OK to
> review wrappers carefully once, and they will be fine for long time.

Here I don't agree with you. These interfaces are so important that I'd
like to have a waterproof method that these don't break.  If this involves
a couple of superfluous instructions then that's what I'm willing to pay
for it.

> > Before doing that I think you should actually revert this patch: my commit
> > 7681df456f97 ("s390/compat: remove superfluous compat wrappers") probably
> > wasn't a very bright idea :)
> This patch is OK for me. pid_t, uid_t, gid_t, unsigned and signed int
> types are all 32-bit both on LP64 and ILP32. Normally, compiler should
> care about top halves... Did I miss something?

The patch was correct when writing it, but e.g. a patch like named above
would introduce a possible bug which would go in unnoticed.
The only thing we save is a _single_ unconditional branch here. I'd say
that's well worth it if you get a (hopefully) always bug free sign and zero
extension infrastructure in return.

> I don't know much about s390 specifics. Maybe because of that I do not
> understand completely your worries. I'm OK with both 1st and 2nd
> version, but I'd choose 2nd one because it allows inlines, and we
> don't need the compat_wrapper.c.

It would be only nicer if we can guarentee correctness all the time. That
being said I'm about to revert my own commit :)

So if you want to go without compat_wrapper.c then we should have a
solution which will do the right thing all the time without that a system
call author has to know about the sign and zero extension issue some
architectures face. It _will_ go wrong.