Add di-sanitizer feature, allowing to disable DI sanitization#320
Add di-sanitizer feature, allowing to disable DI sanitization#320vadorovsky wants to merge 5 commits intoaya-rs:mainfrom
di-sanitizer feature, allowing to disable DI sanitization#320Conversation
0ad0afb to
18fda8c
Compare
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 2 of 3 files at r1, 7 of 7 files at r2, 9 of 14 files at r3, all commit messages.
Reviewable status: 13 of 18 files reviewed, 1 unresolved discussion
-- commits line 9 at r1:
are you going to send that patch? 🍿
tests/sanitizer/btf/assembly/auxiliary/loop-panic-handler.rs line 1 at r3 (raw file):
// no-prefer-dynamic
can these various files that are copies be symlinks instead?
18fda8c to
da10a1e
Compare
|
Something fishy is going on with apt packages. The job that uses LLVM 21.1.5 from packages: https://github.com/aya-rs/bpf-linker/actions/runs/19110300914/job/54605380603?pr=320 fails with: Given that the "source" job uses our fork https://github.com/aya-rs/llvm-project, which currently is LLVM 21.1.3 with my patches applied, I though there is some regression in 21.1.5, but... I'm not able to reproduce that with LLVM built from source from the upstream tag https://github.com/llvm/llvm-project/releases/tag/llvmorg-21.1.5. And the BTF of I was thinking about proposing an upgrade to LLVM 21.1.5 to the Rust fork of LLVM https://github.com/rust-lang/llvm-project/tree/rustc/21.1-2025-08-01, but I will do that after solving that mystery. |
|
OK, mystery solved. The current "21.1.5" deb packages from apt.llvm.org are not made from the actual 21.1.5 release (that was cut 2 days ago), but from some git snapshot of the https://apt.llvm.org/jammy/pool/main/l/llvm-toolchain-21/ 🤡 I will re-enable fixups for the apt variant and remove them once packages are fresh enough to actually contain my fixes. |
|
Jeez that's frustrating behavior from the LLVM apt maintainers. |
3d8c710 to
2423a99
Compare
vadorovsky
left a comment
There was a problem hiding this comment.
Reviewable status: 4 of 19 files reviewed, 1 unresolved discussion (waiting on @tamird)
Previously, tamird (Tamir Duberstein) wrote…
are you going to send that patch? 🍿
Yes, probably some time this week. 😅
tests/sanitizer/btf/assembly/auxiliary/loop-panic-handler.rs line 1 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can these various files that are copies be symlinks instead?
Yes, done
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 14 of 14 files at r4, 14 of 14 files at r6, 1 of 1 files at r7, 14 of 14 files at r8, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @vadorovsky)
It was never necessary in the first place. There is no place in the kernel that enforces BTF map structs themselves to be anonymous. Only pointer types (that are members of BTF maps) have to be anonymous, but that's handled by a separate fixup. BTF maps work just fine without it.
`assembly/auxiliary/loop-panic-handler.rs` and `btf/assembly/auxiliary/loop-panic-handler.rs` had exactly the same content. Instead of keeping copies, make a symlink.
LLVM >=21.1.5 supports them thanks to llvm/llvm-project#155783.
Starting from LLVM 21.1.5, the only necessary fixup is the making sure that the type names are C-compatible and don't contain characters like `<`, `>`. That should be fixed in the kernel. Phasing out this behavior will take time and effort, but ability to disable fixups will make the development efforts easier.
2423a99 to
2ed8455
Compare
tamird
left a comment
There was a problem hiding this comment.
@tamird partially reviewed 18 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @vadorovsky).
src/llvm/di.rs line 89 at r12 (raw file):
} // BPF backend in LLVM <21.1.5 does not support Rust enums.
drop a link to the commit? llvm/llvm-project@ad582e3
.github/workflows/ci.yml line 301 at r13 (raw file):
if: runner.os == 'Linux' && matrix.toolchain.rust == 'nightly' with: # FIXME: Switch to upstream after https://github.com/aya-rs/aya/pull/1382
can you add to this comment explaining that that PR sanitizes the Rust names and that without either that or DI sanitization here in bpf-linker tests won't pass?
Using any of the available LLVM builds is not satisfactionary for us. apt.llvm.org is constantly broken (either does not work or lies about versions shipped by the packages[0]) and does not always catch up with the latest release candidates. Rust CI makes assumptions about which type of linking is used for each platform. It ships only static libraries for `*-musl` targets, and only dynamic ones for `*-apple-darwin` and `*-gnu` targets. We would like to use both types of linking for `*-musl` targets. We prefer dynamic linking for initial rounds of tests (especially the ones using `cargo hack`) to save space, but we also prefer static linking for producing the final binaries. We need to chase LLVM versions used in Rust nightly, which include release candidates as soon as they are out, so packaging provided by Linux distributions is moving too slow for us. Furthermore, building LLVM from source allows us to enable assertions for all LLVM versions, which makes it easier to find regressions. We are in favor of keeping them enabled even for the end users. [0] aya-rs#320 (comment)
Using any of the available LLVM builds is not satisfactionary for us. apt.llvm.org is constantly broken (either does not work or lies about versions shipped by the packages[0]) and does not always catch up with the latest release candidates. Rust CI makes assumptions about which type of linking is used for each platform. It ships only static libraries for `*-musl` targets, and only dynamic ones for `*-apple-darwin` and `*-gnu` targets. We would like to use both types of linking for `*-musl` targets. We prefer dynamic linking for initial rounds of tests (especially the ones using `cargo hack`) to save space, but we also prefer static linking for producing the final binaries. We need to chase LLVM versions used in Rust nightly, which include release candidates as soon as they are out, so packaging provided by Linux distributions is moving too slow for us. Furthermore, building LLVM from source allows us to enable assertions for all LLVM versions, which makes it easier to find regressions. We are in favor of keeping them enabled even for the end users. [0] aya-rs#320 (comment)
Using any of the available LLVM builds is not satisfactionary for us. apt.llvm.org is constantly broken (either does not work or lies about versions shipped by the packages[0]) and does not always catch up with the latest release candidates. Rust CI makes assumptions about which type of linking is used for each platform. It ships only static libraries for `*-musl` targets, and only dynamic ones for `*-apple-darwin` and `*-gnu` targets. We would like to use both types of linking for `*-musl` targets. We prefer dynamic linking for initial rounds of tests (especially the ones using `cargo hack`) to save space, but we also prefer static linking for producing the final binaries. We need to chase LLVM versions used in Rust nightly, which include release candidates as soon as they are out, so packaging provided by Linux distributions is moving too slow for us. Furthermore, building LLVM from source allows us to enable assertions for all LLVM versions, which makes it easier to find regressions. We are in favor of keeping them enabled even for the end users. [0] aya-rs#320 (comment)
LLVM 21.1.5 contains fixes that emit correct BTF[0] without the need to sanitize it, except from the name fixup that:
Introduce the
di-sanitizerfeature, that explicitly activates the use ofDISanitizer. Enable it by default for now, but run Aya integration tests without it.[0] llvm/llvm-project#165154
This change is