Re: [PATCH 1/3] hexdump: Implement macro for converting large buffers

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

On Sat, 15 Feb 2025 16:36:12 +0000
Simon Horman <horms@kernel.org> wrote:

> + David Laight
> 
> On Fri, Feb 14, 2025 at 10:24:34AM -0600, Nick Child wrote:
> > Define for_each_line_in_hex_dump which loops over a buffer and calls
> > hex_dump_to_buffer for each segment in the buffer. This allows the
> > caller to decide what to do with the resulting string and is not
> > limited by a specific printing format like print_hex_dump.
> > 
> > Signed-off-by: Nick Child <nnac123@linux.ibm.com>
> > ---
> >  include/linux/printk.h | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > index 4217a9f412b2..559d4bfe0645 100644
> > --- a/include/linux/printk.h
> > +++ b/include/linux/printk.h
> > @@ -755,6 +755,27 @@ enum {
> >  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> >  			      int groupsize, char *linebuf, size_t linebuflen,
> >  			      bool ascii);
> > +/**
> > + * for_each_line_in_hex_dump - iterate over buffer, converting into hex ASCII
> > + * @i: offset in @buff
> > + * @rowsize: number of bytes to print per line; must be 16 or 32
> > + * @linebuf: where to put the converted data
> > + * @linebuflen: total size of @linebuf, including space for terminating NUL
> > + *		IOW >= (@rowsize * 2) + ((@rowsize - 1 / @groupsize)) + 1
> > + * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + * @buf: data blob to dump
> > + * @len: number of bytes in the @buf
> > + */
> > + #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
> > +				   buf, len) \
> > +	for ((i) = 0;							\
> > +	     (i) < (len) &&						\
> > +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> > +				min((len) - (i), rowsize),		\
> > +				(rowsize), (groupsize), (linebuf),	\
> > +				(linebuflen), false);			\
> > +	     (i) += (rowsize) == 16 || (rowsize) == 32 ? (rowsize) : 16	\

WTF?
I'm not sure why there is a restriction on 'rowsize' by that increment
makes no sense at all.

> > +	    )
> >  #ifdef CONFIG_PRINTK
> >  extern void print_hex_dump(const char *level, const char *prefix_str,
> >  			   int prefix_type, int rowsize, int groupsize,  
> 
> Hi Nick,
> 
> When compiling with gcc 7.5.0 (old, but still supported AFAIK) on x86_64
> with patch 2/3 (and 1/3) applied I see this:
> 
>   CC      lib/hexdump.o
> In file included from <command-line>:0:0:
> lib/hexdump.c: In function 'print_hex_dump':
> ././include/linux/compiler_types.h:542:38: error: call to '__compiletime_assert_11' declared with attribute error: min((len) - (i), rowsize) signedness error
...
> Highlighting the min line in the macro for context, it looks like this:
> 
> 	min((len) - (i), rowsize)
> 
> And in this case the types involved are:
> 
> 	size_t len
> 	int i
> 	int rowsize

Yep, that should fail for all versions of gcc.
Both 'i' and 'rowsize' should be unsigned types.
In fact all three can be 'unsigned int'.

	David

> 
> This is not a proposal, but I made a quick hack changing they type of rowsize
> to size_t and the problem goes away. So I guess it is the type missmatch
> between the two arguments to min that needs to be resolved somehow.
> 
> 
> FWIIW, you should be able to reproduce this problem fairly easily using
> the toolchain here:
> https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/7.5.0/