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

Target modifiers (special marked options) are recorded in metainfo #133138

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc_interface/src/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ fn configure_and_expand(

resolver.resolve_crate(&krate);

CStore::from_tcx(tcx).report_incompatible_target_modifiers(tcx, &krate, resolver.lint_buffer());
krate
}

Expand Down
10 changes: 10 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,16 @@ lint_improper_ctypes_union_layout_help = consider adding a `#[repr(C)]` or `#[re
lint_improper_ctypes_union_layout_reason = this union has unspecified layout
lint_improper_ctypes_union_non_exhaustive = this union is non-exhaustive

lint_incompatible_target_modifiers =
mixing `{$flag_name_suffixed}` will cause an ABI mismatch
.note1 = `{$flag_name_suffixed}`=`{$flag_local_value}` in crate `{$local_crate}`, `{$flag_name_suffixed}`=`{$flag_extern_value}` in crate `{$extern_crate}`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.note1 = `{$flag_name_suffixed}`=`{$flag_local_value}` in crate `{$local_crate}`, `{$flag_name_suffixed}`=`{$flag_extern_value}` in crate `{$extern_crate}`
.note1 = `{$flag_name_suffixed}={$flag_local_value}` in this crate is incompatible with `{$flag_name_suffixed}={$flag_extern_value}` in dependency `{$extern_crate}`

Likewise with the other comment, -Zregparm= rather than regparm is better, this should look as similar to what the user will have passed to the compiler as possible.

.help = This error occurs because the `{$flag_name_suffixed}` flag modifies the ABI,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this should say something like:

Suggested change
.help = This error occurs because the `{$flag_name_suffixed}` flag modifies the ABI,
.help = `{$flag_name_suffixed}` modifies the ABI and Rust crates compiled with different values of this flag cannot be used together safely

It would be good if we could provide the whole option, not just the name, so -Zregparm rather than regparm.

and different crates in your project were compiled with inconsistent settings
.suggestion = To resolve this, ensure that `{$flag_name_suffixed}` is set to the same value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.suggestion = To resolve this, ensure that `{$flag_name_suffixed}` is set to the same value
.suggestion = set `{$flag_name_suffixed}=${flag_extern_value}` in this crate or `{$flag_name_suffixed}=${flag_local_value}` in `{$extern_crate}`

for all crates during compilation
.note2 = To ignore this error, recompile with the following flag:
-Cunsafe-allow-abi-mismatch=`{$flag_name}`
Comment on lines +425 to +426
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.note2 = To ignore this error, recompile with the following flag:
-Cunsafe-allow-abi-mismatch=`{$flag_name}`
.note2 = alternatively, use `-Cunsafe-allow-abi-mismatch={$flag_name}` to silence this error


lint_incomplete_include =
include macro expected single expression in source

Expand Down
16 changes: 16 additions & 0 deletions compiler/rustc_lint/src/context/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,22 @@ pub(super) fn decorate_lint(sess: &Session, diagnostic: BuiltinLintDiag, diag: &
lints::UnusedCrateDependency { extern_crate, local_crate }.decorate_lint(diag)
}
BuiltinLintDiag::WasmCAbi => lints::WasmCAbi.decorate_lint(diag),
BuiltinLintDiag::IncompatibleTargetModifiers {
extern_crate,
local_crate,
flag_name,
flag_name_suffixed,
flag_local_value,
flag_extern_value,
} => lints::IncompatibleTargetModifiers {
extern_crate,
local_crate,
flag_name,
flag_name_suffixed,
flag_local_value,
flag_extern_value,
}
.decorate_lint(diag),
BuiltinLintDiag::IllFormedAttributeInput { suggestions } => {
lints::IllFormedAttributeInput {
num_suggestions: suggestions.len(),
Expand Down
14 changes: 14 additions & 0 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2470,6 +2470,20 @@ pub(crate) struct UnusedCrateDependency {
pub local_crate: Symbol,
}

#[derive(LintDiagnostic)]
#[diag(lint_incompatible_target_modifiers)]
#[help]
#[note(lint_note1)]
#[note(lint_note2)]
pub(crate) struct IncompatibleTargetModifiers {
pub extern_crate: Symbol,
pub local_crate: Symbol,
pub flag_name: String,
pub flag_name_suffixed: String,
pub flag_local_value: String,
pub flag_extern_value: String,
}

#[derive(LintDiagnostic)]
#[diag(lint_wasm_c_abi)]
pub(crate) struct WasmCAbi;
Expand Down
38 changes: 38 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ declare_lint_pass! {
FUZZY_PROVENANCE_CASTS,
HIDDEN_GLOB_REEXPORTS,
ILL_FORMED_ATTRIBUTE_INPUT,
INCOMPATIBLE_TARGET_MODIFIERS,
INCOMPLETE_INCLUDE,
INEFFECTIVE_UNSTABLE_TRAIT_IMPL,
INLINE_NO_SANITIZE,
Expand Down Expand Up @@ -578,6 +579,43 @@ declare_lint! {
crate_level_only
}

declare_lint! {
/// The `incompatible_target_modifiers` lint detects crates with incompatible target modifiers
/// (abi-changing or vulnerability-affecting flags).
///
/// ### Example
///
/// ```rust,ignore (needs extern crate)
/// #![deny(incompatible_target_modifiers)]
/// ```
///
/// When main and dependency crates are compiled with `-Zregparm=1` and `-Zregparm=2` correspondingly.
///
/// This will produce:
///
/// ```text
/// error: crate `incompatible_regparm` has incompatible target modifier with extern crate `wrong_regparm`: `regparm = ( Some(1) | Some(2) )`
/// --> $DIR/incompatible_regparm.rs:5:1
/// |
/// 1 | #![no_core]
/// | ^
/// |
/// = note: `#[deny(incompatible_target_modifiers)]` on by default
/// ```
///
/// ### Explanation
///
/// `Target modifiers` are compilation flags that affects abi or vulnerability resistance.
/// Linking together crates with incompatible target modifiers would produce incorrect code
/// or degradation of vulnerability resistance.
/// So this lint should find such inconsistency.
///
pub INCOMPATIBLE_TARGET_MODIFIERS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a lint?

Deny,
"Incompatible target modifiers",
crate_level_only
}

declare_lint! {
/// The `unused_qualifications` lint detects unnecessarily qualified
/// names.
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_lint_defs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,14 @@ pub enum BuiltinLintDiag {
AvoidUsingIntelSyntax,
AvoidUsingAttSyntax,
IncompleteInclude,
IncompatibleTargetModifiers {
extern_crate: Symbol,
local_crate: Symbol,
flag_name: String,
flag_name_suffixed: String,
flag_local_value: String,
flag_extern_value: String,
},
UnnameableTestItems,
DuplicateMacroAttribute,
CfgAttrNoAttributes,
Expand Down
115 changes: 113 additions & 2 deletions compiler/rustc_metadata/src/creader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use rustc_middle::bug;
use rustc_middle::ty::{TyCtxt, TyCtxtFeed};
use rustc_session::config::{self, CrateType, ExternLocation};
use rustc_session::cstore::{CrateDepKind, CrateSource, ExternCrate, ExternCrateSource};
use rustc_session::lint::{self, BuiltinLintDiag};
use rustc_session::lint::{self, BuiltinLintDiag, LintBuffer};
use rustc_session::output::validate_crate_name;
use rustc_session::search_paths::PathKind;
use rustc_span::edition::Edition;
Expand All @@ -35,7 +35,9 @@ use tracing::{debug, info, trace};

use crate::errors;
use crate::locator::{CrateError, CrateLocator, CratePaths};
use crate::rmeta::{CrateDep, CrateMetadata, CrateNumMap, CrateRoot, MetadataBlob};
use crate::rmeta::{
CrateDep, CrateMetadata, CrateNumMap, CrateRoot, MetadataBlob, TargetModifiers,
};

/// The backend's way to give the crate store access to the metadata in a library.
/// Note that it returns the raw metadata bytes stored in the library file, whether
Expand Down Expand Up @@ -290,6 +292,94 @@ impl CStore {
}
}

pub fn report_incompatible_target_modifiers(
&self,
tcx: TyCtxt<'_>,
krate: &Crate,
lints: &mut LintBuffer,
) {
if tcx.crate_types().contains(&CrateType::ProcMacro) {
return;
}
let sess = tcx.sess;
if let Some(vec) = &sess.opts.cg.unsafe_allow_abi_mismatch
&& vec.is_empty()
{
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to allow users to opt-out of all mismatches?

}
Comment on lines +305 to +309
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that you check access unsafe_allow_abi_mismatch later, maybe do something like this:

Suggested change
if let Some(vec) = &sess.opts.cg.unsafe_allow_abi_mismatch
&& vec.is_empty()
{
return;
}
let allowed_flag_mismatches = sets.opts.cg.unsafe_allow_abi_mismatch.as_ref().unwrap_or_default();
if allowed_flag_mismatches.is_empty() {
// Setting `-Zunsafe-allow-abi-mismatch=` to an empty
// value allows all target modifier mismatches.
return;
}

..and then use allowed_flag_mismatches after this.

(that suggestion might not compile exactly, but you should be able to do something like it)

let span = krate.spans.inner_span.shrink_to_lo();

let splitter = |v: &String| {
let splitted: Vec<_> = v.split("=").collect();
(splitted[0].to_string(), splitted[1].to_string())
};
let name = tcx.crate_name(LOCAL_CRATE);
let mods = sess.opts.gather_target_modifiers();
for (_cnum, data) in self.iter_crate_data() {
if data.is_proc_macro_crate() {
continue;
}
let mut report_diff =
|flag_name: &String, flag_local_value: &String, flag_extern_value: &String| {
if let Some(vec) = &sess.opts.cg.unsafe_allow_abi_mismatch {
if vec.contains(flag_name) {
return;
}
}
lints.buffer_lint(
lint::builtin::INCOMPATIBLE_TARGET_MODIFIERS,
ast::CRATE_NODE_ID,
span,
BuiltinLintDiag::IncompatibleTargetModifiers {
extern_crate: data.name(),
local_crate: name,
flag_name: flag_name.to_string(),
flag_name_suffixed: flag_name.to_string(),
flag_local_value: flag_local_value.to_string(),
flag_extern_value: flag_extern_value.to_string(),
},
);
};
let mut it1 = mods.iter().map(splitter);
let mut it2 = data.target_modifiers().map(splitter);
let mut left_name_val: Option<(String, String)> = None;
let mut right_name_val: Option<(String, String)> = None;
let no_val = "*".to_string();
loop {
left_name_val = left_name_val.or_else(|| it1.next());
right_name_val = right_name_val.or_else(|| it2.next());
match (&left_name_val, &right_name_val) {
(Some(l), Some(r)) => match l.0.cmp(&r.0) {
cmp::Ordering::Equal => {
if l.1 != r.1 {
report_diff(&l.0, &l.1, &r.1);
}
left_name_val = None;
right_name_val = None;
}
cmp::Ordering::Greater => {
report_diff(&r.0, &no_val, &r.1);
right_name_val = None;
}
cmp::Ordering::Less => {
report_diff(&l.0, &l.1, &no_val);
left_name_val = None;
}
},
(Some(l), None) => {
report_diff(&l.0, &l.1, &no_val);
left_name_val = None;
}
(None, Some(r)) => {
report_diff(&r.0, &no_val, &r.1);
right_name_val = None;
}
(None, None) => break,
}
}
}
}

pub fn new(metadata_loader: Box<MetadataLoaderDyn>) -> CStore {
CStore {
metadata_loader,
Expand Down Expand Up @@ -432,6 +522,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
};

let cnum_map = self.resolve_crate_deps(root, &crate_root, &metadata, cnum, dep_kind)?;
let target_modifiers = self.resolve_target_modifiers(&crate_root, &metadata, cnum)?;

let raw_proc_macros = if crate_root.is_proc_macro_crate() {
let temp_root;
Expand All @@ -456,6 +547,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
raw_proc_macros,
cnum,
cnum_map,
target_modifiers,
dep_kind,
source,
private_dep,
Expand Down Expand Up @@ -689,6 +781,25 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
Ok(crate_num_map)
}

fn resolve_target_modifiers(
&mut self,
crate_root: &CrateRoot,
metadata: &MetadataBlob,
krate: CrateNum,
) -> Result<TargetModifiers, CrateError> {
debug!("resolving target modifiers of external crate");
if crate_root.is_proc_macro_crate() {
return Ok(TargetModifiers::new());
}
let mods = crate_root.decode_target_modifiers(metadata);
let mut target_modifiers = TargetModifiers::with_capacity(mods.len());
for modifier in mods {
target_modifiers.push(modifier);
}
debug!("resolve_target_modifiers: target mods for {:?} is {:?}", krate, target_modifiers);
Ok(target_modifiers)
}

fn dlsym_proc_macros(
&self,
path: &Path,
Expand Down
18 changes: 18 additions & 0 deletions compiler/rustc_metadata/src/rmeta/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ impl MetadataBlob {
/// own crate numbers.
pub(crate) type CrateNumMap = IndexVec<CrateNum, CrateNum>;

/// Target modifiers - abi / vulnerability-resist affecting flags
pub(crate) type TargetModifiers = Vec<String>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a (String, String) and we always keep the value separated from the flag?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to have a bitset or enum or something that identifies the target modifiers and then put those in this, (TargetModifier, String), so that we can avoid storing so many strings into the crate metadata (which can have a perf impact).


pub(crate) struct CrateMetadata {
/// The primary crate data - binary metadata blob.
blob: MetadataBlob,
Expand Down Expand Up @@ -110,6 +113,8 @@ pub(crate) struct CrateMetadata {
cnum_map: CrateNumMap,
/// Same ID set as `cnum_map` plus maybe some injected crates like panic runtime.
dependencies: Vec<CrateNum>,
/// Target modifiers - abi and vulnerability-resist affecting flags the crate was compiled with
target_modifiers: TargetModifiers,
/// How to link (or not link) this crate to the currently compiled crate.
dep_kind: CrateDepKind,
/// Filesystem location of this crate.
Expand Down Expand Up @@ -960,6 +965,13 @@ impl CrateRoot {
) -> impl ExactSizeIterator<Item = CrateDep> + Captures<'a> {
self.crate_deps.decode(metadata)
}

pub(crate) fn decode_target_modifiers<'a>(
&self,
metadata: &'a MetadataBlob,
) -> impl ExactSizeIterator<Item = String> + Captures<'a> {
self.target_modifiers.decode(metadata)
}
}

impl<'a> CrateMetadataRef<'a> {
Expand Down Expand Up @@ -1815,6 +1827,7 @@ impl CrateMetadata {
raw_proc_macros: Option<&'static [ProcMacro]>,
cnum: CrateNum,
cnum_map: CrateNumMap,
target_modifiers: TargetModifiers,
dep_kind: CrateDepKind,
source: CrateSource,
private_dep: bool,
Expand Down Expand Up @@ -1846,6 +1859,7 @@ impl CrateMetadata {
cnum,
cnum_map,
dependencies,
target_modifiers,
dep_kind,
source: Lrc::new(source),
private_dep,
Expand Down Expand Up @@ -1875,6 +1889,10 @@ impl CrateMetadata {
self.dependencies.push(cnum);
}

pub(crate) fn target_modifiers(&self) -> impl Iterator<Item = &String> + '_ {
self.target_modifiers.iter()
}

pub(crate) fn update_extern_crate(&mut self, new_extern_crate: ExternCrate) -> bool {
let update =
Some(new_extern_crate.rank()) > self.extern_crate.as_ref().map(ExternCrate::rank);
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_metadata/src/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,7 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
// Encode source_map. This needs to be done last, because encoding `Span`s tells us which
// `SourceFiles` we actually need to encode.
let source_map = stat!("source-map", || self.encode_source_map());
let target_modifiers = stat!("target-modifiers", || self.encode_target_modifiers());

let root = stat!("final", || {
let attrs = tcx.hir().krate_attrs();
Expand Down Expand Up @@ -732,6 +733,7 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
native_libraries,
foreign_modules,
source_map,
target_modifiers,
traits,
impls,
incoherent_impls,
Expand Down Expand Up @@ -1978,6 +1980,12 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
self.lazy_array(deps.iter().map(|(_, dep)| dep))
}

fn encode_target_modifiers(&mut self) -> LazyArray<String> {
empty_proc_macro!(self);
let tcx = self.tcx;
self.lazy_array(tcx.sess.opts.gather_target_modifiers())
}

fn encode_lib_features(&mut self) -> LazyArray<(Symbol, FeatureStability)> {
empty_proc_macro!(self);
let tcx = self.tcx;
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_metadata/src/rmeta/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::marker::PhantomData;
use std::num::NonZero;

pub(crate) use decoder::{CrateMetadata, CrateNumMap, MetadataBlob};
pub(crate) use decoder::{CrateMetadata, CrateNumMap, MetadataBlob, TargetModifiers};
use decoder::{DecodeContext, Metadata};
use def_path_hash_map::DefPathHashMapRef;
use encoder::EncodeContext;
Expand Down Expand Up @@ -283,6 +283,7 @@ pub(crate) struct CrateRoot {
def_path_hash_map: LazyValue<DefPathHashMapRef<'static>>,

source_map: LazyTable<u32, Option<LazyValue<rustc_span::SourceFile>>>,
target_modifiers: LazyArray<String>,

compiler_builtins: bool,
needs_allocator: bool,
Expand Down
Loading
Loading