On Wed, Apr 23, 2014 at 02:22:32PM +0100, Peter Maydell wrote:
> On 23 April 2014 14:11, <riku.voi...@linaro.org> wrote:
> > From: Riku Voipio <riku.voi...@linaro.org>
> >
> > This series is primarily motivated to have a gcc-4.9 buildfix:
> >
> > linux-user/syscall.c: In function ‘host_to_target_stat64’:
> > linux-user/qemu.h:301:19: error: right-hand operand of comma expression has
> > no effect [-Werror=unused-value]
> > ((hptr), (x)), 0)
> >
> > removing the unused 0 moves the bar:
> >
> > linux-user/main.c: In function ‘arm_kernel_cmpxchg64_helper’:
> > linux-user/qemu.h:330:15: error: void value not ignored as it ought to be
> > __ret = __put_user((x), __hptr); \
> >
> > And after fixing that, we see there is a lot of reading the return
> > value of __put_user and __get_user in signal.c - apparently without
> > much of consistency as other functions do check and others don't...
> >
> > I'm not 100% sure that simply removing the checks is right way
> > and signal.c is quite a mess, so push this set early for comments
> > before spending more time on this approach.
>
> So the reason for this inconsistency is that QEMU's approach
> to checking that memory is accessible differs from the Linux kernel.
> In Linux, the check effectively happens on each get/put access
> (for free courtesy of the MMU), so the __get/__put_user accessors
> can each individually succeed or fail, and the return value must
> always be checked.
Right. I guess this will be good to put the commit message
or code comment in the 12/12 patch.
> In QEMU, we do an access check for an entire region via
> lock_user (or lock_user_struct) and catch the failure case there.
> So our __get/__put_user accessors can never fail. However we
> retained the return value because a lot of the code in signal.c
> is pretty much copy-and-pasted from the kernel sources with
> minor tweaks, and so it includes the "gather up and check
> return values at each step" code.
Ok, this explains. Thanks for quick answer!
> Generally in code review or if I've been tidying up or fixing the
> signal handling code for some other reason I've removed the
> pointless accumulation of return values. But a bunch of our
> less-maintained architectures still have it.
> > signal.c: remove __get/__put_user return value reading
> > signal.c setup_frame/x86: __put_user cleanup
> > signal.c: remove return value from copy_siginfo_to_user
> > signal.c: remove return value from setup_sigcontext
> > signal.c: remove return value from restore_sigcontext
> > RFC comment out restore_fpu_state (sparc)
> > do_sigaltstack: remove __get_user value check
> > do_sigreturn - remove __get_user checks
> > signal.c: setup_frame remove __put_user checks
> > remove __put/get error checks from ppc {save,restore}_user_regs
> > sparc64_set_context: remove __get_user checks
> > remove __get_user return check from PPC do_setcontext
> > fix gcc-4.9 compiler error on __{get,put]}_user
>
> Given that signal.c is a mix of code for all our supported
> architectures, it would be nice if these patch summary lines
> were a bit more consistent:
> linux-user/signal.c: Generic change affecting all archs
> linux-user/signal.c: PPC: Change affecting only PPC
Ok, no problem. As said, I wanted to see that the principle,
patch split etc is acceptable before going to detailed polishing
like summary lines.
> and so on.
> I should probably have another go at a tidyup I was
> looking at ages ago which splits linux-user/signal.c
> into linux-user/$arch/signal.c...
Yes, it was in my mind when repeatedly compiling all archs
for any single change in signal.c...
Riku
