From e542f4fa59a2d1ae33bebcd1e11149f7644db5da Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 14 Apr 2020 09:47:03 -0400 Subject: [PATCH 1/3] If an LLVM module's exports change, cannot reuse its post-LTO object file in incremental compilation. This is symmetric to PR #67020, which handled the case where the LLVM module's *imports* changed. This commit builds upon the infrastructure added there; the export map is just the inverse of the import map, so we can build the export map at the same time that we load the serialized import map. Fix #69798 --- src/librustc_codegen_llvm/back/lto.rs | 37 ++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/src/librustc_codegen_llvm/back/lto.rs b/src/librustc_codegen_llvm/back/lto.rs index 816329e06c7a5..c97e108b0f53c 100644 --- a/src/librustc_codegen_llvm/back/lto.rs +++ b/src/librustc_codegen_llvm/back/lto.rs @@ -517,11 +517,20 @@ fn thin_lto( let prev_imports = prev_import_map.modules_imported_by(module_name); let curr_imports = curr_import_map.modules_imported_by(module_name); + let prev_exports = prev_import_map.modules_exported_by(module_name); + let curr_exports = curr_import_map.modules_exported_by(module_name); let imports_all_green = curr_imports .iter() .all(|imported_module| green_modules.contains_key(imported_module)); + let exports_all_green = curr_exports + .iter() + .all(|exported_module| green_modules.contains_key(exported_module)); - if imports_all_green && equivalent_as_sets(prev_imports, curr_imports) { + if imports_all_green + && equivalent_as_sets(prev_imports, curr_imports) + && exports_all_green + && equivalent_as_sets(prev_exports, curr_exports) + { let work_product = green_modules[module_name].clone(); copy_jobs.push(work_product); info!(" - {}: re-used", module_name); @@ -885,6 +894,8 @@ pub unsafe fn optimize_thin_module( pub struct ThinLTOImports { // key = llvm name of importing module, value = list of modules it imports from imports: FxHashMap>, + // key = llvm name of exporting module, value = list of modules it exports to + exports: FxHashMap>, } impl ThinLTOImports { @@ -892,6 +903,10 @@ impl ThinLTOImports { self.imports.get(llvm_module_name).map(|v| &v[..]).unwrap_or(&[]) } + fn modules_exported_by(&self, llvm_module_name: &str) -> &[String] { + self.exports.get(llvm_module_name).map(|v| &v[..]).unwrap_or(&[]) + } + fn save_to_file(&self, path: &Path) -> io::Result<()> { use std::io::Write; let file = File::create(path)?; @@ -909,13 +924,17 @@ impl ThinLTOImports { fn load_from_file(path: &Path) -> io::Result { use std::io::BufRead; let mut imports = FxHashMap::default(); - let mut current_module = None; - let mut current_imports = vec![]; + let mut exports: FxHashMap<_, Vec<_>> = FxHashMap::default(); + let mut current_module: Option = None; + let mut current_imports: Vec = vec![]; let file = File::open(path)?; for line in io::BufReader::new(file).lines() { let line = line?; if line.is_empty() { let importing_module = current_module.take().expect("Importing module not set"); + for imported in ¤t_imports { + exports.entry(imported.clone()).or_default().push(importing_module.clone()); + } imports.insert(importing_module, mem::replace(&mut current_imports, vec![])); } else if line.starts_with(' ') { // Space marks an imported module @@ -927,7 +946,7 @@ impl ThinLTOImports { current_module = Some(line.trim().to_string()); } } - Ok(ThinLTOImports { imports }) + Ok(ThinLTOImports { imports, exports }) } /// Loads the ThinLTO import map from ThinLTOData. @@ -951,7 +970,17 @@ impl ThinLTOImports { .get_mut(importing_module_name) .unwrap() .push(imported_module_name.to_owned()); + + if !map.exports.contains_key(imported_module_name) { + map.exports.insert(imported_module_name.to_owned(), vec![]); + } + + map.exports + .get_mut(imported_module_name) + .unwrap() + .push(importing_module_name.to_owned()); } + let mut map = ThinLTOImports::default(); llvm::LLVMRustGetThinLTOModuleImports( data, From 12207f6c66f6fa5a19790c9c8bf1b398a7dc263d Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 14 Apr 2020 10:52:19 -0400 Subject: [PATCH 2/3] Tests. Namely, a regression test for issue #69798 (export added), and the inverse of that test (export removd). --- .../cgu_invalidated_when_export_added.rs | 26 +++++++++++++++++++ .../cgu_invalidated_when_export_removed.rs | 26 +++++++++++++++++++ 2 files changed, 52 insertions(+) create mode 100644 src/test/incremental/thinlto/cgu_invalidated_when_export_added.rs create mode 100644 src/test/incremental/thinlto/cgu_invalidated_when_export_removed.rs diff --git a/src/test/incremental/thinlto/cgu_invalidated_when_export_added.rs b/src/test/incremental/thinlto/cgu_invalidated_when_export_added.rs new file mode 100644 index 0000000000000..4d48a5f0ac528 --- /dev/null +++ b/src/test/incremental/thinlto/cgu_invalidated_when_export_added.rs @@ -0,0 +1,26 @@ +// revisions: cfail1 cfail2 +// build-pass + +// rust-lang/rust#69798: +// +// This is analgous to cgu_invalidated_when_import_added, but it covers a +// problem uncovered where a change to the *export* set caused a link failure +// when reusing post-LTO optimized object code. + +pub struct Foo {} +impl Drop for Foo { + fn drop(&mut self) { + println!("Dropping Foo"); + } +} +#[no_mangle] +pub extern "C" fn run() { + thread_local! { pub static FOO : Foo = Foo { } ; } + + #[cfg(cfail2)] + { + FOO.with(|_f| ()) + } +} + +pub fn main() { run() } diff --git a/src/test/incremental/thinlto/cgu_invalidated_when_export_removed.rs b/src/test/incremental/thinlto/cgu_invalidated_when_export_removed.rs new file mode 100644 index 0000000000000..e85b4856f3a96 --- /dev/null +++ b/src/test/incremental/thinlto/cgu_invalidated_when_export_removed.rs @@ -0,0 +1,26 @@ +// revisions: cfail1 cfail2 +// build-pass + +// rust-lang/rust#69798: +// +// This is analgous to cgu_invalidated_when_export_added, but it covers the +// other direction. This is analogous to cgu_invalidated_when_import_added: we +// include it, because it may uncover bugs in variant implementation strategies. + +pub struct Foo {} +impl Drop for Foo { + fn drop(&mut self) { + println!("Dropping Foo"); + } +} +#[no_mangle] +pub extern "C" fn run() { + thread_local! { pub static FOO : Foo = Foo { } ; } + + #[cfg(cfail1)] + { + FOO.with(|_f| ()) + } +} + +pub fn main() { run() } From d05ae3a375917cf96ada5bd23f7927636db747e7 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 15 Apr 2020 12:28:01 -0400 Subject: [PATCH 3/3] Incorporated review feedback: Renamed the struct to make it a little clearer that it doesn't just hold one imports map. (I couldn't bring myself to write it as `ThinLTOImportsExports` though, mainly since the exports map is literally derived from the imports map data.) Added some doc to the struct too. Revised comments to add link to the newer issue that discusses why the exports are relevant. Renamed a few of the methods so that the two character difference is more apparent (because 1. the method name is shorter and, perhaps more importantly, the changed characters now lie at the beginning of the method name.) --- src/librustc_codegen_llvm/back/lto.rs | 62 +++++++++++++++++---------- 1 file changed, 40 insertions(+), 22 deletions(-) diff --git a/src/librustc_codegen_llvm/back/lto.rs b/src/librustc_codegen_llvm/back/lto.rs index c97e108b0f53c..e21cdee961dc6 100644 --- a/src/librustc_codegen_llvm/back/lto.rs +++ b/src/librustc_codegen_llvm/back/lto.rs @@ -463,15 +463,18 @@ fn thin_lto( // If previous imports have been deleted, or we get an IO error // reading the file storing them, then we'll just use `None` as the // prev_import_map, which will force the code to be recompiled. - let prev = - if path.exists() { ThinLTOImports::load_from_file(&path).ok() } else { None }; - let curr = ThinLTOImports::from_thin_lto_data(data); + let prev = if path.exists() { + ThinLTOImportMaps::load_from_file(&path).ok() + } else { + None + }; + let curr = ThinLTOImportMaps::from_thin_lto_data(data); (Some(path), prev, curr) } else { // If we don't compile incrementally, we don't need to load the // import data from LLVM. assert!(green_modules.is_empty()); - let curr = ThinLTOImports::default(); + let curr = ThinLTOImportMaps::default(); (None, None, curr) }; info!("thin LTO import map loaded"); @@ -497,10 +500,14 @@ fn thin_lto( let module_name = module_name_to_str(module_name); // If (1.) the module hasn't changed, and (2.) none of the modules - // it imports from has changed, *and* (3.) the import-set itself has - // not changed from the previous compile when it was last - // ThinLTO'ed, then we can re-use the post-ThinLTO version of the - // module. Otherwise, freshly perform LTO optimization. + // it imports from nor exports to have changed, *and* (3.) the + // import and export sets themselves have not changed from the + // previous compile when it was last ThinLTO'ed, then we can re-use + // the post-ThinLTO version of the module. Otherwise, freshly + // perform LTO optimization. + // + // (Note that globally, the export set is just the inverse of the + // import set.) // // This strategy means we can always save the computed imports as // canon: when we reuse the post-ThinLTO version, condition (3.) @@ -509,16 +516,18 @@ fn thin_lto( // version, the current import set *is* the correct one, since we // are doing the ThinLTO in this current compilation cycle.) // - // See rust-lang/rust#59535. + // For more discussion, see rust-lang/rust#59535 (where the import + // issue was discovered) and rust-lang/rust#69798 (where the + // analogous export issue was discovered). if let (Some(prev_import_map), true) = (prev_import_map.as_ref(), green_modules.contains_key(module_name)) { assert!(cgcx.incr_comp_session_dir.is_some()); - let prev_imports = prev_import_map.modules_imported_by(module_name); - let curr_imports = curr_import_map.modules_imported_by(module_name); - let prev_exports = prev_import_map.modules_exported_by(module_name); - let curr_exports = curr_import_map.modules_exported_by(module_name); + let prev_imports = prev_import_map.imports_of(module_name); + let curr_imports = curr_import_map.imports_of(module_name); + let prev_exports = prev_import_map.exports_of(module_name); + let curr_exports = curr_import_map.exports_of(module_name); let imports_all_green = curr_imports .iter() .all(|imported_module| green_modules.contains_key(imported_module)); @@ -890,20 +899,29 @@ pub unsafe fn optimize_thin_module( Ok(module) } +/// Summarizes module import/export relationships used by LLVM's ThinLTO pass. +/// +/// Note that we tend to have two such instances of `ThinLTOImportMaps` in use: +/// one loaded from a file that represents the relationships used during the +/// compilation associated with the incremetnal build artifacts we are +/// attempting to reuse, and another constructed via `from_thin_lto_data`, which +/// captures the relationships of ThinLTO in the current compilation. #[derive(Debug, Default)] -pub struct ThinLTOImports { +pub struct ThinLTOImportMaps { // key = llvm name of importing module, value = list of modules it imports from imports: FxHashMap>, // key = llvm name of exporting module, value = list of modules it exports to exports: FxHashMap>, } -impl ThinLTOImports { - fn modules_imported_by(&self, llvm_module_name: &str) -> &[String] { +impl ThinLTOImportMaps { + /// Returns modules imported by `llvm_module_name` during some ThinLTO pass. + fn imports_of(&self, llvm_module_name: &str) -> &[String] { self.imports.get(llvm_module_name).map(|v| &v[..]).unwrap_or(&[]) } - fn modules_exported_by(&self, llvm_module_name: &str) -> &[String] { + /// Returns modules exported by `llvm_module_name` during some ThinLTO pass. + fn exports_of(&self, llvm_module_name: &str) -> &[String] { self.exports.get(llvm_module_name).map(|v| &v[..]).unwrap_or(&[]) } @@ -921,7 +939,7 @@ impl ThinLTOImports { Ok(()) } - fn load_from_file(path: &Path) -> io::Result { + fn load_from_file(path: &Path) -> io::Result { use std::io::BufRead; let mut imports = FxHashMap::default(); let mut exports: FxHashMap<_, Vec<_>> = FxHashMap::default(); @@ -946,17 +964,17 @@ impl ThinLTOImports { current_module = Some(line.trim().to_string()); } } - Ok(ThinLTOImports { imports, exports }) + Ok(ThinLTOImportMaps { imports, exports }) } /// Loads the ThinLTO import map from ThinLTOData. - unsafe fn from_thin_lto_data(data: *const llvm::ThinLTOData) -> ThinLTOImports { + unsafe fn from_thin_lto_data(data: *const llvm::ThinLTOData) -> ThinLTOImportMaps { unsafe extern "C" fn imported_module_callback( payload: *mut libc::c_void, importing_module_name: *const libc::c_char, imported_module_name: *const libc::c_char, ) { - let map = &mut *(payload as *mut ThinLTOImports); + let map = &mut *(payload as *mut ThinLTOImportMaps); let importing_module_name = CStr::from_ptr(importing_module_name); let importing_module_name = module_name_to_str(&importing_module_name); let imported_module_name = CStr::from_ptr(imported_module_name); @@ -981,7 +999,7 @@ impl ThinLTOImports { .push(importing_module_name.to_owned()); } - let mut map = ThinLTOImports::default(); + let mut map = ThinLTOImportMaps::default(); llvm::LLVMRustGetThinLTOModuleImports( data, imported_module_callback,