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

Submitted by Stefan O'Rear on Sept. 3, 2020, 11:22 a.m.

Details

Message ID 20200903112309.102601-3-sorear@fastmail.com
State New
Series "riscv32 support"
Headers show

Commit Message

Stefan O'Rear Sept. 3, 2020, 11:22 a.m.
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(+)

Patch hide | download patch | download mbox

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
 
 /* Ensure that the plain syscall names are defined even for "time64-only"
  * archs. These facilitate callers passing null time arguments, and make

Comments

Rich Felker Sept. 3, 2020, 3:56 p.m.
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.

Rich
Stefan O'Rear Sept. 3, 2020, 7:36 p.m.
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.

-s
Rich Felker Sept. 3, 2020, 9:17 p.m.
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