[zmodem]

View all comments

LLVM has been refactoring how it stores its internal (-mllvm) options recently, with the goal of getting rid of the global cl::opt objects.

After http://github.com/llvm/llvm-project/pull/233044 LTO passes no longer uses the global options, which would cause codegen-llvm/pgo-counter-bias.rs to fail since it relies on the -runtime-counter-relocation option to get passed to LTO.

Use the C API function LLVMParseCommandLineOptionsWithError which ensures the parsed options are
picked up by new LLVMContexts.

[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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @cuviper

[zmodem]

@rustbot label llvm-main

[zmodem]

r? @nikic

[nikic]

@MaskRay I'm curious how LibraryOptions is supposed to be adopted incrementally. Ideally, we would be parsing the options per-context -- but while not all options are per-context, the ones which are not would get parsed globally multiple times. That's not (thread) safe, right?

So does that mean that until the global options are entirely gone, we should basically keep using a global LibraryOptions, rather than a per-context (and thus per-thread) one? (That is, do what this PR does?)

[durin42]

I rebased this patch so the LLVM/Rust @Head CI can keep passing.

[nikic]

Now that llvm/llvm-project#234820 has landed, I wonder if some parts of this have become unnecessary?

I think things would mostly "just work" if we used the C API for option parsing?

View changes since this review

[zmodem]

I think things would mostly "just work" if we used the C API for option parsing?

Yes, I think so too. Let's close this.

[zmodem]

Hmm no, tests/codegen-llvm/pgo-counter-bias.rs still fails without this. I must have had the wrong branch checked out when I tested locally.

I'll start by rebasing this.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[zmodem]

Hmm no, with head llvm this fails because there's an extra

rustc -Cllvm-args="..." with: Did you mean '--nvvm-reflect-add'?

line in stderr. Not sure why that didn't happen with cl::parseCommandLineOptions.

(I did see this locally too, but I thought maybe my machine was unusual in having the nvptx backend enabled.)

[zmodem]

Oh, that's not the problem. It was failing because

error: ui test did not emit an error

cl::parseCommandLineOptions would exit(1) on error. We can do that too.

[nikic]

LGTM, can you please squash?

View changes since this review

[nikic]

Also the PR description could use an update.

[zmodem]

Updated the description.

[nikic]

@bors r+ rollup

[rust-bors]

📌 Commit 7b2a5cb has been approved by nikic

It is now in the queue for this repository.