failed autopkgtests for evolver vs glibc 2.39 on amd64

reported at
https://gcc.gnu.org/pipermail/gcc-patches/2024-March/647635.html

On x86, this compiles into movdqa which segfaults on unaligned access.

This kind of failure has been seen when running against glibc 2.39,
which incidentally changed the printf implementation to move away from
alloca() for this data to instead append it at the end of an existing
"scratch buffer", with arbitrary alignment, whereas alloca() was
probably more likely to be naturally aligned.

Tested by adding the patch to the Ubuntu gcc-14 package in
https://launchpad.net/~schopin/+archive/ubuntu/libquadmath

Signed-off-by: Simon Chopin <email address hidden>
---
 libquadmath/printf/printf_fp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libquadmath/printf/printf_fp.c b/libquadmath/printf/printf_fp.c
index 8effcee88fa..d86aa650d38 100644
--- a/libquadmath/printf/printf_fp.c
+++ b/libquadmath/printf/printf_fp.c
@@ -363,7 +363,7 @@ __quadmath_printf_fp (struct __quadmath_printf_file *fp,

   /* Fetch the argument value. */
     {
- fpnum = **(const __float128 **) args[0];
+ memcpy(&fpnum, *(void* const *) args[0], sizeof(fpnum));

       /* Check for special values: not a number or infinity. */
       if (isnanq (fpnum))

The question is whether the caller misbehaves according to the ABI here? There's likely a known alignment present we could re-instantiate with a
__builtin_assume_aligned?

From what I can see, glibc uses there the same thing as libquadmath does, so why is it ok on the glibc side and not on the libquadmath side?

I mean
https://sourceware.org/git/?p=glibc.git;a=blob;f=stdio-common/printf_fp.c;h=e75706f089bba3baabbcfb6bcf41514bad0a9dcb;hb=HEAD#l222
and
https://sourceware.org/git/?p=glibc.git;a=blob;f=stdio-common/printf_fp.c;h=e75706f089bba3baabbcfb6bcf41514bad0a9dcb;hb=HEAD#l191

Is the stack properly aligned at this point?

This looks like a glibc bug to me.

Without a test case it is hard to tell.

Looks like the issue is that args_pa_user is not kept aligned. On the other hand, flt128_va already uses memcpy, so it does not expect the memory to be aligned.

Note also that in glibc, _Float128 support in printf code can only be used in limited circumstances: either on powerpc64le, as one of the multiple supported long double formats there, or through the sharing of the printf code with the implementation of strfromf128.

In particular, there are no glibc printf formats corresponding directly to _FloatN / _FloatNx types. There was support in principle at the WG14 meeting in Strasbourg in January for having some form of printf/scanf support for such types in C2y, but major work is still needed on the wording that was proposed in N3184.

I guess we should go with the above patch after fixing formatting, but it isn't enough,
printf_fphex.c has similar code.
Even in glibc which doesn't support printing _Float128 nor any other type which would require similar alignment, the hooks only register a function to fill in some mem and allows specification of size, but can't specify alignment.

Adding myself here as I need hex format for gfortran EX format specifiers.

Created attachment 57853
gcc14-pr114533.patch

Untested fix. Unfortunately, we don't have any testsuite for libquadmath, hope it will be tested during libgfortran testing.

while not a test case, that was seen when running autopkg tests of the evolver package against glibc 2.39 packages.

https://bugs.launchpad.net/ubuntu/+source/glibc/+bug/2052929

the failing evolver test is:

echo "g 5; v; r ; g 10; v;" | evolver-nox-q cube

The master branch has been updated by Jakub Jelinek <email address hidden>:

https://gcc.gnu.org/g:8455d6f6cd43b7b143ab9ee19437452fceba9cc9

commit r14-9769-g8455d6f6cd43b7b143ab9ee19437452fceba9cc9
Author: Jakub Jelinek <email address hidden>
Date: Wed Apr 3 10:02:35 2024 +0200

    libquadmath: Don't assume the storage for __float128 arguments is aligned [PR114533]

    With the register_printf_type/register_printf_modifier/register_printf_specifier
    APIs the C library is just told the size of the argument and is provided with
    a callback to fetch the argument from va_list using va_arg into C library provided
    memory. The C library isn't told what alignment requirement it has, but we were
    using direct load of a __float128 value from that memory which assumes
    __alignof (__float128) alignment.

    The following patch fixes that by using memcpy instead.

    I haven't been able to reproduce an actual crash, tried
     #include <quadmath.h>
     #include <stdlib.h>
     #include <stdio.h>

    int main ()
    {
      __float128 r;
      int prec = 20;
      int width = 46;
      char buf[128];

      r = 2.0q;
      r = sqrtq (r);
      int n = quadmath_snprintf (buf, sizeof buf, "%+-#*.20Qe", width, r);
      if ((size_t) n < sizeof buf)
        printf ("%s\n", buf);
        /* Prints: +1.41421356237309504880e+00 */
      quadmath_snprintf (buf, sizeof buf, "%Qa", r);
      if ((size_t) n < sizeof buf)
        printf ("%s\n", buf);
        /* Prints: 0x1.6a09e667f3bcc908b2fb1366ea96p+0 */
      n = quadmath_snprintf (NULL, 0, "%+-#46.*Qe", prec, r);
      if (n > -1)
        {
          char *str = malloc (n + 1);
          if (str)
            {
              quadmath_snprintf (str, n + 1, "%+-#46.*Qe", prec, r);
              printf ("%s\n", str);
              /* Prints: +1.41421356237309504880e+00 */
            }
          free (str);
        }
      printf ("%+-#*.20Qe\n", width, r);
      printf ("%Qa\n", r);
      printf ("%+-#46.*Qe\n", prec, r);
      printf ("%d %Qe %d %Qe %d %Qe\n", 1, r, 2, r, 3, r);
      return 0;
    }
    In any case, I think memcpy for loading from it is right.

    2024-04-03 Simon Chopin <email address hidden>
                Jakub Jelinek <email address hidden>

            PR libquadmath/114533
            * printf/printf_fp.c (__quadmath_printf_fp): Use memcpy to copy
            __float128 out of args.
            * printf/printf_fphex.c (__quadmath_printf_fphex): Likewise.

    Signed-off-by: Simon Chopin <email address hidden>

The releases/gcc-13 branch has been updated by Jakub Jelinek <email address hidden>:

https://gcc.gnu.org/g:cc39bd532d4de1ba0b2785246fb6fdd63ec2e92c

commit r13-8625-gcc39bd532d4de1ba0b2785246fb6fdd63ec2e92c
Author: Jakub Jelinek <email address hidden>
Date: Wed Apr 3 10:02:35 2024 +0200

    libquadmath: Don't assume the storage for __float128 arguments is aligned [PR114533]

    With the register_printf_type/register_printf_modifier/register_printf_specifier
    APIs the C library is just told the size of the argument and is provided with
    a callback to fetch the argument from va_list using va_arg into C library provided
    memory. The C library isn't told what alignment requirement it has, but we were
    using direct load of a __float128 value from that memory which assumes
    __alignof (__float128) alignment.

    The following patch fixes that by using memcpy instead.

    I haven't been able to reproduce an actual crash, tried
     #include <quadmath.h>
     #include <stdlib.h>
     #include <stdio.h>

    int main ()
    {
      __float128 r;
      int prec = 20;
      int width = 46;
      char buf[128];

      r = 2.0q;
      r = sqrtq (r);
      int n = quadmath_snprintf (buf, sizeof buf, "%+-#*.20Qe", width, r);
      if ((size_t) n < sizeof buf)
        printf ("%s\n", buf);
        /* Prints: +1.41421356237309504880e+00 */
      quadmath_snprintf (buf, sizeof buf, "%Qa", r);
      if ((size_t) n < sizeof buf)
        printf ("%s\n", buf);
        /* Prints: 0x1.6a09e667f3bcc908b2fb1366ea96p+0 */
      n = quadmath_snprintf (NULL, 0, "%+-#46.*Qe", prec, r);
      if (n > -1)
        {
          char *str = malloc (n + 1);
          if (str)
            {
              quadmath_snprintf (str, n + 1, "%+-#46.*Qe", prec, r);
              printf ("%s\n", str);
              /* Prints: +1.41421356237309504880e+00 */
            }
          free (str);
        }
      printf ("%+-#*.20Qe\n", width, r);
      printf ("%Qa\n", r);
      printf ("%+-#46.*Qe\n", prec, r);
      printf ("%d %Qe %d %Qe %d %Qe\n", 1, r, 2, r, 3, r);
      return 0;
    }
    In any case, I think memcpy for loading from it is right.

    2024-04-03 Simon Chopin <email address hidden>
                Jakub Jelinek <email address hidden>

            PR libquadmath/114533
            * printf/printf_fp.c (__quadmath_printf_fp): Use memcpy to copy
            __float128 out of args.
            * printf/printf_fphex.c (__quadmath_printf_fphex): Likewise.

    Signed-off-by: Simon Chopin <email address hidden>
    (cherry picked from commit 8455d6f6cd43b7b143ab9ee19437452fceba9cc9)

Fixed for 13.3 too.