From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H2,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 24978 invoked from network); 12 Sep 2022 13:59:23 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 12 Sep 2022 13:59:23 -0000 Received: (qmail 23610 invoked by uid 550); 12 Sep 2022 13:59:18 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 22524 invoked from network); 12 Sep 2022 13:59:17 -0000 Date: Mon, 12 Sep 2022 09:59:04 -0400 From: Rich Felker To: Gabriel Ravier Cc: gravier@gmail.com, musl@lists.openwall.com Message-ID: <20220912135904.GI9709@brightrain.aerifal.cx> References: <20220908163649.634728-1-gabravier@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220908163649.634728-1-gabravier@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] vfprintf: support C2x %b and %B conversion specifiers On Thu, Sep 08, 2022 at 06:36:49PM +0200, Gabriel Ravier wrote: > These specifiers allow for formatted input/output of binary integers, > and have been added to C2x with N2630. The uppercase B specifier is > not formally required by C2x, as only lowercase specifiers were > reserved by C, and thus an implementation could have been using > uppercase B for an extension of their own, but C2x still has a note > saying that it is recommended practice to implement it as the logical > counterpart to b. > > I have tested this on: > - glibc's tests for %b and %B > - The libc testsuite I'm developing over at https://github.com/GabrielRavier/yalibct > - musl's libc-test > - musl's libc-testsuite > > and observed no regressions. > --- > src/stdio/vfprintf.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/src/stdio/vfprintf.c b/src/stdio/vfprintf.c > index 9b961e7f..89426b72 100644 > --- a/src/stdio/vfprintf.c > +++ b/src/stdio/vfprintf.c > @@ -49,7 +49,7 @@ enum { > static const unsigned char states[]['z'-'A'+1] = { > { /* 0: bare types */ > S('d') = INT, S('i') = INT, > - S('o') = UINT, S('u') = UINT, S('x') = UINT, S('X') = UINT, > + S('o') = UINT, S('u') = UINT, S('x') = UINT, S('X') = UINT, S('b') = UINT, S('B') = UINT, > S('e') = DBL, S('f') = DBL, S('g') = DBL, S('a') = DBL, > S('E') = DBL, S('F') = DBL, S('G') = DBL, S('A') = DBL, > S('c') = CHAR, S('C') = INT, > @@ -59,7 +59,7 @@ static const unsigned char states[]['z'-'A'+1] = { > S('z') = ZTPRE, S('j') = JPRE, S('t') = ZTPRE, > }, { /* 1: l-prefixed */ > S('d') = LONG, S('i') = LONG, > - S('o') = ULONG, S('u') = ULONG, S('x') = ULONG, S('X') = ULONG, > + S('o') = ULONG, S('u') = ULONG, S('x') = ULONG, S('X') = ULONG, S('b') = ULONG, S('B') = ULONG, > S('e') = DBL, S('f') = DBL, S('g') = DBL, S('a') = DBL, > S('E') = DBL, S('F') = DBL, S('G') = DBL, S('A') = DBL, > S('c') = INT, S('s') = PTR, S('n') = PTR, > @@ -68,17 +68,20 @@ static const unsigned char states[]['z'-'A'+1] = { > S('d') = LLONG, S('i') = LLONG, > S('o') = ULLONG, S('u') = ULLONG, > S('x') = ULLONG, S('X') = ULLONG, > + S('b') = ULLONG, S('B') = ULLONG, > S('n') = PTR, > }, { /* 3: h-prefixed */ > S('d') = SHORT, S('i') = SHORT, > S('o') = USHORT, S('u') = USHORT, > S('x') = USHORT, S('X') = USHORT, > + S('b') = USHORT, S('B') = USHORT, > S('n') = PTR, > S('h') = HHPRE, > }, { /* 4: hh-prefixed */ > S('d') = CHAR, S('i') = CHAR, > S('o') = UCHAR, S('u') = UCHAR, > S('x') = UCHAR, S('X') = UCHAR, > + S('b') = UCHAR, S('B') = UCHAR, > S('n') = PTR, > }, { /* 5: L-prefixed */ > S('e') = LDBL, S('f') = LDBL, S('g') = LDBL, S('a') = LDBL, > @@ -88,11 +91,13 @@ static const unsigned char states[]['z'-'A'+1] = { > S('d') = PDIFF, S('i') = PDIFF, > S('o') = SIZET, S('u') = SIZET, > S('x') = SIZET, S('X') = SIZET, > + S('b') = SIZET, S('B') = SIZET, > S('n') = PTR, > }, { /* 7: j-prefixed */ > S('d') = IMAX, S('i') = IMAX, > S('o') = UMAX, S('u') = UMAX, > S('x') = UMAX, S('X') = UMAX, > + S('b') = UMAX, S('B') = UMAX, > S('n') = PTR, > } > }; > @@ -162,6 +167,12 @@ static char *fmt_o(uintmax_t x, char *s) > return s; > } > > +static char *fmt_b(uintmax_t x, char *s) > +{ > + for (; x; x>>=1) *--s = '0' + (x&1); > + return s; > +} > + > static char *fmt_u(uintmax_t x, char *s) > { > unsigned long y; > @@ -529,7 +540,7 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg, > if (!f) continue; > > z = buf + sizeof(buf); > - prefix = "-+ 0X0x"; > + prefix = "-+ 0X0x0B0b"; > pl = 0; > t = s[-1]; > > @@ -559,6 +570,10 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg, > a = fmt_x(arg.i, z, t&32); > if (arg.i && (fl & ALT_FORM)) prefix+=(t>>4), pl=2; > if (0) { > + case 'b': case 'B': > + a = fmt_b(arg.i, z); > + if (arg.i && (fl & ALT_FORM)) prefix += 9 + ((t == 'b') << 1), pl=2; > + } if (0) { > case 'o': > a = fmt_o(arg.i, z); > if ((fl&ALT_FORM) && p -- > 2.37.3 I'm not sure what the schedule on taking this or other C2x changes relative to the standard becoming official should be, so we should probably discuss that at some point. The only real hard concern is that we need to be careful not to take anything where the standard mandated behavior might change before it's final. For the above patch specifically: 1. There's a buffer overflow because you did not adjust the size formula for buf[]. Presently it's only 40-55 bytes (due to the inclusion of LDBL_MANT_DIG/4 in there, which is almost surely cruft predating the initial release) while a binary-form string requires at least 64 bytes. 2. Presumably the wide printf forms need to accept %b too. Does scanf need to accept them too? I think those would all be easy changes, since scanf already has the strto* core with arbitrary base available to it, and wide printf just calls back to the narrow one. I don't see anything else immediately wrong. No rush to submit an updated patch. Let's first figure out the timeline for C2x features, and I'll try to figure out what's going on with the weird buf[] size expression and clean it up separately so that your patch is just adding the feature and not also fighting with historical cruft. Rich