Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Session::crate_types_opt? #72427

Closed
ecstatic-morse opened this issue May 21, 2020 · 4 comments · Fixed by #72435
Closed

Remove Session::crate_types_opt? #72427

ecstatic-morse opened this issue May 21, 2020 · 4 comments · Fixed by #72435
Assignees
Labels
A-codegen Area: Code generation C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ecstatic-morse
Copy link
Contributor

See #72256 (comment) for context.

There's only one place in the compiler that can't call Session::crate_types because that field is sometimes uninitialized.

/// PIE is potentially more effective than PIC, but can only be used in executables.
/// If all our outputs are executables, then we can relax PIC to PIE when producing object code.
/// If the list of crate types is not yet known we conservatively return `false`.
pub fn all_outputs_are_pic_executables(sess: &Session) -> bool {

Perhaps this indicates that we want to force initialization of crate_types somewhere?

@ecstatic-morse ecstatic-morse added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 21, 2020
@petrochenkov
Copy link
Contributor

all_outputs_are_pic_executables is used during creation of LLVM TargetMachine.
Sometimes we create an "informational" target machine to query supported LLVM target features, for example.
We don't need to force initialize crate_types in this case because the queried information doesn't depend on them, but we also can't do it in some cases because initialization of crate_types requires running parsing and expansion, which would create cycles because the LLVM target feature list is an input for expansion.

@Mark-Simulacrum
Copy link
Member

Hm, it sounds like maybe we should not attempt a read of the crate types of we're creating it for that informational use case?

I presume the crate type can influence LLVM in some way, right?

@petrochenkov
Copy link
Contributor

Ha, the PositionIndependentExecutable parameter in LLVMRustCreateTargetMachine is not even used.
We need to enable more warnings in C++ code.

So it should be possible to remove crate_types_opt from all_outputs_are_pic_executables, I'll prepare a PR now.

@petrochenkov
Copy link
Contributor

Fixed in #72435.

@bors bors closed this as completed in e7503ca May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants