Re: [musl] [PATCH 02/14] time64: Don't make aliases to nonexistent syscalls

[Rich Felker <dalias@libc.org>]

On Thu, Sep 03, 2020 at 03:36:00PM -0400, Stefan O'Rear wrote:
> On Thu, Sep 3, 2020, at 11:56 AM, Rich Felker wrote:
> > On Thu, Sep 03, 2020 at 07:22:57AM -0400, Stefan O'Rear wrote:
> > > riscv32 and future architectures lack the _time32 variants entirely, so
> > > don't try to use their numbers.
> > > ---
> > >  src/internal/syscall.h | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/src/internal/syscall.h b/src/internal/syscall.h
> > > index d5f294d4..66fc4e5c 100644
> > > --- a/src/internal/syscall.h
> > > +++ b/src/internal/syscall.h
> > > @@ -201,6 +201,7 @@ static inline long __alt_socketcall(int sys, int sock, int cp, long a, long b, l
> > >  #define SYS_sendfile SYS_sendfile64
> > >  #endif
> > >  
> > > +#ifdef SYS_timer_settime32
> > >  #ifndef SYS_timer_settime
> > >  #define SYS_timer_settime SYS_timer_settime32
> > >  #endif
> > > @@ -240,6 +241,7 @@ static inline long __alt_socketcall(int sys, int sock, int cp, long a, long b, l
> > >  #ifndef SYS_settimeofday
> > >  #define SYS_settimeofday SYS_settimeofday_time32
> > >  #endif
> > > +#endif
> > 
> > The existing expectation internally in musl is that archs that lack
> > legacy time32 syscalls have both the unadorned and _time64 macros
> > defined to the same value. The public headers don't have to be like
> > that but the internal ones do. See arch/x32/syscall_arch.h which does
> > it but in the opposite direction. This logic is all tested for x32 and
> > a big part of the point of that was preparing for rv32 and future
> > 32-bit archs.
> > 
> > I'm actually missing how this patch worked as-written, since for
> > example timer_settime.c unconditionally assumes SYS_timer_settime is
> > defined, and if it's defined to anything other than the same value as
> > the time64 version, it will use the value as a fallback.
> 
> src/internal/syscall.h has two groups of conditional definitions of unadorned
> syscall numbers, the first of which sets undefined syscalls to the _time32
> version, the second of which sets to the _time64 version.  So for instance I
> see no way for the #define SYS_clock_gettime SYS_clock_gettime64
> (https://git.musl-libc.org/cgit/musl/tree/src/internal/syscall.h#n250) to execute.

OK, I think I see what happened. Originally 4bbd7baea7 was done with
the intent that riscv32 and future 32-bit archs would not need to do
their own definitions of the unadorned syscall names in terms of the
time64 ones, and when committed it would have worked. However, later
5a105f19b5 and 2cae9f59da hid the old syscalls that were unable to be
used safely in applictions, but at risk of being used as such, behind
_time32 renamings, and broke the intended mechanism.

As such, your patch does get around the problem and make the right
definitions exposed, but I don't really like that it conditions them
all on a single #ifdef SYS_timer_settime32. If we want to keep the
property that rv32 and future archs aren't required to define the
unadorned names themselves (maybe they'd want to for the sake of
public headers anyway, but I'm not sure about this still; I think we
need it at least for futex), a solution I like somewhat better would
be along the lines of:

-#ifndef SYS_timer_settime
+#ifdef SYS_timer_settime32

for each of the affected syscalls. This would also have the nice
effect of producing an error if there already was a conflicting
definition where the unadorned and time32 names both exist but don't
match.

Rich