Fix: for loops with per-cpu aggregations #3664
Conversation
jordalgo
requested review from
ajor,
viktormalik,
danobi and
fbs
as code owners
|
This might need to go into the release branch. Will check tomorrow. Edit: Yes, going to cherry-pick this on to the new release branch after it gets merged. |
| @@ -72,3 +72,11 @@ EXPECT_REGEX ^abc\ndef\ndef$ | |||
| NAME variable context multiple | |||
| PROG BEGIN { @map[0] = 0; $var1 = 123; $var2 = "abc"; for ($kv : @map) { print(($var1, $var2)); } exit(); } | |||
| EXPECT (123, abc) | |||
|
|
|||
| NAME map two keys with a per cpu aggregation | |||
There was a problem hiding this comment.
yes, both of these tests don't pass on master 😞
jordalgo
force-pushed
the
fix_for_loop_per_cpu_agg
branch
from
f2d35a9 to
3a42669
Compare
tests/codegen/llvm/avg_cast_loop.ll
Outdated
There was a problem hiding this comment.
why does this fix work? looking at this codegen change, seems like the PR is a noop.
(although avoiding a stack allocation is still better, but that's different)
There was a problem hiding this comment.
I'll add a codegen test too but basically for values inBpfMemory (like tuples) the key in the line b_.CreateStore(key, key_ptr); was populated with a pointer instead of the actual value so key_ptr was storing a pointer and then we were taking a pointer to a pointer for the map lookup (see line 4557)
There was a problem hiding this comment.
Ah ok. This detail would be good to include in PR and commit message
jordalgo
force-pushed
the
fix_for_loop_per_cpu_agg
branch
from
3a42669 to
47e9a7b
Compare
For a map that had multiple keys or a single key that was `inBpfMemory` and contained a per-cpu aggregation as a value (e.g. `count` or `sum`) then retrieving the value from the `$kv` tuple was broken e.g. ``` @Map[1, 2] = count(); for ($kv : @Map) { print($kv.1); // this would always be 0 } ``` The fix is to just use the callback arg ptr instead of allocating a new value (as the key) and trying to store something in it. Previously, for values inBpfMemory (like tuples) the key in the line `b_.CreateStore(key, key_ptr);` was populated with a pointer instead of the actual value so key_ptr was storing a pointer and then we were taking a pointer to a pointer for the map lookup, which was wrong.
jordalgo
force-pushed
the
fix_for_loop_per_cpu_agg
branch
from
47e9a7b to
fc91148
Compare
|
Does this need a fixes entry in changelog? Or was buggy behavior not released yet? |
|
@danobi I'm going to make a PR for the release branch with the changelog entry and then another PR for master with the changelog entry under the new release branch section. 🏋️ |
For a map that had multiple keys or a single key
that was
inBpfMemoryand contained a per-cpuaggregation as a value (e.g.
countorsum)then retrieving the value from the
$kvtuple was broken e.g.The fix is to just use the callback arg ptr
instead of allocating a new value (as the key)
and trying to store something in it.
Previously, for values inBpfMemory (like tuples) the
key in the line
b_.CreateStore(key, key_ptr);waspopulated with a pointer instead of the actual value
so key_ptr was storing a pointer and then we were
taking a pointer to a pointer for the map lookup,
which was wrong.
Checklist
man/adoc/bpftrace.adocCHANGELOG.md