Fix PR #1416: unsigned int overflow bug and unstable tests #1436
Conversation
suyuee
force-pushed
the
fix_print_avg_patch
branch
from
19e7356
to
e758f36
Compare
suyuee
changed the title
suyuee
changed the title
suyuee
force-pushed
the
fix_print_avg_patch
branch
from
e758f36
to
b536854
Compare
|
Update the CHANGELOG.md |
| EXPECT ^4$ | ||
| NAME print_avg_map_args | ||
| RUN bpftrace -e 'BEGIN { @["a"] = avg(10); @["b"] = avg(20); @["c"] = avg(30); @["d"] = avg(40); print(@, 2, 10); print("END"); clear(@); exit(); }' | ||
| EXPECT Attaching 1 probe\.\.\.\n@\[c\]: 3\n@\[d\]: 4\n\nEND |
There was a problem hiding this comment.
I think the regex does a line-by-line match which means I think you can do this
| EXPECT Attaching 1 probe\.\.\.\n@\[c\]: 3\n@\[d\]: 4\n\nEND | |
| EXPECT @\[c\]: 3\n@\[d\]: 4\n\nEND |
There was a problem hiding this comment.
Hmm actually, I do this on purpose. I think the 'EXPECT' actually means 'CONTAINS' in runtime test framework, in other words EXPECT @\[c\]: 3\n@\[d\]: 4\n\nEND would allow something like:
Attaching 1 probe...
@[a]: 1
@[b]: 2
@[c]: 3
@[d]: 4
END
because it contains specified pattern. Prefix the pattern with the line 'Attaching 1 probe...' can prevent it from ignoring the wrong output.
There was a problem hiding this comment.
Hmm, what "wrong output" would it avoid? Isn't it enough to match only
@[a]: 1
@[b]: 2
@[c]: 3
@[d]: 4
END
Why would we need to match the entirety of the output?
There was a problem hiding this comment.
I'm not sure if I got it right here, but here is what I think. Since we have top = 2, the script below should only print the top 2 entries:
# bpftrace -e 'BEGIN { @["a"] = avg(10); @["b"] = avg(20); @["c"] = avg(30); @["d"] = avg(40); print(@, 2, 10); print("END"); clear(@); exit(); }'
Attaching 1 probe...
@[c]: 3
@[d]: 4
END
And since we want top to be effective, we do not actually want to see all four entries being printed out. If the output contains four entries, top is not functioning properly; but in this case, the four-entry output can still pass the test with EXPECT @\[c\]: 3\n@\[d\]: 4\n\nEND because the output contains this pattern.
If I'm not getting it wrong, the output with all four entries should be the wrong output we may want to avoid here. The expected output should contains exactly @[c]: 3 and @[d]: 4.
There was a problem hiding this comment.
Ah yeah that makes sense. Thanks for the explanation
| EXPECT ^@\[[0-9]+\]: 1$ | ||
| NAME print_avg_map_with_large_top | ||
| RUN bpftrace -e 'BEGIN { @["a"] = avg(10); @["b"] = avg(20); @["c"] = avg(30); @["d"] = avg(40); print(@, 10, 10); print("END"); clear(@); exit(); }' | ||
| EXPECT Attaching 1 probe\.\.\.\n@\[a\]: 1\n@\[b\]: 2\n@\[c\]: 3\n@\[d\]: 4\n\nEND |
|
Ah, just saw #1437 . |
The merged PR which enables `top` and `div` args of `print` for `avg` maps has two issues that need addressing: 1. It came with subtractions of unsigned integers which can easily cause negative overflows. 2. The runtime tests it came with are somehow unstable and fails randomly. This patch brings more straightforward tests.
suyuee
force-pushed
the
fix_print_avg_patch
branch
from
b536854
to
2103866
Compare
|
Rebased. |
Related to #1416.The merged PR which enables
topanddivargs ofprintforavgmaps hastwo issues that need addressing:
negative overflows.
patch brings more straightforward tests.
Checklist
docs/reference_guide.mdCHANGELOG.md