On Sat, 12 Sep 2026 18:47:22 +0900
Donggeun Yoo <[email protected]> wrote:
> create_field_var_hist() builds a hist trigger on the onmatch() event by
> copying the key list of the compatible histogram found there, reading
> each name from key_field->field->name. That pointer is NULL when the key
> is not an event field: parse_field() leaves it NULL for common_cpu,
> common_comm, common_timestamp, common_stacktrace and hitcount.
> compatible_keys() compares only the type, size and signedness of each
> key, so two common_cpu keys match and the loop faults.
>
> The generated trigger is a real histogram whose element the variable is
> later read out of, so it has to bucket the same way as the one it
> mirrors. Render the keys with expr_field_str(), which carries .log2 and
> .usecs, modifiers that change a key's value. Two things it did not carry
> are added here. get_hist_field_flags() reports .stacktrace for the
> common_stacktrace pseudo-field too, which parse_field() takes only on a
> real field, so gate it on having one. And it reports a bare "buckets"
> with the size held separately in hist_field->buckets, so append that the
> way hist_field_print() does. expr_field_str() also renders expression
> operands, so an expression over a bucketed field now prints the size
> too, matching what hist_field_print() already shows for the key.
The above change log is extremely hard to read. Did you get it directly
from AI? That tends to be overly verbose and adds way more data than
needed making it harder to find the important parts. Please make the
change logs more precise to what the issue is and remove the unneeded
details.
For this patch, I'm currently testing it, but let me try to decipher it
(but please clean up future patches)...
OK, I gave up on the change log as it's just garbage. I ended up
triggering the bug mentioned and looking at the code and figured it out
myself.
OK, this patch does three things so it really needs to be three
different patches. One, it fixes the reported bug by the change to call
expr_field_str(). The change log for that bug should be:
In event_hist_trigger_parse() where it needs to create actions like
"onmatch", it calls:
event_hist_trigger_parse() {
create_actions() {
action_create() {
trace_action_create() {
trace_action_create_field_var() {
create_field_var_hist()
Where create_field_var_hist() does a loop on the hist_data
representing the keys. The issue is, if the keys uses one of the
pseudo field types (like common_cpu), the hist_data field element
will have NULL for its field member causing a NULL pointer
dereference when accessing the key_field->field->name.
Instead of accessing it directly, use the proper handler
expr_field_str() to get the name.
> @@ -3090,7 +3093,7 @@ create_field_var_hist(struct hist_trigger_data
> *target_hist_data,
> key_field = hist_data->fields[i];
> if (!first)
> seq_buf_putc(&s, ',');
> - seq_buf_puts(&s, key_field->field->name);
> + expr_field_str(key_field, &s);
> first = false;
> }
The changes and the change log for the other two is confusing and it
doesn't show examples of the errors. They can be dropped from the NULL
pointer dereference fix and sent separately with examples of what is
actually wrong.
Oh, and there's another bug here. With your example, where you used
"prio" for sched_switch (which isn't a field), there's an error message
created, but the command still errors out.
[ 415.300003] hist:sched:sched_switch: error: Couldn't parse field variable
Command:
hist:keys=stacktrace:wakeup_lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).my_synth($wakeup_lat,prio)
-- Steve
