| Message ID | 20250214162436.241359-2-nnac123@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Use new for_each macro to create hexdumps | expand |
Hi Dave, Thanks for reviewing, On 2/14/25 12:00 PM, Dave Marquardt wrote: > Nick Child <nnac123@linux.ibm.com> writes: > >> + (i) += (rowsize) == 16 || (rowsize) == 32 ? (rowsize) : 16 \ > Nit: If you left out the (rowsize) == 16 check here you'd still add 16 > to (i). I was trying to have this translate into "if invalid rowsize was used then default to 16" since hex_dump_to_buffer has a very similar conditional. But I agree, logically it looks strange. If I send a v3 (I also foolishly forgot the v2 tag in this patch), I will change this like to + (i) += (rowsize) == 32 ? 32 : 16 \
+ 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 _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) ^ ././include/linux/compiler_types.h:523:4: note: in definition of macro '__compiletime_assert' prefix ## suffix(); \ ^~~~~~ ././include/linux/compiler_types.h:542:2: note: in expansion of macro '_compiletime_assert' _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) ^~~~~~~~~~~~~~~~~~~ ./include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) ^~~~~~~~~~~~~~~~~~ ./include/linux/minmax.h:93:2: note: in expansion of macro 'BUILD_BUG_ON_MSG' BUILD_BUG_ON_MSG(!__types_ok(ux, uy), \ ^~~~~~~~~~~~~~~~ ./include/linux/minmax.h:98:2: note: in expansion of macro '__careful_cmp_once' __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) ^~~~~~~~~~~~~~~~~~ ./include/linux/minmax.h:105:19: note: in expansion of macro '__careful_cmp' #define min(x, y) __careful_cmp(min, x, y) ^~~~~~~~~~~~~ ./include/linux/printk.h:774:5: note: in expansion of macro 'min' min((len) - (i), rowsize), \ ^~~ lib/hexdump.c:272:2: note: in expansion of macro 'for_each_line_in_hex_dump' for_each_line_in_hex_dump(i, rowsize, linebuf, sizeof(linebuf), ^~~~~~~~~~~~~~~~~~~~~~~~~ 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 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/
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/
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/ >
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).
On Sun, 16 Feb 2025 09:32:04 +0000 Simon Horman <horms@kernel.org> wrote: >... > > > 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. Doesn't matter, nothing with assign the address of the function to a variable so changing the types (to unsigned) doesn't affect any callers. The values better be positive! I just changed the prototypes (include/linux/printk.h) to make both rowsize and groupsize 'unsigned int'. The same change in lib/hexdump.c + changing the local 'i, linelen, remaining' to unsigned int and it all compiled. FWIW that hexdump code is pretty horrid (especially if groupsize != 1). David
Thank you David and Simon for testing and review! On Sun, Feb 16, 2025 at 11:24:30AM +0000, David Laight wrote: > > I just changed the prototypes (include/linux/printk.h) to make both > rowsize and groupsize 'unsigned int'. > The same change in lib/hexdump.c + changing the local 'i, linelen, remaining' > to unsigned int and it all compiled. > > FWIW that hexdump code is pretty horrid (especially if groupsize != 1). > Given this discussion and my own head-scratching, I think I will take a closer look at hex_dump_to_buffer. I was trying to avoid editing this function due the number of callers it has across the kernel. But I think we can get away with keeping the API (but change args to uint's) and editing the body of the function to always iterate byte-by-byte, addding space chars where necessary. At the cost of a few more cycles, this will allow for more dynamic values for rowsize and groupsize and shorten the code up a bit. This would also address the "Side question" in my cover letter. Will send a v3 regardless if I can figure that out or not. The return value of hex_dump_to_buffer on error still irks me a bit but I don't think that can easily be changed. Thanks again!
On 2/17/25 4:09 PM, Nick Child wrote: > Thank you David and Simon for testing and review! > > On Sun, Feb 16, 2025 at 11:24:30AM +0000, David Laight wrote: >> >> I just changed the prototypes (include/linux/printk.h) to make both >> rowsize and groupsize 'unsigned int'. >> The same change in lib/hexdump.c + changing the local 'i, linelen, remaining' >> to unsigned int and it all compiled. >> >> FWIW that hexdump code is pretty horrid (especially if groupsize != 1). >> > > Given this discussion and my own head-scratching, I think I will take a > closer look at hex_dump_to_buffer. > > I was trying to avoid editing this function due the number of callers it > has across the kernel. But I think we can get away with keeping the > API (but change args to uint's) and editing the body of the function > to always iterate byte-by-byte, addding space chars where necessary. At the > cost of a few more cycles, this will allow for more dynamic values > for rowsize and groupsize and shorten the code up a bit. This would also > address the "Side question" in my cover letter. Will send a v3 > regardless if I can figure that out or not. > > The return value of hex_dump_to_buffer on error still irks me a bit but > I don't think that can easily be changed. For the new versions, please: - respect the 24h grace period: https://elixir.bootlin.com/linux/v6.11.8/source/Documentation/process/maintainer-netdev.rst#L15 - add the target tree in the subj prefix (in this case 'net-next') - ensure all the patches in the series have consistent subj prefix, otherwise patchwork will be fooled. I think it would be better to avoid modifying hex_dump_to_buffer(), if not necessary, and I think you could do so either changing the type used in patch 2 or even dropping such patch. /P
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,
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(+)