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

[Simon Horman <horms@kernel.org>]

On Sat, Feb 15, 2025 at 05:46:35PM +0000, David Laight wrote:
> 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'.

To give a bit more context, a complication changing the types is that the
type of len and rowsise (but not i) is in the signature of the calling
function, print_hex_dump(). And I believe that function is widely used
throughout the tree.

> 
> 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'.

FTR, I did not see this with GCC 14.2.0 (or clang 19.1.7).