[Lldb-commits] [PATCH] D120948: Disable LLDB index cache for .o files with no UUID.

[Greg Clayton via Phabricator via lldb-commits]

clayborg created this revision.
clayborg added reviewers: labath, JDevlieghere, jingham, aadsm, wallace, yinghuitan.
Herald added a subscriber: arphaman.
Herald added a project: All.
clayborg requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

After enabling the LLDB index cache in production we discovered that some distributed build systems play with the modification times of any .o files that were downloaded from the build cache. This was causing the LLDB index cache to read the wrong cache file for files that didn't have a UUID as all of the modfication times were set to the same value by the build system. When new .o files were downloaded, the only unique identifier was the mod time which were all the same, and we would load an older cache for the updated .o file. So disabling caching of files that have no UUIDs for now until we can create a more solid solution.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120948

Files:
  lldb/include/lldb/Core/DataFileCache.h
  lldb/test/API/functionalities/module_cache/bsd/TestModuleCacheBSD.py


Index: lldb/test/API/functionalities/module_cache/bsd/TestModuleCacheBSD.py
===================================================================
--- lldb/test/API/functionalities/module_cache/bsd/TestModuleCacheBSD.py
+++ lldb/test/API/functionalities/module_cache/bsd/TestModuleCacheBSD.py
@@ -41,6 +41,25 @@
     @skipUnlessDarwin
     def test(self):
         """
+            This test has been modified to make sure .o files that don't have
+            UUIDs are not cached after discovering build systems that play with
+            modification times of .o files that the modification times are not
+            unique enough to ensure the .o file within the .a file are the right
+            files as this was causing older cache files to be accepted for new
+            updated .o files.
+
+            ELF .o files do calculate a UUID from the contents of the file,
+            which is expensive, but no one loads .o files into a debug sessions
+            when using ELF files. Mach-o .o files do not have UUID values and do
+            no calculate one as they _are_ used during debug sessions when no
+            dSYM file is generated. If we can find a way to uniquely and cheaply
+            create UUID values for mach-o .o files in the future, this test will
+            be updated to test this functionality. This test will now make sure
+            there are no cache entries for any .o files in BSD archives.
+
+            The old test case description is below in case we enable caching for
+            .o files again:
+
             Test module cache functionality for bsd archive object files.
 
             This will test that if we enable the module cache, we have a
@@ -76,10 +95,20 @@
         main_module.GetNumSymbols()
         a_o_cache_files = self.get_module_cache_files("libfoo.a(a.o)")
         b_o_cache_files = self.get_module_cache_files("libfoo.a(b.o)")
+
+
         # We expect the directory for a.o to have two cache directories:
         # - 1 for the a.o with a earlier mod time
         # - 1 for the a.o that was renamed from c.o that should be 2 seconds older
-        self.assertEqual(len(a_o_cache_files), 2,
-                "make sure there are two files in the module cache directory (%s) for libfoo.a(a.o)" % (self.cache_dir))
-        self.assertEqual(len(b_o_cache_files), 1,
-                "make sure there are two files in the module cache directory (%s) for libfoo.a(b.o)" % (self.cache_dir))
+        # self.assertEqual(len(a_o_cache_files), 2,
+        #         "make sure there are two files in the module cache directory (%s) for libfoo.a(a.o)" % (self.cache_dir))
+        # self.assertEqual(len(b_o_cache_files), 1,
+        #         "make sure there are two files in the module cache directory (%s) for libfoo.a(b.o)" % (self.cache_dir))
+
+        # We are no longer caching .o files in the lldb index cache. If we ever
+        # re-enable this functionality, we can uncomment out the above lines of
+        # code.
+        self.assertEqual(len(a_o_cache_files), 0,
+                "make sure there are no files in the module cache directory (%s) for libfoo.a(a.o)" % (self.cache_dir))
+        self.assertEqual(len(b_o_cache_files), 0,
+                "make sure there are no files in the module cache directory (%s) for libfoo.a(b.o)" % (self.cache_dir))
Index: lldb/include/lldb/Core/DataFileCache.h
===================================================================
--- lldb/include/lldb/Core/DataFileCache.h
+++ lldb/include/lldb/Core/DataFileCache.h
@@ -118,11 +118,13 @@
     m_obj_mod_time = llvm::None;
   }
 
-  /// Return true if any of the signature member variables have valid values.
-  bool IsValid() const {
-    return m_uuid.hasValue() || m_mod_time.hasValue() ||
-           m_obj_mod_time.hasValue();
-  }
+  /// Return true only if the CacheSignature is valid.
+  ///
+  /// Cache signatures are considered valid only if there is a UUID in the file
+  /// that can uniquely identify the file. Some build systems play with
+  /// modification times of file so we can not trust them without using valid
+  /// unique idenifier like the UUID being valid.
+  bool IsValid() const { return m_uuid.hasValue(); }
 
   /// Check if two signatures are the same.
   bool operator!=(const CacheSignature &rhs) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D120948.412869.patch
Type: text/x-patch
Size: 4310 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20220304/82fb353b/attachment-0001.bin>