Fixes for watchpoint probes and runtime tests #3161
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viktormalik
force-pushed
the
watchpoint-fix
branch
from
a2e86ff
to
7f3b44d
Compare
Commit ccb7785 ("Add probe index to LLVM function name") changed the way LLVM function and section names are generated for probes. It did it wrong for watchpoint setup probes where it generated <attach_point>_<index>_wp_setup_<index> instead of <attach_point>_wp_setup_<index> for the LLVM function name (notice the extra "_<index>"). The same difference is for sections names which just have "s_" prepended. Consequently, bpftrace was not able to find BPF programs for watchpoint setup probes as they were not stored under expected section names. Fix the function/section name to have the expected form.
Since commit 611176d ("Runtime tests: Make EXPECT only match exact lines of output"), the EXPECT clause in runtime tests must contain the exact entire line. The change has not been reflected in a few runtime tests and it was missed b/c they are not run in the CI (this will be addressed by the following patch). Update the affected tests to check for correct lines.
During CMake, all runtime test files are copied into the CMake build directory using add_custom_command which issues 'cmake -E copy ...'. For some reason, the last file in the tests/runtime directory (tests/runtime/watchpoint) is never copied which causes two issues: - the watchpoint tests are never run in CI, - even if the local setup already has tests/runtime/watchpoint in the build directory (it seems to have been working in the past), it is never updated. I'm not sure if this is caused by our change or a CMake update, however, our approach is IMHO not correct as it makes the runtime_tests target depend on the runtime tests files from the source directory. Changing it to depend on the (newly copied) files from the build directory eliminates the above issue.
viktormalik
force-pushed
the
watchpoint-fix
branch
from
7f3b44d
to
ac6c37a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix two issues with watchpoint probes/tests introduced by recent PRs:
EXPECTclauses for watchpoint runtime tests,The reason why neither of these was captured is a strange issue in CMake. During CMake run, we copy files from
tests/runtime/tobuild/tests/runtime, however, the last file (in our casetests/runtime/watchpoint) is never copied. Due to this, watchpoint tests are not run in the CI and also do not get updated in local builds (it seems to have worked in the past b/c I had the file in my build directory but it was never updated).I'm not sure if this is caused by our change or by a CMake update, however, our approach is IMHO not correct as it makes the
runtime_teststarget depend on the runtime tests files from the source directory. Changing it to depend on the (newly copied) files from the build directory eliminates the above issue. This is done by the last commit in the PR.Checklist
man/adoc/bpftrace.adocCHANGELOG.md