Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
40 changes: 22 additions & 18 deletions compiler/rustc_codegen_llvm/src/back/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@ use rustc_codegen_ssa::back::write::{
TargetMachineFactoryFn,
};
use rustc_codegen_ssa::base::wants_wasm_eh;
use rustc_codegen_ssa::debuginfo::command_line_args::quote_command_line_args;
use rustc_codegen_ssa::traits::*;
use rustc_codegen_ssa::{CompiledModule, ModuleCodegen, ModuleKind};
use rustc_data_structures::profiling::SelfProfilerRef;
use rustc_data_structures::small_c_str::SmallCStr;
use rustc_errors::{DiagCtxtHandle, Level};
use rustc_fs_util::{link_or_copy, path_to_c_string};
use rustc_middle::middle::debuginfo::CommandLineArgsForDebuginfo;
use rustc_middle::ty::TyCtxt;
use rustc_session::Session;
use rustc_session::config::{
Expand Down Expand Up @@ -108,7 +108,11 @@ pub(crate) fn create_informational_target_machine(
sess: &Session,
only_base_features: bool,
) -> OwnedTargetMachine {
let config = TargetMachineFactoryConfig { split_dwarf_file: None, output_obj_file: None };
let config = TargetMachineFactoryConfig {
split_dwarf_file: None,
output_obj_file: None,
args_for_debuginfo: None,
};
// Can't use query system here quite yet because this function is invoked before the query
// system/tcx is set up.
let features = llvm_util::global_llvm_features(sess, false, only_base_features);
Expand All @@ -133,7 +137,12 @@ pub(crate) fn create_target_machine(tcx: TyCtxt<'_>, mod_name: &str) -> OwnedTar
mod_name,
tcx.sess.invocation_temp.as_deref(),
));
let config = TargetMachineFactoryConfig { split_dwarf_file, output_obj_file };

let config = TargetMachineFactoryConfig {
split_dwarf_file,
output_obj_file,
args_for_debuginfo: Some(Arc::clone(tcx.args_for_debuginfo())),
};

target_machine_factory(
tcx.sess,
Expand Down Expand Up @@ -253,19 +262,6 @@ pub(crate) fn target_machine_factory(

let use_emulated_tls = matches!(sess.tls_model(), TlsModel::Emulated);

// Command-line information to be included in the target machine.
// This seems to only be used for embedding in PDB debuginfo files.
// FIXME(Zalathar): Maybe skip this for non-PDB targets?
let argv0 = std::env::current_exe()
.unwrap_or_default()
.into_os_string()
.into_string()
.unwrap_or_default();
let command_line_args = quote_command_line_args(&sess.expanded_args);
// Self-profile counter for the number of bytes produced by command-line quoting.
// Values are summed, so the summary result is cumulative across all TM factories.
sess.prof.artifact_size("quoted_command_line_args", "-", command_line_args.len() as u64);

let debuginfo_compression = sess.opts.debuginfo_compression.to_string();
match sess.opts.debuginfo_compression {
rustc_session::config::DebugInfoCompression::Zlib => {
Expand Down Expand Up @@ -304,6 +300,14 @@ pub(crate) fn target_machine_factory(
let split_dwarf_file = path_to_cstring_helper(config.split_dwarf_file);
let output_obj_file = path_to_cstring_helper(config.output_obj_file);

let (argv0, quoted_args) = config
.args_for_debuginfo
.as_deref()
.map(|CommandLineArgsForDebuginfo { argv0, quoted_args }| {
(argv0.as_str(), quoted_args.as_str())
})
.unwrap_or_default();

OwnedTargetMachine::new(
&triple,
&cpu,
Expand All @@ -326,8 +330,8 @@ pub(crate) fn target_machine_factory(
&output_obj_file,
&debuginfo_compression,
use_emulated_tls,
&argv0,
&command_line_args,
argv0,
quoted_args,
use_wasm_eh,
)
})
Expand Down
16 changes: 13 additions & 3 deletions compiler/rustc_codegen_ssa/src/back/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use rustc_incremental::{
use rustc_metadata::fs::copy_to_stdout;
use rustc_middle::bug;
use rustc_middle::dep_graph::{WorkProduct, WorkProductId};
use rustc_middle::middle::debuginfo::CommandLineArgsForDebuginfo;
use rustc_middle::ty::TyCtxt;
use rustc_session::Session;
use rustc_session::config::{
Expand Down Expand Up @@ -283,6 +284,10 @@ pub struct TargetMachineFactoryConfig {
/// The name of the output object file. Used for setting OutputFilenames in target options
/// so that LLVM can emit the CodeView S_OBJNAME record in pdb files
pub output_obj_file: Option<PathBuf>,

/// Contains a copy of [`TyCtxt::args_for_debuginfo`] for codegen,
/// or `None` if the target machine is informational only.
pub args_for_debuginfo: Option<Arc<CommandLineArgsForDebuginfo>>,
}

impl TargetMachineFactoryConfig {
Expand All @@ -306,7 +311,12 @@ impl TargetMachineFactoryConfig {
module_name,
cgcx.invocation_temp.as_deref(),
));
TargetMachineFactoryConfig { split_dwarf_file, output_obj_file }

TargetMachineFactoryConfig {
split_dwarf_file,
output_obj_file,
args_for_debuginfo: Some(Arc::clone(&cgcx.args_for_debuginfo)),
}
}
}

Expand Down Expand Up @@ -350,7 +360,7 @@ pub struct CodegenContext<B: WriteBackendMethods> {
/// This will only be used within debug info, e.g. in the pdb file on windows
/// This is mainly useful for other tools that reads that debuginfo to figure out
/// how to call the compiler with the same arguments.
pub expanded_args: Vec<String>,
pub args_for_debuginfo: Arc<CommandLineArgsForDebuginfo>,

/// Emitter to use for diagnostics produced during codegen.
pub diag_emitter: SharedEmitter,
Expand Down Expand Up @@ -1153,7 +1163,7 @@ fn start_executing_work<B: ExtraBackendMethods>(
remark: sess.opts.cg.remark.clone(),
remark_dir,
incr_comp_session_dir: sess.incr_comp_session_dir_opt().map(|r| r.clone()),
expanded_args: tcx.sess.expanded_args.clone(),
args_for_debuginfo: Arc::clone(tcx.args_for_debuginfo()),
diag_emitter: shared_emitter.clone(),
output_filenames: Arc::clone(tcx.output_filenames(())),
module_config: regular_config,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,42 @@
use std::sync::Arc;

use rustc_middle::middle::debuginfo::CommandLineArgsForDebuginfo;
use rustc_middle::ty::TyCtxt;
use rustc_middle::util::Providers;

#[cfg(test)]
mod tests;

pub(crate) fn provide(providers: &mut Providers) {
providers.hooks.args_for_debuginfo = args_for_debuginfo;
}

/// Hook implementation for [`TyCtxt::args_for_debuginfo`].
fn args_for_debuginfo<'tcx>(tcx: TyCtxt<'tcx>) -> &'tcx Arc<CommandLineArgsForDebuginfo> {
tcx.args_for_debuginfo_cache.get_or_init(|| {
// Command-line information to be included in the target machine.
// This seems to only be used for embedding in PDB debuginfo files.
// FIXME(Zalathar): Maybe skip this for non-PDB targets?
let argv0 = std::env::current_exe()
.unwrap_or_default()
.into_os_string()
.into_string()
.unwrap_or_default();
let quoted_args = quote_command_line_args(&tcx.sess.expanded_args);

// Self-profile counter for the number of bytes produced by command-line quoting.
tcx.prof.artifact_size("quoted_command_line_args", "-", quoted_args.len() as u64);

Arc::new(CommandLineArgsForDebuginfo { argv0, quoted_args })
})
}

/// Joins command-line arguments into a single space-separated string, quoting
/// and escaping individual arguments as necessary.
///
/// The result is intended to be informational, for embedding in debug metadata,
/// and might not be properly quoted/escaped for actual command-line use.
pub fn quote_command_line_args(args: &[String]) -> String {
fn quote_command_line_args(args: &[String]) -> String {
// Start with a decent-sized buffer, since rustc invocations tend to be long.
let mut buf = String::with_capacity(128);

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/debuginfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use rustc_middle::bug;
use rustc_middle::ty::layout::{IntegerExt, PrimitiveExt, TyAndLayout};
use rustc_middle::ty::{self, Ty, TyCtxt};

pub mod command_line_args;
pub(crate) mod command_line_args;
pub mod type_names;

/// Returns true if we want to generate a DW_TAG_enumeration_type description for
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_codegen_ssa/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ pub enum CodegenErrors {
pub fn provide(providers: &mut Providers) {
crate::back::symbol_export::provide(providers);
crate::base::provide(providers);
crate::debuginfo::command_line_args::provide(providers);
crate::target_features::provide(providers);
crate::codegen_attrs::provide(providers);
providers.queries.global_backend_features = |_tcx: TyCtxt<'_>, ()| vec![];
Expand Down
10 changes: 10 additions & 0 deletions compiler/rustc_middle/src/hooks/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
//! similar to queries, but queries come with a lot of machinery for caching and incremental
//! compilation, whereas hooks are just plain function pointers without any of the query magic.

use std::sync::Arc;

use rustc_hir::def_id::{DefId, DefPathHash};
use rustc_session::StableCrateId;
use rustc_span::def_id::{CrateNum, LocalDefId};
use rustc_span::{ExpnHash, ExpnId};

use crate::middle::debuginfo::CommandLineArgsForDebuginfo;
use crate::mir;
use crate::ty::{Ty, TyCtxt};

Expand Down Expand Up @@ -102,6 +105,13 @@ declare_hooks! {
/// Ensure the given scalar is valid for the given type.
/// This checks non-recursive runtime validity.
hook validate_scalar_in_layout(scalar: crate::ty::ScalarInt, ty: Ty<'tcx>) -> bool;

/// Builds a quoted form of _all_ command-line args, for inclusion in some
/// forms of debuginfo (specifically PDB).
///
/// This needs to _not_ be a query, because it would ruin incremental
/// compilation for CGUs that would otherwise be considered up-to-date.
Copy link
Member

Choose a reason for hiding this comment

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

If the commandline changed, then we should recompile all CGUs as otherwise the commandline arguments in the debuginfo would be wrong. If it doesn't change, then no CGUs would be recompiled even if it was a query. If unconditionally recompiling cgus when the cli changes is not acceptable, then maybe this entire feature should be put behind a cli flag and be disabled by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, bypassing the query system is already the current behaviour. So if that's a problem, maybe the whole “command-line in PDB” thing needs to be ripped out until it can be re-landed in an acceptable way.

(I don't have any attachment to the feature myself; I'm just trying to make the compiler do this quoting step once per process instead of literally 500+ times for no good reason.)

Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, bypassing the query system is already the current behaviour.

Yeah, I don't think anyone considered this when landing the original version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's also the semi-related #128842, where the EXE path being embedded in PDB is reportedly troublesome.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess we should either rip out this feature or put it behind a flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Some.20PDB.20info.20bypasses.20the.20query.20system.20and.20path.20remapping/with/541369247 to ask if anyone else has opinions.

My preference is to just rip out the whole thing and wait for someone to complain, since it's having outsized impact relative to its niche use-case, and we have no idea whether anyone is actually benefiting from it.

hook args_for_debuginfo() -> &'tcx Arc<CommandLineArgsForDebuginfo>;
}

#[cold]
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_middle/src/middle/debuginfo.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#[derive(Debug)]
pub struct CommandLineArgsForDebuginfo {
pub argv0: String,
pub quoted_args: String,
}
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/middle/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
pub mod codegen_fn_attrs;
pub mod debugger_visualizer;
pub mod debuginfo;
pub mod dependency_format;
pub mod exported_symbols;
pub mod lang_items;
Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_middle/src/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ use crate::infer::canonical::{CanonicalParamEnvCache, CanonicalVarKind, Canonica
use crate::lint::lint_level;
use crate::metadata::ModChild;
use crate::middle::codegen_fn_attrs::{CodegenFnAttrs, TargetFeature};
use crate::middle::debuginfo::CommandLineArgsForDebuginfo;
use crate::middle::resolve_bound_vars;
use crate::mir::interpret::{self, Allocation, ConstAllocation};
use crate::mir::{Body, Local, Place, PlaceElem, ProjectionKind, Promoted};
Expand Down Expand Up @@ -1593,6 +1594,10 @@ pub struct GlobalCtxt<'tcx> {

/// A jobserver reference used to release then acquire a token while waiting on a query.
pub jobserver_proxy: Arc<Proxy>,

/// Memoization slot for the [`TyCtxt::args_for_debuginfo`] hook, which needs
/// to not be a query, but should still be cached after being computed once.
pub args_for_debuginfo_cache: OnceLock<Arc<CommandLineArgsForDebuginfo>>,
}

impl<'tcx> GlobalCtxt<'tcx> {
Expand Down Expand Up @@ -1818,6 +1823,7 @@ impl<'tcx> TyCtxt<'tcx> {
alloc_map: interpret::AllocMap::new(),
current_gcx,
jobserver_proxy,
args_for_debuginfo_cache: OnceLock::new(),
});

// This is a separate function to work around a crash with parallel rustc (#135870)
Expand Down
Loading