Re: [PATCH 9/9] um: switch ptrace FP register access to nolibc

["Berg, Benjamin" <benjamin.berg@intel.com>]

Hi,

On Mon, 2025-09-15 at 11:07 +0200, Thomas Weißschuh wrote:
> On 2025-09-15 09:11:15+0200, Benjamin Berg wrote:
> > From: Benjamin Berg <benjamin.berg@intel.com>
> > 
> > The registers.c file only contain the routines for floating point
> > register access in ptrace mode and initial size detection. The file can
> > be moved over to nolibc by replacing the ptrace libc call with a simple
> > wrapper that does a direct syscall.
> > 
> > Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
> > ---
> >  arch/x86/um/os-Linux/Makefile    |  5 ++++-
> >  arch/x86/um/os-Linux/registers.c | 22 ++++++++--------------
> >  2 files changed, 12 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/x86/um/os-Linux/Makefile b/arch/x86/um/os-Linux/Makefile
> > index 77a308aaa5ec..d37320430822 100644
> > --- a/arch/x86/um/os-Linux/Makefile
> > +++ b/arch/x86/um/os-Linux/Makefile
> > @@ -3,10 +3,13 @@
> >  # Licensed under the GPL
> >  #
> >  
> > -obj-y = registers.o mcontext.o
> > +obj-y = mcontext.o
> >  
> >  obj-$(CONFIG_X86_32) += tls.o
> >  
> >  USER_OBJS := $(obj-y)
> >  
> > +obj-y += registers.o
> > +NOLIBC_OBJS := registers.o
> > +
> >  include $(srctree)/arch/um/scripts/Makefile.rules
> > diff --git a/arch/x86/um/os-Linux/registers.c b/arch/x86/um/os-Linux/registers.c
> > index eb1cdadc8a61..55bce0d3f5d2 100644
> > --- a/arch/x86/um/os-Linux/registers.c
> > +++ b/arch/x86/um/os-Linux/registers.c
> > @@ -6,18 +6,20 @@
> >  
> >  #include <errno.h>
> 
> Given that you are explicitly disabling errno support for nolibc, is
> this include necessary?

I think it is technically correct as we do need ENODEV and ENOMEM to be
defined. Not that we actually need the include if we pull in nolibc.h.

Considering we would never build against libc, should we maybe just do
an explicit nolibc.h include and rely on it pulling in the rest
automatically? That seems a bit weird to me, but as-is we will never
notice when we forget an include.

> >  #include <stdlib.h>
> > -#include <sys/ptrace.h>
> > +#include <linux/ptrace.h>
> >  #ifdef __i386__
> >  #include <sys/user.h>
> >  #endif
> >  #include <longjmp.h>
> >  #include <sysdep/ptrace_user.h>
> > -#include <sys/uio.h>
> > +#include <linux/uio.h>
> 
> It looks fairly trivial to add sys/uio.h to nolibc.
> Only 'struct iovec' (already provided by the UAPI) and readv()/writev()
> are necessary.
> 
> >  #include <asm/sigcontext.h>
> >  #include <linux/elf.h>
> >  #include <registers.h>
> >  #include <sys/mman.h>
> >  
> > +#define my_ptrace(...) my_syscall4(__NR_ptrace, __VA_ARGS__)
> 
> Why not add sys/ptrace.h to nolibc and then use sys_ptrace()?

Honestly, I just got a bit lazy at that point and this was a reasonable
proof that mixing nolibc with libc works fine. You are absolutely right
that it would be better to add this to nolibc.

> In general I'm not a fan of the my_syscall() naming scheme and would
> like to change this in nolibc itself, so having fewer external users
> would be nice.

How about adding a sys_syscall macro? That would match the naming
scheme of the other functions. Then, once all users are ported, one can
simply change the my_ prefix to __nolibc_.

Benjamin

> 
> > +
> >  static unsigned long ptrace_regset;
> >  unsigned long host_fp_size;
> 
> (...)
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928