Revert "linux: Implement mremap in C" (bug 31968)

This reverts commit 5b3e31e3124bf89710e5c25176c70fdf66c2a212.

The C implementation processes the optional argument based on
flags.  Linux 5.7 added MREMAP_DONTUNMAP, which needs the optional
argument just like MREMAP_FIXED.  Rather than creating a continuous
maintenance task, revert back to the assembler implementation.
There does not seem to be a need for the C implementation
(unlike what we saw with prctl on x86-64 x32):

<https://inbox.sourceware.org/libc-alpha/177225da-682e-4f57-9cde-5ff8f366266a@linaro.org/>

Tested on x86_64-linux-gnu.

---
 sysdeps/unix/sysv/linux/Makefile      |  1 -
 sysdeps/unix/sysv/linux/mremap.c      | 41 -----------------------------------
 sysdeps/unix/sysv/linux/syscalls.list |  1 +
 3 files changed, 1 insertion(+), 42 deletions(-)


base-commit: 9fc639f654dc004736836613be703e6bed0c36a8

On 09/07/24 12:00, Florian Weimer wrote:
> This reverts commit 5b3e31e3124bf89710e5c25176c70fdf66c2a212.
> 
> The C implementation processes the optional argument based on
> flags.  Linux 5.7 added MREMAP_DONTUNMAP, which needs the optional
> argument just like MREMAP_FIXED.  Rather than creating a continuous
> maintenance task, revert back to the assembler implementation.
> There does not seem to be a need for the C implementation
> (unlike what we saw with prctl on x86-64 x32):
> 
> <https://inbox.sourceware.org/libc-alpha/177225da-682e-4f57-9cde-5ff8f366266a@linaro.org/>
> 
> Tested on x86_64-linux-gnu.
> 

So we will need to continue use assembly implementations, with duplicates the
required code, adds extra complexity, and possible pass unitialized values to
kABI to avoid such issue? 

I really would like to get rid at all of the assembly wrappers so we can 
eventually could make LTO possible in some case. 

> ---
>  sysdeps/unix/sysv/linux/Makefile      |  1 -
>  sysdeps/unix/sysv/linux/mremap.c      | 41 -----------------------------------
>  sysdeps/unix/sysv/linux/syscalls.list |  1 +
>  3 files changed, 1 insertion(+), 42 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 097b5a26fc..e8dd4518d9 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -82,7 +82,6 @@ sysdep_routines += \
>    lxstat \
>    lxstat64 \
>    mlock2 \
> -  mremap \
>    open_by_handle_at \
>    personality \
>    pkey_get \
> diff --git a/sysdeps/unix/sysv/linux/mremap.c b/sysdeps/unix/sysv/linux/mremap.c
> deleted file mode 100644
> index 4f770799c4..0000000000
> --- a/sysdeps/unix/sysv/linux/mremap.c
> +++ /dev/null
> @@ -1,41 +0,0 @@
> -/* Remap a virtual memory address.  Linux specific syscall.
> -   Copyright (C) 2021-2024 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -
> -   The GNU C Library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later version.
> -
> -   The GNU C Library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with the GNU C Library; if not, see
> -   <https://www.gnu.org/licenses/>.  */
> -
> -#include <sys/mman.h>
> -#include <sysdep.h>
> -#include <stdarg.h>
> -#include <stddef.h>
> -
> -void *
> -__mremap (void *addr, size_t old_len, size_t new_len, int flags, ...)
> -{
> -  va_list va;
> -  void *new_addr = NULL;
> -
> -  if (flags & MREMAP_FIXED)
> -    {
> -      va_start (va, flags);
> -      new_addr = va_arg (va, void *);
> -      va_end (va);
> -    }
> -
> -  return (void *) INLINE_SYSCALL_CALL (mremap, addr, old_len, new_len, flags,
> -				       new_addr);
> -}
> -libc_hidden_def (__mremap)
> -weak_alias (__mremap, mremap)
> diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list
> index 9ac42c3436..2fff680964 100644
> --- a/sysdeps/unix/sysv/linux/syscalls.list
> +++ b/sysdeps/unix/sysv/linux/syscalls.list
> @@ -39,6 +39,7 @@ mlockall	-	mlockall	i:i	mlockall
>  mount		EXTRA	mount		i:sssUp	__mount	mount
>  mount_setattr	EXTRA	mount_setattr	i:isUpU	mount_setattr
>  move_mount	EXTRA	move_mount	i:isisU	move_mount
> +mremap		EXTRA	mremap		b:aUUip	__mremap	mremap
>  munlock		-	munlock		i:aU	munlock
>  munlockall	-	munlockall	i:	munlockall
>  nfsservctl	EXTRA	nfsservctl	i:ipp	__compat_nfsservctl	nfsservctl@GLIBC_2.0:GLIBC_2.28
> 
> base-commit: 9fc639f654dc004736836613be703e6bed0c36a8
>

* Adhemerval Zanella Netto:

> On 09/07/24 12:00, Florian Weimer wrote:
>> This reverts commit 5b3e31e3124bf89710e5c25176c70fdf66c2a212.
>> 
>> The C implementation processes the optional argument based on
>> flags.  Linux 5.7 added MREMAP_DONTUNMAP, which needs the optional
>> argument just like MREMAP_FIXED.  Rather than creating a continuous
>> maintenance task, revert back to the assembler implementation.
>> There does not seem to be a need for the C implementation
>> (unlike what we saw with prctl on x86-64 x32):
>> 
>> <https://inbox.sourceware.org/libc-alpha/177225da-682e-4f57-9cde-5ff8f366266a@linaro.org/>
>> 
>> Tested on x86_64-linux-gnu.
>> 
>
> So we will need to continue use assembly implementations, with
> duplicates the required code, adds extra complexity, and possible pass
> unitialized values to kABI to avoid such issue?

I don't see how it duplicates code more so than the C implementation.
It's just the standard system call wrapper.

> I really would like to get rid at all of the assembly wrappers so we can 
> eventually could make LTO possible in some case.

It would need a GCC extension because the calling convention is
non-standard.

We could make the new mremap argument mandatory, or provide different
non-variadic functions for different argument counts (but that would
duplicate code).

Thanks,
Florian

On 09/07/24 12:48, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 09/07/24 12:00, Florian Weimer wrote:
>>> This reverts commit 5b3e31e3124bf89710e5c25176c70fdf66c2a212.
>>>
>>> The C implementation processes the optional argument based on
>>> flags.  Linux 5.7 added MREMAP_DONTUNMAP, which needs the optional
>>> argument just like MREMAP_FIXED.  Rather than creating a continuous
>>> maintenance task, revert back to the assembler implementation.
>>> There does not seem to be a need for the C implementation
>>> (unlike what we saw with prctl on x86-64 x32):
>>>
>>> <https://inbox.sourceware.org/libc-alpha/177225da-682e-4f57-9cde-5ff8f366266a@linaro.org/>
>>>
>>> Tested on x86_64-linux-gnu.
>>>
>>
>> So we will need to continue use assembly implementations, with
>> duplicates the required code, adds extra complexity, and possible pass
>> unitialized values to kABI to avoid such issue?
> 
> I don't see how it duplicates code more so than the C implementation.
> It's just the standard system call wrapper.

The duplication come from the multiple PSEUDO/PSEUDO_NOERRO/etc macros that
each port need to provide so the make-syscalls.sh could use for the autogenerated
ones.  And we also need the macros to issues direct syscall in C code.

> 
>> I really would like to get rid at all of the assembly wrappers so we can 
>> eventually could make LTO possible in some case.
> 
> It would need a GCC extension because the calling convention is
> non-standard.

The mremap was already defined as by the headers as variadic, using the
assembly wrapper each architecture will need to make sure that variadic
maps correct to the expected kABI (which is usually true, but it is
an implicit constraint not clearly defined).  

I don't see this as a better alternative as to make it explicit in the 
C implementation, where at least it pass the arguments as cleared instead
of garbage.

> 
> We could make the new mremap argument mandatory, or provide different
> non-variadic functions for different argument counts (but that would
> duplicate code).
> 
> Thanks,
> Florian
>

* Adhemerval Zanella Netto:

> On 09/07/24 12:48, Florian Weimer wrote:
>> * Adhemerval Zanella Netto:
>> 
>>> On 09/07/24 12:00, Florian Weimer wrote:
>>>> This reverts commit 5b3e31e3124bf89710e5c25176c70fdf66c2a212.
>>>>
>>>> The C implementation processes the optional argument based on
>>>> flags.  Linux 5.7 added MREMAP_DONTUNMAP, which needs the optional
>>>> argument just like MREMAP_FIXED.  Rather than creating a continuous
>>>> maintenance task, revert back to the assembler implementation.
>>>> There does not seem to be a need for the C implementation
>>>> (unlike what we saw with prctl on x86-64 x32):
>>>>
>>>> <https://inbox.sourceware.org/libc-alpha/177225da-682e-4f57-9cde-5ff8f366266a@linaro.org/>
>>>>
>>>> Tested on x86_64-linux-gnu.
>>>>
>>>
>>> So we will need to continue use assembly implementations, with
>>> duplicates the required code, adds extra complexity, and possible pass
>>> unitialized values to kABI to avoid such issue?
>> 
>> I don't see how it duplicates code more so than the C implementation.
>> It's just the standard system call wrapper.
>
> The duplication come from the multiple PSEUDO/PSEUDO_NOERRO/etc macros
> that each port need to provide so the make-syscalls.sh could use for
> the autogenerated ones.  And we also need the macros to issues direct
> syscall in C code.

Ah, I see.  We can avoid that if we are willing to live with the ABI
mismatch between the C implementation and the installed header
declaration.  As I said, for LTO that would need a GCC extension.

It would also possible to generate both assembler implementations from a
common generator.
 
>>> I really would like to get rid at all of the assembly wrappers so we can 
>>> eventually could make LTO possible in some case.
>> 
>> It would need a GCC extension because the calling convention is
>> non-standard.
>
> The mremap was already defined as by the headers as variadic, using the
> assembly wrapper each architecture will need to make sure that variadic
> maps correct to the expected kABI (which is usually true, but it is
> an implicit constraint not clearly defined).

The initial declaration was not variadic.  It was changed in this commit
(from the history repository):

commit 43f03e3eadbf982b75d1e788ae66bd56e2ffdcb3
Author: Ulrich Drepper <drepper@redhat.com>
Date:   Fri Oct 14 13:39:09 2005 +0000

    Add ellipsis after last parameter of mremap and adjust leading comment.

I don't think we would make such changes today.

> I don't see this as a better alternative as to make it explicit in the 
> C implementation, where at least it pass the arguments as cleared instead
> of garbage.

We've seen problems when people call these variadic functions through
non-variadic prototypes.  Full compatibility requires writing the
implementation as a non-variadic function.  It does not have to be
assembler, but if it's in C, we rely on a compiler barrier to make this
well-defined.

We could write it as a variadic function and call va_arg
unconditionally.  I don't see the point because it still requires a GCC
extension with LTO, and GCC code generation for this construct isn't
great for most targets.  And that's with GCC's optimization not to store
incoming registers on the stack that are never read as variadic
arguments (something that Clang cannot do).

Thanks,
Florian

On 09/07/24 13:22, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 09/07/24 12:48, Florian Weimer wrote:
>>> * Adhemerval Zanella Netto:
>>>
>>>> On 09/07/24 12:00, Florian Weimer wrote:
>>>>> This reverts commit 5b3e31e3124bf89710e5c25176c70fdf66c2a212.
>>>>>
>>>>> The C implementation processes the optional argument based on
>>>>> flags.  Linux 5.7 added MREMAP_DONTUNMAP, which needs the optional
>>>>> argument just like MREMAP_FIXED.  Rather than creating a continuous
>>>>> maintenance task, revert back to the assembler implementation.
>>>>> There does not seem to be a need for the C implementation
>>>>> (unlike what we saw with prctl on x86-64 x32):
>>>>>
>>>>> <https://inbox.sourceware.org/libc-alpha/177225da-682e-4f57-9cde-5ff8f366266a@linaro.org/>
>>>>>
>>>>> Tested on x86_64-linux-gnu.
>>>>>
>>>>
>>>> So we will need to continue use assembly implementations, with
>>>> duplicates the required code, adds extra complexity, and possible pass
>>>> unitialized values to kABI to avoid such issue?
>>>
>>> I don't see how it duplicates code more so than the C implementation.
>>> It's just the standard system call wrapper.
>>
>> The duplication come from the multiple PSEUDO/PSEUDO_NOERRO/etc macros
>> that each port need to provide so the make-syscalls.sh could use for
>> the autogenerated ones.  And we also need the macros to issues direct
>> syscall in C code.
> 
> Ah, I see.  We can avoid that if we are willing to live with the ABI
> mismatch between the C implementation and the installed header
> declaration.  As I said, for LTO that would need a GCC extension.
> 

Why exactly we will need a extension? Can't LTO handle variadic arguments
or does it generated subpar code?

> It would also possible to generate both assembler implementations from a
> common generator.>  
>>>> I really would like to get rid at all of the assembly wrappers so we can 
>>>> eventually could make LTO possible in some case.
>>>
>>> It would need a GCC extension because the calling convention is
>>> non-standard.
>>
>> The mremap was already defined as by the headers as variadic, using the
>> assembly wrapper each architecture will need to make sure that variadic
>> maps correct to the expected kABI (which is usually true, but it is
>> an implicit constraint not clearly defined).
> 
> The initial declaration was not variadic.  It was changed in this commit
> (from the history repository):
> 
> commit 43f03e3eadbf982b75d1e788ae66bd56e2ffdcb3
> Author: Ulrich Drepper <drepper@redhat.com>
> Date:   Fri Oct 14 13:39:09 2005 +0000
> 
>     Add ellipsis after last parameter of mremap and adjust leading comment.
> 
> I don't think we would make such changes today.

Yeah, and that was unfortunate.  From commit message it seams that problem
was that the last argument was not factored initially in prototype and at
least we know today that variadic function prototypes are tricky, and tend 
to generate sub-optimal code (from previous iteration from this thread).

> 
>> I don't see this as a better alternative as to make it explicit in the 
>> C implementation, where at least it pass the arguments as cleared instead
>> of garbage.
> 
> We've seen problems when people call these variadic functions through
> non-variadic prototypes.  Full compatibility requires writing the
> implementation as a non-variadic function.  It does not have to be
> assembler, but if it's in C, we rely on a compiler barrier to make this
> well-defined.

But this is essentially an error with the user code that is using a bogus
definition from who knows why, isn't it? Not sure why we should support
it.

Recently with the clang fortify wrapper works, we found out that some code
redefine the 'const' keyword so the program can do some data initialization
at statup, and the later code would see the type as 'const'.  It works with 
gcc fortify wrapper but it breaks with clang one that require the const in 
first argument. Should we keep supporting it for the sake of compatibility? 
I don't think so.

> 
> We could write it as a variadic function and call va_arg
> unconditionally.  I don't see the point because it still requires a GCC
> extension with LTO, and GCC code generation for this construct isn't
> great for most targets.  And that's with GCC's optimization not to store
> incoming registers on the stack that are never read as variadic
> arguments (something that Clang cannot do).

I don't think we should really focus on code generation to squeeze some
instruction for a syscall that would most likely take some thousand of
cycles and multiple memory operations.

What I would really like is to avoid assembly routines where there is no
need to do, like other projects such Linux kernel are also moving to.

* Adhemerval Zanella Netto:

>> Ah, I see.  We can avoid that if we are willing to live with the ABI
>> mismatch between the C implementation and the installed header
>> declaration.  As I said, for LTO that would need a GCC extension.
>
> Why exactly we will need a extension? Can't LTO handle variadic
> arguments or does it generated subpar code?

We don't want to perform conditional extraction of the extra argument,
otherwise we need to update glibc every time the kernel adds a new flag.
(To avoid actual undefined behavior in case of kernel flag additions,
we'd need to reject unknown flags with EINVAL.)

Unconditionally extracting a variadic argument that doesn't exist
requires a GCC extension with LTO.

At this point, we might as well just write a non-variadic function,
which generates way better code on some architectures.

>>> I don't see this as a better alternative as to make it explicit in the 
>>> C implementation, where at least it pass the arguments as cleared instead
>>> of garbage.
>> 
>> We've seen problems when people call these variadic functions through
>> non-variadic prototypes.  Full compatibility requires writing the
>> implementation as a non-variadic function.  It does not have to be
>> assembler, but if it's in C, we rely on a compiler barrier to make this
>> well-defined.
>
> But this is essentially an error with the user code that is using a bogus
> definition from who knows why, isn't it? Not sure why we should support
> it.

Is it really bogus if it's from the manual pages?  Or if it matches used
to be in a glibc header?  Although perhaps that is less of an issue
today because many manual pages no longer use C syntax to describe
function declarations.

> What I would really like is to avoid assembly routines where there is no
> need to do, like other projects such Linux kernel are also moving to.

Understood.  If we don't want a benign ABI mismatch at the C level, we
have to write the function in assembler.

Thanks,
Florian

On Tue, Jul 9, 2024, 11:01 PM Florian Weimer <fweimer@redhat.com> wrote:

> This reverts commit 5b3e31e3124bf89710e5c25176c70fdf66c2a212.
>
> The C implementation processes the optional argument based on
> flags.  Linux 5.7 added MREMAP_DONTUNMAP, which needs the optional
>

Missing a test for MREMAP_DONTUNMAP.

argument just like MREMAP_FIXED.  Rather than creating a continuous
> maintenance task, revert back to the assembler implementation.
> There does not seem to be a need for the C implementation
> (unlike what we saw with prctl on x86-64 x32):
>
> <
> https://inbox.sourceware.org/libc-alpha/177225da-682e-4f57-9cde-5ff8f366266a@linaro.org/
> >
>
> Tested on x86_64-linux-gnu.
>
> ---
>  sysdeps/unix/sysv/linux/Makefile      |  1 -
>  sysdeps/unix/sysv/linux/mremap.c      | 41
> -----------------------------------
>  sysdeps/unix/sysv/linux/syscalls.list |  1 +
>  3 files changed, 1 insertion(+), 42 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/Makefile
> b/sysdeps/unix/sysv/linux/Makefile
> index 097b5a26fc..e8dd4518d9 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -82,7 +82,6 @@ sysdep_routines += \
>    lxstat \
>    lxstat64 \
>    mlock2 \
> -  mremap \
>    open_by_handle_at \
>    personality \
>    pkey_get \
> diff --git a/sysdeps/unix/sysv/linux/mremap.c
> b/sysdeps/unix/sysv/linux/mremap.c
> deleted file mode 100644
> index 4f770799c4..0000000000
> --- a/sysdeps/unix/sysv/linux/mremap.c
> +++ /dev/null
> @@ -1,41 +0,0 @@
> -/* Remap a virtual memory address.  Linux specific syscall.
> -   Copyright (C) 2021-2024 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -
> -   The GNU C Library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later version.
> -
> -   The GNU C Library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with the GNU C Library; if not, see
> -   <https://www.gnu.org/licenses/>.  */
> -
> -#include <sys/mman.h>
> -#include <sysdep.h>
> -#include <stdarg.h>
> -#include <stddef.h>
> -
> -void *
> -__mremap (void *addr, size_t old_len, size_t new_len, int flags, ...)
> -{
> -  va_list va;
> -  void *new_addr = NULL;
> -
> -  if (flags & MREMAP_FIXED)
> -    {
> -      va_start (va, flags);
> -      new_addr = va_arg (va, void *);
> -      va_end (va);
> -    }
> -
> -  return (void *) INLINE_SYSCALL_CALL (mremap, addr, old_len, new_len,
> flags,
> -                                      new_addr);
> -}
> -libc_hidden_def (__mremap)
> -weak_alias (__mremap, mremap)
> diff --git a/sysdeps/unix/sysv/linux/syscalls.list
> b/sysdeps/unix/sysv/linux/syscalls.list
> index 9ac42c3436..2fff680964 100644
> --- a/sysdeps/unix/sysv/linux/syscalls.list
> +++ b/sysdeps/unix/sysv/linux/syscalls.list
> @@ -39,6 +39,7 @@ mlockall      -       mlockall        i:i     mlockall
>  mount          EXTRA   mount           i:sssUp __mount mount
>  mount_setattr  EXTRA   mount_setattr   i:isUpU mount_setattr
>  move_mount     EXTRA   move_mount      i:isisU move_mount
> +mremap         EXTRA   mremap          b:aUUip __mremap        mremap
>  munlock                -       munlock         i:aU    munlock
>  munlockall     -       munlockall      i:      munlockall
>  nfsservctl     EXTRA   nfsservctl      i:ipp   __compat_nfsservctl
>  nfsservctl@GLIBC_2.0:GLIBC_2.28
>
> base-commit: 9fc639f654dc004736836613be703e6bed0c36a8
>
>
>

* H. J. Lu:

> On Tue, Jul 9, 2024, 11:01 PM Florian Weimer <fweimer@redhat.com> wrote:
>
>  This reverts commit 5b3e31e3124bf89710e5c25176c70fdf66c2a212.
>
>  The C implementation processes the optional argument based on
>  flags.  Linux 5.7 added MREMAP_DONTUNMAP, which needs the optional
>
> Missing a test for MREMAP_DONTUNMAP.

A colleague is writing one.  I expect it's going to be posted to
libc-alpha soon.

Thanks,
Florian

On Wed, Jul 10, 2024, 2:57 PM Florian Weimer <fweimer@redhat.com> wrote:

> * H. J. Lu:
>
> > On Tue, Jul 9, 2024, 11:01 PM Florian Weimer <fweimer@redhat.com> wrote:
> >
> >  This reverts commit 5b3e31e3124bf89710e5c25176c70fdf66c2a212.
> >
> >  The C implementation processes the optional argument based on
> >  flags.  Linux 5.7 added MREMAP_DONTUNMAP, which needs the optional
> >
> > Missing a test for MREMAP_DONTUNMAP.
>
> A colleague is writing one.  I expect it's going to be posted to
> libc-alpha soon.
>

Shouldn't the test be the part of the patch set?


> Thanks,
> Florian
>
>
>

* H. J. Lu:

> On Wed, Jul 10, 2024, 2:57 PM Florian Weimer <fweimer@redhat.com> wrote:
>
>> * H. J. Lu:
>>
>> > On Tue, Jul 9, 2024, 11:01 PM Florian Weimer <fweimer@redhat.com> wrote:
>> >
>> >  This reverts commit 5b3e31e3124bf89710e5c25176c70fdf66c2a212.
>> >
>> >  The C implementation processes the optional argument based on
>> >  flags.  Linux 5.7 added MREMAP_DONTUNMAP, which needs the optional
>> >
>> > Missing a test for MREMAP_DONTUNMAP.
>>
>> A colleague is writing one.  I expect it's going to be posted to
>> libc-alpha soon.
>
> Shouldn't the test be the part of the patch set?

I don't know if the test will be ready for in time for 2.40.  In the
end, it doesn't really matter because we have to backport the fix to
2.35 and later anyway.

Thanks,
Florian

On 09/07/24 14:43, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>>> Ah, I see.  We can avoid that if we are willing to live with the ABI
>>> mismatch between the C implementation and the installed header
>>> declaration.  As I said, for LTO that would need a GCC extension.
>>
>> Why exactly we will need a extension? Can't LTO handle variadic
>> arguments or does it generated subpar code?
> 
> We don't want to perform conditional extraction of the extra argument,
> otherwise we need to update glibc every time the kernel adds a new flag.
> (To avoid actual undefined behavior in case of kernel flag additions,
> we'd need to reject unknown flags with EINVAL.)
> 
> Unconditionally extracting a variadic argument that doesn't exist
> requires a GCC extension with LTO.
> 
> At this point, we might as well just write a non-variadic function,
> which generates way better code on some architectures.
I don't know if we should really bound to this constraint, it means
that we won't be able to write a wrapper in standard C and we will need
to keep the assembly wrapper support indefinitely.  Also, fo mremap
a new flag happens once in a years, so I am not sure if this is really
an issue.

And we don't this for open/openat/mq_open, nor I think we should. We 
already get the lesson to not use variadic signatures for syscall wrapper,
but and I also don't think falling back to the old mechanism is really
an improvement.

> 
>>>> I don't see this as a better alternative as to make it explicit in the 
>>>> C implementation, where at least it pass the arguments as cleared instead
>>>> of garbage.
>>>
>>> We've seen problems when people call these variadic functions through
>>> non-variadic prototypes.  Full compatibility requires writing the
>>> implementation as a non-variadic function.  It does not have to be
>>> assembler, but if it's in C, we rely on a compiler barrier to make this
>>> well-defined.
>>
>> But this is essentially an error with the user code that is using a bogus
>> definition from who knows why, isn't it? Not sure why we should support
>> it.
> 
> Is it really bogus if it's from the manual pages?  Or if it matches used
> to be in a glibc header?  Although perhaps that is less of an issue
> today because many manual pages no longer use C syntax to describe
> function declarations.

IMHO the manual is clearly wrong to not state the current prototype in
from the header (which should be canonical way), specially because on 
some ABI mixing variadic and non-variadic is really troublesome.

But from where these programs are getting the non-variadic prototypes?
Are they re-defining the symbol somehow?

> 
>> What I would really like is to avoid assembly routines where there is no
>> need to do, like other projects such Linux kernel are also moving to.
> 
> Understood.  If we don't want a benign ABI mismatch at the C level, we
> have to write the function in assembler.

We have to write the function in assembly only to satisfy the constraint
to always va_args the argument, where it will violate the standard anyway.
It is a way to do it, but I really think we should avoid it.  OK, this 
incur in a bit more maintainability to keep in sync with new kernel 
additions.

On Wed, Jul 10, 2024, 9:51 PM Adhemerval Zanella Netto <
adhemerval.zanella@linaro.org> wrote:

>
>
> On 09/07/24 14:43, Florian Weimer wrote:
> > * Adhemerval Zanella Netto:
> >
> >>> Ah, I see.  We can avoid that if we are willing to live with the ABI
> >>> mismatch between the C implementation and the installed header
> >>> declaration.  As I said, for LTO that would need a GCC extension.
> >>
> >> Why exactly we will need a extension? Can't LTO handle variadic
> >> arguments or does it generated subpar code?
> >
> > We don't want to perform conditional extraction of the extra argument,
> > otherwise we need to update glibc every time the kernel adds a new flag.
> > (To avoid actual undefined behavior in case of kernel flag additions,
> > we'd need to reject unknown flags with EINVAL.)
> >
> > Unconditionally extracting a variadic argument that doesn't exist
> > requires a GCC extension with LTO.
> >
> > At this point, we might as well just write a non-variadic function,
> > which generates way better code on some architectures.
> I don't know if we should really bound to this constraint, it means
> that we won't be able to write a wrapper in standard C and we will need
> to keep the assembly wrapper support indefinitely.  Also, fo mremap
> a new flag happens once in a years, so I am not sure if this is really
> an issue.
>
> And we don't this for open/openat/mq_open, nor I think we should. We
> already get the lesson to not use variadic signatures for syscall wrapper,
> but and I also don't think falling back to the old mechanism is really
> an improvement.
>
> >
> >>>> I don't see this as a better alternative as to make it explicit in
> the
> >>>> C implementation, where at least it pass the arguments as cleared
> instead
> >>>> of garbage.
> >>>
> >>> We've seen problems when people call these variadic functions through
> >>> non-variadic prototypes.  Full compatibility requires writing the
> >>> implementation as a non-variadic function.  It does not have to be
> >>> assembler, but if it's in C, we rely on a compiler barrier to make this
> >>> well-defined.
> >>
> >> But this is essentially an error with the user code that is using a
> bogus
> >> definition from who knows why, isn't it? Not sure why we should support
> >> it.
> >
> > Is it really bogus if it's from the manual pages?  Or if it matches used
> > to be in a glibc header?  Although perhaps that is less of an issue
> > today because many manual pages no longer use C syntax to describe
> > function declarations.
>
> IMHO the manual is clearly wrong to not state the current prototype in
> from the header (which should be canonical way), specially because on
> some ABI mixing variadic and non-variadic is really troublesome.
>
> But from where these programs are getting the non-variadic prototypes?
> Are they re-defining the symbol somehow?
>
> >
> >> What I would really like is to avoid assembly routines where there is no
> >> need to do, like other projects such Linux kernel are also moving to.
> >
> > Understood.  If we don't want a benign ABI mismatch at the C level, we
> > have to write the function in assembler.
>
> We have to write the function in assembly only to satisfy the constraint
> to always va_args the argument, where it will violate the standard anyway.
> It is a way to do it, but I really think we should avoid it.  OK, this
> incur in a bit more maintainability to keep in sync with new kernel
> additions.
>

I'd like to see testcases first.  They may have
impacts on how the fix will look like.  Testcases should cover all known
mremap constants.
The function prototype should be consistent
with the implementation.  Can we keep the
C implementation and add a compile time
check for unknown constants?



>

On Thu, Jul 11, 2024, 10:23 AM H.J. Lu <hjl.tools@gmail.com> wrote:

> On Wed, Jul 10, 2024, 9:51 PM Adhemerval Zanella Netto <
> adhemerval.zanella@linaro.org> wrote:
>
>>
>>
>> On 09/07/24 14:43, Florian Weimer wrote:
>> > * Adhemerval Zanella Netto:
>> >
>> >>> Ah, I see.  We can avoid that if we are willing to live with the ABI
>> >>> mismatch between the C implementation and the installed header
>> >>> declaration.  As I said, for LTO that would need a GCC extension.
>> >>
>> >> Why exactly we will need a extension? Can't LTO handle variadic
>> >> arguments or does it generated subpar code?
>> >
>> > We don't want to perform conditional extraction of the extra argument,
>> > otherwise we need to update glibc every time the kernel adds a new flag.
>> > (To avoid actual undefined behavior in case of kernel flag additions,
>> > we'd need to reject unknown flags with EINVAL.)
>> >
>> > Unconditionally extracting a variadic argument that doesn't exist
>> > requires a GCC extension with LTO.
>> >
>> > At this point, we might as well just write a non-variadic function,
>> > which generates way better code on some architectures.
>> I don't know if we should really bound to this constraint, it means
>> that we won't be able to write a wrapper in standard C and we will need
>> to keep the assembly wrapper support indefinitely.  Also, fo mremap
>> a new flag happens once in a years, so I am not sure if this is really
>> an issue.
>>
>> And we don't this for open/openat/mq_open, nor I think we should. We
>> already get the lesson to not use variadic signatures for syscall wrapper,
>> but and I also don't think falling back to the old mechanism is really
>> an improvement.
>>
>> >
>> >>>> I don't see this as a better alternative as to make it explicit in
>> the
>> >>>> C implementation, where at least it pass the arguments as cleared
>> instead
>> >>>> of garbage.
>> >>>
>> >>> We've seen problems when people call these variadic functions through
>> >>> non-variadic prototypes.  Full compatibility requires writing the
>> >>> implementation as a non-variadic function.  It does not have to be
>> >>> assembler, but if it's in C, we rely on a compiler barrier to make
>> this
>> >>> well-defined.
>> >>
>> >> But this is essentially an error with the user code that is using a
>> bogus
>> >> definition from who knows why, isn't it? Not sure why we should support
>> >> it.
>> >
>> > Is it really bogus if it's from the manual pages?  Or if it matches used
>> > to be in a glibc header?  Although perhaps that is less of an issue
>> > today because many manual pages no longer use C syntax to describe
>> > function declarations.
>>
>> IMHO the manual is clearly wrong to not state the current prototype in
>> from the header (which should be canonical way), specially because on
>> some ABI mixing variadic and non-variadic is really troublesome.
>>
>> But from where these programs are getting the non-variadic prototypes?
>> Are they re-defining the symbol somehow?
>>
>> >
>> >> What I would really like is to avoid assembly routines where there is
>> no
>> >> need to do, like other projects such Linux kernel are also moving to.
>> >
>> > Understood.  If we don't want a benign ABI mismatch at the C level, we
>> > have to write the function in assembler.
>>
>> We have to write the function in assembly only to satisfy the constraint
>> to always va_args the argument, where it will violate the standard anyway.
>> It is a way to do it, but I really think we should avoid it.  OK, this
>> incur in a bit more maintainability to keep in sync with new kernel
>> additions.
>>
>
> I'd like to see testcases first.  They may have
> impacts on how the fix will look like.  Testcases should cover all known
> mremap constants.
> The function prototype should be consistent
> with the implementation.  Can we keep the
> C implementation and add a compile time
> check for unknown constants?
>

I uploaded a different patch at

https://sourceware.org/bugzilla/show_bug.cgi?id=31968#c3


>
>
>>
>>

On Thu, Jul 11, 2024, 11:26 AM H.J. Lu <hjl.tools@gmail.com> wrote:

>
> On Thu, Jul 11, 2024, 10:23 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
>> On Wed, Jul 10, 2024, 9:51 PM Adhemerval Zanella Netto <
>> adhemerval.zanella@linaro.org> wrote:
>>
>>>
>>>
>>> On 09/07/24 14:43, Florian Weimer wrote:
>>> > * Adhemerval Zanella Netto:
>>> >
>>> >>> Ah, I see.  We can avoid that if we are willing to live with the ABI
>>> >>> mismatch between the C implementation and the installed header
>>> >>> declaration.  As I said, for LTO that would need a GCC extension.
>>> >>
>>> >> Why exactly we will need a extension? Can't LTO handle variadic
>>> >> arguments or does it generated subpar code?
>>> >
>>> > We don't want to perform conditional extraction of the extra argument,
>>> > otherwise we need to update glibc every time the kernel adds a new
>>> flag.
>>> > (To avoid actual undefined behavior in case of kernel flag additions,
>>> > we'd need to reject unknown flags with EINVAL.)
>>> >
>>> > Unconditionally extracting a variadic argument that doesn't exist
>>> > requires a GCC extension with LTO.
>>> >
>>> > At this point, we might as well just write a non-variadic function,
>>> > which generates way better code on some architectures.
>>> I don't know if we should really bound to this constraint, it means
>>> that we won't be able to write a wrapper in standard C and we will need
>>> to keep the assembly wrapper support indefinitely.  Also, fo mremap
>>> a new flag happens once in a years, so I am not sure if this is really
>>> an issue.
>>>
>>> And we don't this for open/openat/mq_open, nor I think we should. We
>>> already get the lesson to not use variadic signatures for syscall
>>> wrapper,
>>> but and I also don't think falling back to the old mechanism is really
>>> an improvement.
>>>
>>> >
>>> >>>> I don't see this as a better alternative as to make it explicit in
>>> the
>>> >>>> C implementation, where at least it pass the arguments as cleared
>>> instead
>>> >>>> of garbage.
>>> >>>
>>> >>> We've seen problems when people call these variadic functions through
>>> >>> non-variadic prototypes.  Full compatibility requires writing the
>>> >>> implementation as a non-variadic function.  It does not have to be
>>> >>> assembler, but if it's in C, we rely on a compiler barrier to make
>>> this
>>> >>> well-defined.
>>> >>
>>> >> But this is essentially an error with the user code that is using a
>>> bogus
>>> >> definition from who knows why, isn't it? Not sure why we should
>>> support
>>> >> it.
>>> >
>>> > Is it really bogus if it's from the manual pages?  Or if it matches
>>> used
>>> > to be in a glibc header?  Although perhaps that is less of an issue
>>> > today because many manual pages no longer use C syntax to describe
>>> > function declarations.
>>>
>>> IMHO the manual is clearly wrong to not state the current prototype in
>>> from the header (which should be canonical way), specially because on
>>> some ABI mixing variadic and non-variadic is really troublesome.
>>>
>>> But from where these programs are getting the non-variadic prototypes?
>>> Are they re-defining the symbol somehow?
>>>
>>> >
>>> >> What I would really like is to avoid assembly routines where there is
>>> no
>>> >> need to do, like other projects such Linux kernel are also moving to.
>>> >
>>> > Understood.  If we don't want a benign ABI mismatch at the C level, we
>>> > have to write the function in assembler.
>>>
>>> We have to write the function in assembly only to satisfy the constraint
>>> to always va_args the argument, where it will violate the standard
>>> anyway.
>>> It is a way to do it, but I really think we should avoid it.  OK, this
>>> incur in a bit more maintainability to keep in sync with new kernel
>>> additions.
>>>
>>
>> I'd like to see testcases first.  They may have
>> impacts on how the fix will look like.  Testcases should cover all known
>> mremap constants.
>> The function prototype should be consistent
>> with the implementation.  Can we keep the
>> C implementation and add a compile time
>> check for unknown constants?
>>
>
> I uploaded a different patch at
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=31968#c3
>

I took a look at kernel selftest.   Without
MREMAP_FIXED, the additional argument
is NULL which is compatible with assembly
implementation.  The new flag bit itself
dones't need the new argument.
A testcase needs to justify the revert.


>
>
>>
>>
>>>
>>>

> >
> > I'd like to see testcases first.  They may have
> > impacts on how the fix will look like.  Testcases should cover all known
> > mremap constants.
> > The function prototype should be consistent
> > with the implementation.  Can we keep the
> > C implementation and add a compile time
> > check for unknown constants?
> >
> 
> I uploaded a different patch at
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=31968#c3
> 

Practical question, since 2.40 is coming closer...  *If* this fix is still 
supposed to go in (and especially if not all tests are ready in time?)- 

How about using the revert to old, known code (i.e. the assembler implementation)
first for the release and updating it afterwards to the new C implementation
(which can be backported, I guess)?

That would remove some time pressure.

> 
> >
> >
> >>
> >>
>

On Sun, Jul 14, 2024, 10:05 PM Andreas K. Huettel <dilfridge@gentoo.org>
wrote:

> > >
> > > I'd like to see testcases first.  They may have
> > > impacts on how the fix will look like.  Testcases should cover all
> known
> > > mremap constants.
> > > The function prototype should be consistent
> > > with the implementation.  Can we keep the
> > > C implementation and add a compile time
> > > check for unknown constants?
> > >
> >
> > I uploaded a different patch at
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=31968#c3
> >
>
> Practical question, since 2.40 is coming closer...  *If* this fix is still
> supposed to go in (and especially if not all tests are ready in time?)-
>
> How about using the revert to old, known code (i.e. the assembler
> implementation)
> first for the release and updating it afterwards to the new C
> implementation
> (which can be backported, I guess)?
>

This different patch set includes a test.


> That would remove some time pressure.
>
> >
> > >
> > >
> > >>
> > >>
> >
>
>
> --
> Andreas K. Hüttel
> dilfridge@gentoo.org
> Gentoo Linux developer
> (council, comrel, toolchain, base-system, perl, libreoffice)
> https://wiki.gentoo.org/wiki/User:Dilfridge
>

* Andreas K. Huettel:

>> >
>> > I'd like to see testcases first.  They may have
>> > impacts on how the fix will look like.  Testcases should cover all known
>> > mremap constants.
>> > The function prototype should be consistent
>> > with the implementation.  Can we keep the
>> > C implementation and add a compile time
>> > check for unknown constants?
>> >
>> 
>> I uploaded a different patch at
>> 
>> https://sourceware.org/bugzilla/show_bug.cgi?id=31968#c3
>> 
>
> Practical question, since 2.40 is coming closer...  *If* this fix is still 
> supposed to go in (and especially if not all tests are ready in time?)- 

It's not a regression, so I don't think this should block the 2.40
release.  I thought the revert would be uncontroversial, but that does
not seem to be the case.

Thanks,
Florian

Am Montag, 15. Juli 2024, 13:19:42 CEST schrieb Florian Weimer:
> * Andreas K. Huettel:
> 
> >> >
> >> > I'd like to see testcases first.  They may have
> >> > impacts on how the fix will look like.  Testcases should cover all known
> >> > mremap constants.
> >> > The function prototype should be consistent
> >> > with the implementation.  Can we keep the
> >> > C implementation and add a compile time
> >> > check for unknown constants?
> >> >
> >> 
> >> I uploaded a different patch at
> >> 
> >> https://sourceware.org/bugzilla/show_bug.cgi?id=31968#c3
> >> 
> >
> > Practical question, since 2.40 is coming closer...  *If* this fix is still 
> > supposed to go in (and especially if not all tests are ready in time?)- 
> 
> It's not a regression, so I don't think this should block the 2.40
> release.  I thought the revert would be uncontroversial, but that does
> not seem to be the case.

Agreed. Let's postpone all variants until afterwards.

> 
> Thanks,
> Florian
> 
>