llvm: update remarks support on LLVM 22

[durin42]

LLVM change dfbd76bda01e removed separate remark support entirely, but
it turns out we can just drop the parameter and everything appears to
work fine.

Fixes #146912 as far as I can tell (the test passes.)

[rustbot]

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[durin42]

I've logged #146912 for the future work required on this. I didn't do enough digging today to figure out if the feature even makes sense with the changes in LLVM.

[cuviper]

Won't we need something like //@ max-llvm-major-version: 21 on the remarks-dir tests?

[nikic]

Given the additional information in #146912, we should not land this patch and instead make the minor necessary changes to support the change.

View changes since this review

[cuviper]

The upgrade remarks also mention release/finalization:

You can safely remove SerializerMode::Separate from createRemarkSerializer(). If you move the created Serializer into LLVMRemarkStreamer, you will have to call LLVMRemarkStreamer::releaseSerializer() once you're done emitting Remarks. Calling llvm::finalizeLLVMOptimizationRemarks() can do this for you.

I'm not sure off-hand where that should go...

[nikic]

If I understand correctly, we should get an assertion failure if we're using it incorrectly. Assuming the tests don't assert on LLVM 22, we should be fine.

[Prabhuk]

@nikic -- I do not fully understand the context here but our builders are broken and this patch could unblock us. What else that needs to happen here to land this change besides the concern about necessary finalization?

[durin42]

If I understand correctly, we should get an assertion failure if we're using it incorrectly. Assuming the tests don't assert on LLVM 22, we should be fine.

They don't assert as far as I can tell. Our buildkite has been happy with this patch applied.

[nikic]

If I understand correctly, we should get an assertion failure if we're using it incorrectly. Assuming the tests don't assert on LLVM 22, we should be fine.

They don't assert as far as I can tell. Our buildkite has been happy with this patch applied.

Is that with or without the rm -rf tests/run-make/optimization-remarks-dir buildkite is doing?

[durin42]

Shoot, I forgot we'd also done that. I just ran that test on my workstation with this PR applied at LLVM HEAD and it's passing without any assertion output that I see:

python3 x.py test tests/ui/zero-sized/zero-sized-btreemap-insert.rs
Building bootstrap
    Finished `dev` profile [unoptimized] target(s) in 0.14s
Building stage1 compiler artifacts (stage0 -> stage1, x86_64-unknown-linux-gnu)
    Finished `release` profile [optimized] target(s) in 0.40s
Creating a sysroot for stage1 compiler (use `rustup toolchain link 'name' build/host/stage1`)
Building stage1 lld-wrapper (stage0 -> stage1, x86_64-unknown-linux-gnu)
    Finished `release` profile [optimized] target(s) in 0.15s
Building stage1 library artifacts (stage1 -> stage1, x86_64-unknown-linux-gnu)
    Finished `release` profile [optimized] target(s) in 0.05s
Building stage1 compiletest (stage0 -> stage1, x86_64-unknown-linux-gnu)
    Finished `release` profile [optimized] target(s) in 0.20s
Building test helpers for x86_64-unknown-linux-gnu
Testing stage1 with compiletest suite=ui mode=ui (x86_64-unknown-linux-gnu)

running 1 tests
.

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 19817 filtered out; finished in 332.67ms

Build completed successfully in 0:00:02

[cuviper]

You need LLVM_ENABLE_ASSERTIONS=ON too, and this does fail for me:

rustc: /home/jistone/llvm-project/llvm/lib/Remarks/RemarkStreamer.cpp:38: llvm::remarks::RemarkStreamer::~RemarkStreamer(): Assertion `!RemarkSerializer && "RemarkSerializer must be released before RemarkStreamer is " "destroyed. Ensure llvm::finalizeOptimizationRemarks is called."' failed.

[nikic]

Okay, then we do need a call to finalizeOptimizationRemarks(). I think the destructor of RustDiagnosticHandler would work for that purpose?

[cuviper]

I added a destructor, and it fixes the assertion while passing our test.

r? nikic

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[nikic]

@bors r+ rollup

[bors]

📌 Commit fe440ec has been approved by nikic

It is now in the queue for this repository.