On Tue, 17 Dec 2024 at 14:52, Steven Rostedt rostedt@goodmis.org wrote:
Where the buf value holds the binary storage from vbin_printf() and written in trace_vbprintk(). Yeah, it looks like it does depend on the arguments being word aligned.
So the thing is, they actually aren't word-aligned at all.
Yes, the buffer is ostensibly individual words, and the buffer size is given in words, and 64-bit data is saved/fetched as two separate words.
But if you look more closely, you'll see that the way the buffer is managed is actually not as a word array at all, but using
char *str, *end;
instead of word pointers. And the reason is because it does
str = PTR_ALIGN(str, sizeof(type)); ... str += sizeof(type);
so byte-sized data is *not* given a word, it's only given a single char, and then if it's followed by an "int", the pointer will be aligned at that point.
It does mean that the binary buffers are a bit denser, but since %c and %hhd etc are VERY unusual (and often will be mixed with int-sized data), that denser format isn't commonly an actual advantage.
And the reason I noticed this? When I was trying to clean up and simplify the vsnprintf() code to not have so many different cases, the *regular* number handling for char/short/int-sized cases ends up being just one case that looks like this:
num = get_num(va_arg(args, int), fmt.state, spec);
because char/short/int are all just "va_arg(args, int)" values.
Then the actual printout depends on that printf_spec thing (and, in my experimental branch, that "fmt.state", but that's a local trial thing where I split the printf_spec differently for better code generation).
So basically the core vfsprintf case doesn't need to care about fetching char/short/int, because va_args always just formats those kinds of arguments as int, as per the usual C implicit type expansion rules.
But the binary printf thing then has to have three different cases, because unlike the normal calling convention, it actually packs char/short/int differently. So with all those nice cleanups I tried still look like this:
case FORMAT_STATE_2BYTE: num = get_num(get_arg(short), fmt.state, spec); break; case FORMAT_STATE_1BYTE: num = get_num(get_arg(char), fmt.state, spec); break; default: num = get_num(get_arg(int), fmt.state, spec); break;
which is admittedly still a lot better than the current situation in top-of-tree, which has separate versions for a *lot* more types.
So right now top-of-tree has 11 different enumerated cases for number printing:
FORMAT_TYPE_LONG_LONG, FORMAT_TYPE_ULONG, FORMAT_TYPE_LONG, FORMAT_TYPE_UBYTE, FORMAT_TYPE_BYTE, FORMAT_TYPE_USHORT, FORMAT_TYPE_SHORT, FORMAT_TYPE_UINT, FORMAT_TYPE_INT, FORMAT_TYPE_SIZE_T, FORMAT_TYPE_PTRDIFF
and in my test cleanup, I've cut this down to just four cases: FORMAT_STATE_xBYTE (x = 1, 2, 4, 8).
And the actual argument fetch for the *normal* case is actually just two cases (because the 8-byte and the "4 bytes or less" cases are different for va_list, but 1/2/4 bytes are just that single case).
But the binary printf argument save/fetch is not able to be cut down to just two cases because of how it does that odd "ostensibly a word array, but packed byte/short fields" thing.
Oh well. It's not a big deal. But I was doing this to see if I could regularize the vsnprintf() logic and make sharing better - and then just the binary version already causes unnecessary duplication.
If the *only* thing that accesses that word array is vbin_printf and bstr_printf, then I could just change the packing to act like va_list does (ie the word array would *actually* be a word array, and char and short values would get individual words).
But at least the bpf cde seems to know about the crazy packing, and also does that
tmp_buf = PTR_ALIGN(tmp_buf, sizeof(u32));
in bpf_bprintf_prepare() because it knows that it's not *actually* an array of words despite it being documented as such.
Of course, the bpf code only does the packed access thing for '%c', and doesn't seem to know that the same is true of '%hd' and '%hhd', presumably because nobody actually uses that.
Let's add Alexei to the participants. I think bpf too would actually prefer that the odd char/short packing *not* be done, if only because it clearly does the wrong thing as-is for non-%c argument (ie those %hd/%hhd cases).
Who else accesses that odd "binary printed pseudo-word array"?
Linus