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

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

On Sat, 15 Feb 2025 17:40:39 +0000
David Laight <david.laight.linux@gmail.com> wrote:

> 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	\  
> > > +	    )
> > >  #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'.

Thinking a bit more.
If the compiler can determine that 'rowsize >= 0' then the test will pass.
More modern compilers do better value tracking so that might be enough
to stop the compiler 'bleating'.

	David

> 
> 	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/  
>