Auto merge of #71131 - pnkfelix:issue-69798-dont-reuse-post-lto-products-when-exports-change, r=nagisa

Do not reuse post LTO products when exports change

Do not reuse post lto products when exports change

Generalizes code from PR #67020, which handled case when imports change.

Fix #69798

Author: bors

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,19 +516,30 @@ 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_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));
+                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);
@@ -881,17 +899,32 @@ 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<String, Vec<String>>,
+    // key = llvm name of exporting module, value = list of modules it exports to
+    exports: FxHashMap<String, Vec<String>>,
 }
 
-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(&[])
     }
 
+    /// 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(&[])
+    }
+
     fn save_to_file(&self, path: &Path) -> io::Result<()> {
         use std::io::Write;
         let file = File::create(path)?;
@@ -906,16 +939,20 @@ impl ThinLTOImports {
         Ok(())
     }
 
-    fn load_from_file(path: &Path) -> io::Result<ThinLTOImports> {
+    fn load_from_file(path: &Path) -> io::Result<ThinLTOImportMaps> {
         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<String> = None;
+        let mut current_imports: Vec<String> = 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 &current_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,17 +964,17 @@ impl ThinLTOImports {
                 current_module = Some(line.trim().to_string());
             }
         }
-        Ok(ThinLTOImports { imports })
+        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);
@@ -951,8 +988,18 @@ 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();
+
+        let mut map = ThinLTOImportMaps::default();
         llvm::LLVMRustGetThinLTOModuleImports(
             data,
             imported_module_callback,

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() }

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() }