[PATCH] Account for octal marker in %#o format

[PATCH] Account for octal marker in %#o format

[Andreas Schwab]

---
 stdio-common/Makefile               |  1 +
 stdio-common/tst-printf-oct.c       | 50 +++++++++++++++++++++++++++++
 stdio-common/vfprintf-process-arg.c |  6 ++++
 3 files changed, 57 insertions(+)
 create mode 100644 stdio-common/tst-printf-oct.c

diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 8150453944..c787450e5c 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -207,6 +207,7 @@ tests := \
   tst-printf-bz25691 \
   tst-printf-fp-free \
   tst-printf-fp-leak \
+  tst-printf-oct \
   tst-printf-round \
   tst-printfsz \
   tst-put-error \
diff --git a/stdio-common/tst-printf-oct.c b/stdio-common/tst-printf-oct.c
new file mode 100644
index 0000000000..d1c88d5641
--- /dev/null
+++ b/stdio-common/tst-printf-oct.c
@@ -0,0 +1,50 @@
+/* Test printf width padding with octal format
+   Copyright (C) 2023 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 <locale.h>
+#include <stdio.h>
+#include <support/check.h>
+#include <support/support.h>
+
+static int
+do_test (void)
+{
+  char buf[80];
+
+  sprintf (buf, "%-7o", 123);
+  TEST_COMPARE_STRING (buf, "173    ");
+
+  sprintf (buf, "%#-7o", 123);
+  TEST_COMPARE_STRING (buf, "0173   ");
+
+  sprintf (buf, "%7o", 123);
+  TEST_COMPARE_STRING (buf, "    173");
+
+  sprintf (buf, "%#7o", 123);
+  TEST_COMPARE_STRING (buf, "   0173");
+
+  sprintf (buf, "%07o", 123);
+  TEST_COMPARE_STRING (buf, "0000173");
+
+  sprintf (buf, "%#07o", 123);
+  TEST_COMPARE_STRING (buf, "0000173");
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/stdio-common/vfprintf-process-arg.c b/stdio-common/vfprintf-process-arg.c
index cd3eaf5c0c..279a0efa9d 100644
--- a/stdio-common/vfprintf-process-arg.c
+++ b/stdio-common/vfprintf-process-arg.c
@@ -196,6 +196,9 @@ LABEL (unsigned_number):      /* Unsigned number of base BASE.  */
         /* Account for 0X, 0x, 0B or 0b hex or binary marker.  */
         width -= 2;
 
+      if (octal_marker)
+        --width;
+
       if (is_negative || showsign || space)
         --width;
 
@@ -257,6 +260,9 @@ LABEL (unsigned_number):      /* Unsigned number of base BASE.  */
           width -= 2;
         }
 
+      if (octal_marker)
+	--width;
+
       width -= number_length + prec;
 
       Xprintf_buffer_pad (buf, L_('0'), prec);
-- 
2.39.1


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Account for octal marker in %#o format

[Florian Weimer]

* Andreas Schwab via Libc-alpha:

> @@ -257,6 +260,9 @@ LABEL (unsigned_number):      /* Unsigned number of base BASE.  */
>            width -= 2;
>          }
>  
> +      if (octal_marker)
> +	--width;
> +
>        width -= number_length + prec;

This doesn't apply because master still has

      width -= workend - string + prec;

Apart from that, the change looks okay.

Carlos, this would be a regression, so I think we should fix it before
the release.

Thanks,
Florian

Re: [PATCH] Account for octal marker in %#o format

[Carlos O'Donell]

On 1/30/23 06:11, Florian Weimer wrote:
> * Andreas Schwab via Libc-alpha:
> 
>> @@ -257,6 +260,9 @@ LABEL (unsigned_number):      /* Unsigned number of base BASE.  */
>>            width -= 2;
>>          }
>>  
>> +      if (octal_marker)
>> +	--width;
>> +
>>        width -= number_length + prec;
> 
> This doesn't apply because master still has
> 
>       width -= workend - string + prec;
> 
> Apart from that, the change looks okay.
> 
> Carlos, this would be a regression, so I think we should fix it before
> the release.

Agreed. We should fix this ASAP for the 2.37 release.

Andreas, Thanks for putting together a fix. I'm reviewing this now.

-- 
Cheers,
Carlos.

Re: [PATCH] Account for octal marker in %#o format

[Carlos O'Donell]

On 1/30/23 04:53, Andreas Schwab via Libc-alpha wrote:

We don't need a bug number because this issues was never visible
to a user in a release.

I applied the fix carefully by hand, since as Florian notes this
seems to be from a branch where you are working on your other
fixes that are under review.

Confirmed test fails without the change on x86_64 and i686 and
passes after the fix. It is not impacted by your other changes
as far as I can tell. It correctly fixes the required space
calculation in both branches of the argument processing, and that
looks like the correct fix to me.

Note that CI/CD failed because patch fails to apply, but we
still want this fix in ASAP before the release.

OK for 2.37. Please commit ASAP.

No regressions on x86-64 and i686.

Tested-by: Carlos O'Donell <carlos@redhat.com>
Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  stdio-common/Makefile               |  1 +
>  stdio-common/tst-printf-oct.c       | 50 +++++++++++++++++++++++++++++
>  stdio-common/vfprintf-process-arg.c |  6 ++++
>  3 files changed, 57 insertions(+)
>  create mode 100644 stdio-common/tst-printf-oct.c
> 
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index 8150453944..c787450e5c 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -207,6 +207,7 @@ tests := \
>    tst-printf-bz25691 \
>    tst-printf-fp-free \
>    tst-printf-fp-leak \
> +  tst-printf-oct \

OK. Sorted correctly. Adds new test for regression.

>    tst-printf-round \
>    tst-printfsz \
>    tst-put-error \
> diff --git a/stdio-common/tst-printf-oct.c b/stdio-common/tst-printf-oct.c
> new file mode 100644
> index 0000000000..d1c88d5641
> --- /dev/null
> +++ b/stdio-common/tst-printf-oct.c
> @@ -0,0 +1,50 @@
> +/* Test printf width padding with octal format
> +   Copyright (C) 2023 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 <locale.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +
> +static int
> +do_test (void)
> +{
> +  char buf[80];
> +
> +  sprintf (buf, "%-7o", 123);
> +  TEST_COMPARE_STRING (buf, "173    ");
> +
> +  sprintf (buf, "%#-7o", 123);
> +  TEST_COMPARE_STRING (buf, "0173   ");
> +
> +  sprintf (buf, "%7o", 123);
> +  TEST_COMPARE_STRING (buf, "    173");
> +
> +  sprintf (buf, "%#7o", 123);
> +  TEST_COMPARE_STRING (buf, "   0173");
> +
> +  sprintf (buf, "%07o", 123);
> +  TEST_COMPARE_STRING (buf, "0000173");
> +
> +  sprintf (buf, "%#07o", 123);
> +  TEST_COMPARE_STRING (buf, "0000173");
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/stdio-common/vfprintf-process-arg.c b/stdio-common/vfprintf-process-arg.c
> index cd3eaf5c0c..279a0efa9d 100644
> --- a/stdio-common/vfprintf-process-arg.c
> +++ b/stdio-common/vfprintf-process-arg.c
> @@ -196,6 +196,9 @@ LABEL (unsigned_number):      /* Unsigned number of base BASE.  */
>          /* Account for 0X, 0x, 0B or 0b hex or binary marker.  */
>          width -= 2;
>  
> +      if (octal_marker)
> +        --width;
> +
>        if (is_negative || showsign || space)
>          --width;
>  
> @@ -257,6 +260,9 @@ LABEL (unsigned_number):      /* Unsigned number of base BASE.  */
>            width -= 2;
>          }
>  
> +      if (octal_marker)
> +	--width;
> +
>        width -= number_length + prec;
>  
>        Xprintf_buffer_pad (buf, L_('0'), prec);

-- 
Cheers,
Carlos.