Use new pass manager

[tamird]

Should we put this behind a flag?

---

This change is 

[ajwerner]

I'm fine with this. The memset rewriting works and makes sense, and seems like a good answer in the general case.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @alessandrod, @tamird, and @vadorovsky)

---

src/bin/bpf-linker.rs line 144 at r2 (raw file):

    /// Whether to treat LLVM errors as fatal.
    #[clap(long, action = clap::ArgAction::Set, default_value_t = true)]

Let's plumb through true in CI rather than changing the default here.

---

src/llvm/mod.rs line 249 at r3 (raw file):

    LLVMStripModuleDebugInfo(module);

    // Collect up all the memory intrinsics. We're going to replace these with

We've talked about this change, and I think it's fine, but it's worth noting that we're doing it explicitly because teaching the new pass manager about inlining thresholds is not possible directly. From rust-lang/rust#89742 it seems like the thresholds can be set via a flag when you initialize LLVM.

---

src/llvm/mod.rs line 227 at r5 (raw file):

    LLVMStripModuleDebugInfo(module);

    // Collect up all the memory intrinsics. We're going to replace these with

The below code section isn't well motivated with commentary, and it probably ought to be. Perhaps also add a debug log line to state what is going on here like the other steps in this function.

I continue to feel that this is a lot of inline code that does one thing as part of another function that does a whole bunch of things. A separated function would clarify the intent and give places to anchor commentary. This remains a nit so I defer to you as the implementer.

[tamird]

Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @ajwerner, @alessandrod, and @vadorovsky)

---

src/bin/bpf-linker.rs line 144 at r2 (raw file):

Previously, ajwerner wrote…

Let's plumb through true in CI rather than changing the default here.

Done.

---

src/llvm/mod.rs line 249 at r3 (raw file):

Previously, ajwerner wrote…

We've talked about this change, and I think it's fine, but it's worth noting that we're doing it explicitly because teaching the new pass manager about inlining thresholds is not possible directly. From rust-lang/rust#89742 it seems like the thresholds can be set via a flag when you initialize LLVM.

Isn't that issue about how those flags have no effect?

---

src/llvm/mod.rs line 227 at r5 (raw file):

Previously, ajwerner wrote…

The below code section isn't well motivated with commentary, and it probably ought to be. Perhaps also add a debug log line to state what is going on here like the other steps in this function.

I continue to feel that this is a lot of inline code that does one thing as part of another function that does a whole bunch of things. A separated function would clarify the intent and give places to anchor commentary. This remains a nit so I defer to you as the implementer.

I've put this whole thing in a block and anchored a comment on it.

[vadorovsky]

Hmm... I think that since Aya and bpf-linker are always relying on Rust nightly, and therefore the newest LLVM it uses, I think it would make sense to just always use the new pass manager? From my understanding, LLVM 17 already removes the old one from llvm-c, so as soon as Rust switches to it (might be soon - rust-lang/rust#114048), we won't be able to use the old one anyway.

[tamird]

Yeah, that's a good point (and the motivation for this change), but because symbols are resolved at runtime when rustc's LLVM is used, we'll still be able to build bpf-linker even if we make it a runtime flag. @alessandrod thoughts?

[alessandrod]

I think we either merge without a flag, but then give it enough time to bake (no new releases until people with large projects have had a chance to test), or we put it behind a flag and then if stuff breaks we can tell people to pin to a nightly with old PM.

If the old PM wasn't going away in rustc so soon I'd say definitely flag, but given that it's going away I don't really have a strong preference.

[tamird]

Sounds good. Should we change the fatal errors flag to default true and include instructions on how to disable it in the README? I'm weary of letting possible miscompilation continue to happen.

[alessandrod]

I'm ok with switching fatal errors on if we ignore the bogus builtin one and remove the manual intrinsic => call thing lowering

[tamird]

Why remove that? It's strictly more robust than string matching, and we'll remove it properly when the fix has gone in upstream.

[alessandrod]

Why remove that? It's strictly more robust than string matching, and we'll remove it properly when the fix has gone in upstream.

Because it's strictly more complex than just ignoring the error and it's conceptually wrong. The target emits that bogus error but otherwise is doing the right thing, so there's no need to do anything about the generated code. The problem is the error not the IR.

[tamird]

It's more complex, yes, but it will fail loudly if we do the wrong thing.

Why is it conceptually wrong? LLVM will generate the same code, we're just doing it earlier to avoid the error. What is conceptually wrong?

[alessandrod]

What is conceptually wrong?

The IR is valid, having intrinsics there is perfectly fine. This is standard LLVM behavior and the intrinsic => libcall lowering is a well understood and documented LLVM API. Again, the bug is the error.

Doing manual lowering of the code is like handwriting inline assembly to avoid an error from the code generator: sure it has the desired effect, but is not the simplest nor the most direct way to solve the problem.

[tamird]

I agree - and I'm working with upstream to do the proper thing. As a pragmatic solution, I am much more comfortable with rewriting IR than I am with string matching errors. Can you help me understand why this bothers you so much, relative to string matching on errors?

[alessandrod]

As a pragmatic solution, I am much more comfortable with rewriting IR than I am with string matching errors. Can you help me understand why this bothers you so much, relative to string matching on errors?

Because as a pragmatic solution, you can't possibly argue in good faith that adding all that unsafe LLVM code is better than a string match?

It's more code, it's unsafe code, it's wrappers around the LLVM C API which we know is not exactly great, it's calling largely undocumented functions, it requires a lot more knowledge to maintain going forward, there is literally not one advantage I can see in doing that over a string match.

[tamird]

I absolutely can. There's nothing really unsafe here - it's unsafe only because it uses ffi. Are you suggesting that there's unsound code here?

[alessandrod]

There's nothing really unsafe here - it's unsafe only because it uses ffi. Are you suggesting that there's unsound code here?

It's unsafe because it's written in debatable quality C (didn't you recently fix a LLVM-C bug that had effectively always been broken without nobody noticing?), that wraps very complex C++ code.

And quoting myself

It's more code, it's unsafe code, it's wrappers around the LLVM C API which we know is not exactly great, it's calling largely undocumented functions, it requires a lot more knowledge to maintain going forward, there is literally not one advantage I can see in doing that over a string match.

...and it requires an extra loop over all the instructions of all the functions.

I absolutely can.

Ok, what is your best argument in favor of doing the IR transformation? The string match is like at most 5 lines of 100% safe rust with no extra loops. What are the downsides you see with it?

[ajwerner]

Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @alessandrod, @tamird, and @vadorovsky)

---

src/llvm/mod.rs line 249 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Isn't that issue about how those flags have no effect?

The issue talks about how that that flag to rust no longer has any effect. The thread later points out that if the flag flows to LLVM (which it currently doesn't) then the old behavior gets recovered; a suggestion is to make the flag become equivalent to-C llvm-args=-inline-threshold=N.

[ajwerner]

Reviewed 1 of 8 files at r11, 1 of 5 files at r12, 2 of 5 files at r13, 1 of 4 files at r14, all commit messages.
Reviewable status: 5 of 8 files reviewed, 1 unresolved discussion (waiting on @alessandrod, @tamird, and @vadorovsky)

[tamird]

It's unsafe because it's written in debatable quality C (didn't you recently fix a LLVM-C bug that had effectively always been broken without nobody noticing?), that wraps very complex C++ code.

Yes, the LLVM C-API is of questionable quality to say the least. But I don't think that's what we're discussing here. If we're talking about the unsafe keyword, that's just caused by FFI, and there's plenty of it in this program. I don't think a "safe" interface to LLVM would protect us from latent bugs in LLVM. It's just a fact of life that LLVM's C-API is not widely used.

And quoting myself

It's more code, it's unsafe code, it's wrappers around the LLVM C API which we know is not exactly great, it's calling largely undocumented functions, it requires a lot more knowledge to maintain going forward, there is literally not one advantage I can see in doing that over a string match.

...and it requires an extra loop over all the instructions of all the functions.

It is possible to do this rewrite without this loop, would you like me to do that?

I absolutely can.

Ok, what is your best argument in favor of doing the IR transformation? The string match is like at most 5 lines of 100% safe rust with no extra loops. What are the downsides you see with it?

False negatives. String matching is inherently brittle, and I'm very worried about incorrectly matching a string that comes from a different error.

[tamird]

Reviewable status: 5 of 8 files reviewed, all discussions resolved (waiting on @ajwerner, @alessandrod, and @vadorovsky)

---

src/llvm/mod.rs line 249 at r3 (raw file):

Previously, ajwerner wrote…

The issue talks about how that that flag to rust no longer has any effect. The thread later points out that if the flag flows to LLVM (which it currently doesn't) then the old behavior gets recovered; a suggestion is to make the flag become equivalent to-C llvm-args=-inline-threshold=N.

I now understand what you're saying -- but this doesn't any problems being discussed here. We saw an issue with inlining (or absence of it) in the logging macros, but that was fixed by aya-rs/aya#683.

Done anyway.

[tamird]

Implemented string matching.