Fix runtime tests for aarch64 #3185
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
ttreyer
force-pushed
the
fix-aarch64-fedora40
branch
3 times, most recently
from
97c1afb
to
77ec3c0
Compare
ttreyer
changed the title
ttreyer
force-pushed
the
fix-aarch64-fedora40
branch
from
77ec3c0
to
68328c8
Compare
ajor
reviewed
May 22, 2024
src/types.cpp
Outdated
| @@ -256,7 +256,7 @@ SizedType CreateInteger(size_t bits, bool is_signed) | |||
| // enough information to figure out the exact size of the integer. Later | |||
| // passes infer the exact size. | |||
| assert(bits == 0 || bits == 1 || bits == 8 || bits == 16 || bits == 32 || | |||
| bits == 64); | |||
| bits == 64 || bits == 128); | |||
There was a problem hiding this comment.
Do you know what happens in codegen if something tries to use a 128-bit integer?
There was a problem hiding this comment.
Let's add a test and figure it out...
There was a problem hiding this comment.
@ajor Following up on our chat, I modified SizedType to accept 128-bits integers. However, they get truncated down to only 64-bits with a warning to the user. The warning is done by the semantic_analyser, then the codegen_llvm insert the truncate for a FieldAccess on a user-defined type, if required.
|
Confirmed that |
ttreyer
force-pushed
the
fix-aarch64-fedora40
branch
2 times, most recently
from
aa3c233
to
6bd069d
Compare
ajor
requested changes
Jun 7, 2024
There was a problem hiding this comment.
Looks like it's possible to get a 128-bit integer through to codegen and have it trigger a seg-fault:
void uprobeFunction3(__int128_t bigint) {}
uprobe:./build/tests/testprogs/uprobe_test:uprobeFunction3 {
print(args.bigint);
}
I think for simplicity, codegen shouldn't know anything about 128-bit integers at all - they should get truncated in the type system before they reach that point.
ttreyer
force-pushed
the
fix-aarch64-fedora40
branch
2 times, most recently
from
4a854de
to
8028901
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| __uint128_t y = 0xEFEFEFEFEFEFEFEF; | ||
| __uint128_t z = 0xCDCDCDCDCDCDCDCD; | ||
| __uint128_t w = 0xABABABABABABABAB; | ||
| uprobeFunctionUint128(x, y, z, w); |
Check warning
Code scanning / CodeQL
Expression has no effect Warning
ttreyer
force-pushed
the
fix-aarch64-fedora40
branch
from
8028901
to
f7c29f8
Compare
jordalgo
reviewed
Jun 13, 2024
* Update `CreateInteger` to accept 128-bits integers. The ARM specific [`struct user_fpsimd_state`](https://elixir.bootlin.com/linux/latest/source/arch/arm64/include/uapi/asm/ptrace.h#L95) has a 128-bits integers. * Update DWARF parser to consider arrays of `unsigned char` as strings. The `char`s from "tests/data/data_source.c"'s `struct Arrays::char_arr` have the Basic Type `lldb::eBasicTypeUnsignedChar`. Is this valid? Is there a bug in GCC for generating wrong DWARF? * Force frame pointers for each `testprogs` and `testlibs`. The compiler doesn't have to set-up a frame pointer for leaf functions. We enforce their generation with `-fno-omit-frame-pointer` AND `-mno-omit-leaf-frame-pointer`. * Ignore `STT_NOTYPE` symbols from the symbol table. ARM binaries might contain [mapping symbols](https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst#mapping-symbols), which are not relevant to bpftrace. These symbols messes with symbolication, as they might have the same address as other symbols. * Fix runtime test `other.positional ustack`. The expected regex would not tolerate extra stack trace. By using the `s` flag with `.*`, we accept any number of extra lines, while being strict enough to validate that `uprobeFunction1` is on top of the last stack trace. * Fix `runner.py` to escape invalid characters comming from the test's stdout. After these fixes, the following runtime tests still fail: * `probe.kprobe_offset_fail_size`: the function `AttachedProbe::resolve_offset_kprobe` cannot locate `vmlinux`, because my machine only has `vmlinuZ`, and thus can't check the boundaries of `vfs_read`. The Kernel didn't reject the probe `vfs_read+100000`. Shouldn't this function use `/proc/kallsyms` instead? * `call.path`: The `path` builtin function returns path with the form `/dev/pts/4`, but never returns the expected path of the temporary file created for the test. Is this a Kernel bug? * `call.strcontains`: This test uses the `path` builtin function and fails because of the issue above. f
ttreyer
force-pushed
the
fix-aarch64-fedora40
branch
from
f7c29f8
to
8a69419
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.
This PR fixes the following tests, that are failing when running on ARM:
CreateIntegerto accept 128-bits integers.The ARM specific
struct user_fpsimd_statehas a 128-bits integers.unsigned charas strings.The
chars from "tests/data/data_source.c"'sstruct Arrays::char_arrhave the Basic Typelldb::eBasicTypeUnsignedChar. Is this valid? Is there a bug in GCC for generating wrong DWARF?testprogsandtestlibs.The compiler doesn't have to set-up a frame pointer for leaf functions. We enforce their generation with
-fno-omit-frame-pointerAND-mno-omit-leaf-frame-pointer.STT_NOTYPEsymbols from the symbol table.ARM binaries might contain mapping symbols, which are not relevant to bpftrace. These symbols messes with symbolication, as they might have the same address as other symbols.
other.positional ustack.The expected regex would not tolerate extra stack trace. By using the
sflag with.*, we accept any number of extra lines, while being strict enough to validate thatuprobeFunction1is on top of the last stack trace.After these fixes, the following runtime tests still fail:
probe.kprobe_offset_fail_size: the functionAttachedProbe::resolve_offset_kprobecannot locatevmlinux, because my machine only hasvmlinuZ, and thus can't check the boundaries ofvfs_read. The Kernel didn't reject the probevfs_read+100000. Shouldn't this function use/proc/kallsymsinstead?call.path: Thepathbuiltin function returns path with the form/dev/pts/4, but never returns the expected path of the temporary file created for the test. Is this a Kernel bug?call.strcontains: This test uses thepathbuiltin function and fails because of the issue above.Checklist
man/adoc/bpftrace.adocCHANGELOG.md