On Wed, 18 Dec 2024 at 07:31, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> I went to test this by adding the following:
>
> diff --git a/samples/trace_printk/trace-printk.c b/samples/trace_printk/trace-printk.c
> index cfc159580263..1ff688637404 100644
> --- a/samples/trace_printk/trace-printk.c
> +++ b/samples/trace_printk/trace-printk.c
> @@ -43,6 +43,17 @@ static int __init trace_printk_init(void)
>
> trace_printk(trace_printk_test_global_str_fmt, "", "dynamic string");
>
> + trace_printk("Print unsigned long long %llu\n", -1LL);
> + trace_printk("Print long long %lld\n", -1LL);
> + trace_printk("Print unsigned long %llu\n", -1L);
> + trace_printk("Print long %ld\n", -1L);
> + trace_printk("Print unsigned int %u\n", -1);
> + trace_printk("Print int %d\n", -1);
> + trace_printk("Print unsigned short %hu\n", (short)-1);
> + trace_printk("Print short %hd\n", (short)-1);
> + trace_printk("Print unsigned char %hhu\n", (char)-1);
> + trace_printk("Print char %hhd\n", (char)-1);
For testing the real corner cases, you should probably check the real
truncation handling.
IOW, things like '%hh{d,u}' together with a value like 0x3456789ab.
Because yes, the truncation is very much part of the number handling,
and is actually a very important part of printk, and really the only
actual reason '%hhd' and friends even exist (without truncation, you'd
just use %d and %u).
The fact that both you and Rasmus felt that part needed more of a
comment clearly just means that I may be more aware of it than most
people are.
Because I considered the "handle sign" issue to be also making sure we
handle the size and the *lack* of sign corrrectly.
So I'll extend on the comment.
As you also point out with your tracing test:
> modprobe-905 [003] ..... 113.624842: bprint: [FAILED TO PARSE] ip=0xffffffffc060e045 fmt=0xffff8c05c338e760 buf=ARRAY[]
> modprobe-905 [003] ..... 113.624843: bprint: [FAILED TO PARSE] ip=0xffffffffc060e045 fmt=0xffff8c05c338ec40 buf=ARRAY[]
> modprobe-905 [003] ..... 113.624843: bprint: [FAILED TO PARSE] ip=0xffffffffc060e045 fmt=0xffff8c05c338e280 buf=ARRAY[]
>
> Those "[FAILED TO PARSE]" messages have nothing to do with your code, but
> it means that it doesn't handle 'h' at all. Even the "unsigned short"
> printed but still failed to parse properly.
Yeah, %h{d,u} and %hh{d,u} are not hugely common, and apparently it's
not just your tracing tools that don't understand them: Alexei
reported that the bpf binary printk code also refused them.
That said, they *do* exist in the kernel, including in tracing:
git grep 'TP_printk.*".*%hh*[ud].*"'
doesn't return lots of hits, but does report a handful.
> This is because libtraceevent appears to not support "%h" in print formats.
> That at least means there would be no breakage if they are modified in any
> way.
Oh, %hd is not getting modified (and if I did, that would be a major bug).
It's very much a part of the standard printf format, and is very much
inherent to the whole varargs and C integer promotion rules (ie you
literally *cannot* pass an actual 'char' value to a varargs function -
the normal C integer type extension rules apply).
So this is not really some odd kernel extension, and while there are
only a handful of users in tracing (that apparently trace-cmd cannot
deal with), it's not even _that_ uncommon in general:
git grep '".*%hh*[ud].*"' | wc -l
reports that we have 501 of them in the kernel sources.
Linus
