Re: [PATCH next 1/1] lib: Optimise hex_dump_to_buffer()

[David Laight <david.laight.linux@gmail.com>]

On Sun, 16 Feb 2025 22:47:49 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sun, Feb 16, 2025 at 08:19:01PM +0000, David Laight wrote:
> > Fastpath the normal case of single byte output that fits in the buffer.
> > Output byte groups (byteswapped on little-endian) without calling snprintf().
> > Remove the restriction that rowsize must be 16 or 32.
> > Remove the restriction that groupsize must be 8 or less.
> > If groupsize isn't a power of 2 or doesn't divide into both len and
> >   rowsize it is set to 1 (otherwise byteswapping is hard).
> > Change the types of the rowsize and groupsize parameters to be unsigned types.  
> 
> > Tested in a userspace harness, code size (x86-64) halved to 723 bytes.  
> 
> Does it imply running the respective test cases we have?
> Do you need to add more test cases? I believe so.
> Without test cases added it's a no go.
> 
> > +extern size_t hex_dump_to_buffer(const void *buf, size_t len, size_t rowsize,
> > +			      size_t groupsize, char *linebuf,
> > +			      size_t linebuflen, bool ascii);  
> 
> Looking at another thread where upper layer function wants to have unsigned
> long flags instead of bool ascii, I would also do the new API, that takes flags
> and leave the old one as a simple wrapper with all restrictions being applied.

I can't imagine any code relying on the rowsize being converted to 16.
And (elsewhere) I've definitely needed to do hexdumps with strange numbers
of bytes/line.

> 
> And again, provide it together with a bunch of test cases.
> 
> ...
> 
> > +			dst[0] = hex_asc_hi(ch);
> > +			dst[1] = hex_asc_lo(ch);  
> 
> We have hex_pack_byte() or so

At least some versions of gcc have generated better code if you don't
use *ptr++ but do the increment afterwards.
It is also what the old version used.
Not to mention being another wrapper you need to look up to work out
what the code is doing.

..
> ...
> 
> > -		linebuf[lx++] = (isascii(ch) && isprint(ch)) ? ch : '.';
> > +		*dst++ = ch >= ' ' && ch < 0x7f ? ch : '.';  
> 
> Please also add a test case for this to make sure it has no changes.

Well isascii() usually checks for the 0x80 bit being clear and isprint()
rejects control characters and 'del' (0x7f).
I'm not sure what isascii() does for EBCDIC type charsets - but I don't
expect Linux runs on any of those so who cares.
Oh, and isprint() seems to be based on a memory lookup in an _ctype[] array.
Very 1970s.

	David

>