[Lldb-commits] [PATCH] D71603: [lldb] Make ClangASTImporter::LayoutRecordType always return the same layout

[Raphael Isemann via Phabricator via lldb-commits]

teemperor created this revision.
teemperor added a reviewer: shafik.
Herald added a reviewer: martong.
Herald added a reviewer: a.sidorin.
Herald added subscribers: lldb-commits, JDevlieghere.
Herald added a project: LLDB.
teemperor removed reviewers: martong, a.sidorin.
Herald added a reviewer: a.sidorin.

ClangASTImporter::LayoutRecordType provides record layouts for Clang to use during CodeGen.

Currently this method only provides a layout once and then deletes the layout from the internal
storage. The next time we get a request for the same record layout we pretend that we don't know
the record and leave CodeGen on its own to produce the layout. This system currently seems to work as we
usually request a layout over an ASTContext which caches the result, so the ClangASTImporter
will most likely never be asked twice to layout a record unless the ASTContext changes at some
point.

I think it makes sense that we do not rely on the caching mechanism in the ASTContext to save
us from doing the wrong thing and instead always return the same layout from ClangASTContext.
Also marks the method as `const` because it is essentially just a getter now.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D71603

Files:
  lldb/include/lldb/Symbol/ClangASTImporter.h
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/unittests/Symbol/TestClangASTImporter.cpp


Index: lldb/unittests/Symbol/TestClangASTImporter.cpp
===================================================================
--- lldb/unittests/Symbol/TestClangASTImporter.cpp
+++ lldb/unittests/Symbol/TestClangASTImporter.cpp
@@ -203,18 +203,25 @@
   layout_info.field_offsets[field] = 1;
   importer.InsertRecordDecl(source_record, layout_info);
 
-  uint64_t bit_size;
-  uint64_t alignment;
-  llvm::DenseMap<const clang::FieldDecl *, uint64_t> field_offsets;
-  llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits> base_offsets;
-  llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits> vbase_offsets;
-  importer.LayoutRecordType(source_record, bit_size, alignment, field_offsets,
-                            base_offsets, vbase_offsets);
-
-  EXPECT_EQ(15U, bit_size);
-  EXPECT_EQ(2U, alignment);
-  EXPECT_EQ(1U, field_offsets.size());
-  EXPECT_EQ(1U, field_offsets[field]);
-  EXPECT_EQ(0U, base_offsets.size());
-  EXPECT_EQ(0U, vbase_offsets.size());
+  // Check that asking for the layout multiple times will always return the
+  // same result. Three times should be enough to find any changing
+  for (unsigned i = 0; i < 3U; ++i) {
+    SCOPED_TRACE("Iteration: " + std::to_string(i));
+
+    uint64_t bit_size;
+    uint64_t alignment;
+    llvm::DenseMap<const clang::FieldDecl *, uint64_t> field_offsets;
+    llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits> base_offsets;
+    llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits>
+        vbase_offsets;
+    importer.LayoutRecordType(source_record, bit_size, alignment, field_offsets,
+                              base_offsets, vbase_offsets);
+
+    EXPECT_EQ(15U, bit_size);
+    EXPECT_EQ(2U, alignment);
+    EXPECT_EQ(1U, field_offsets.size());
+    EXPECT_EQ(1U, field_offsets[field]);
+    EXPECT_EQ(0U, base_offsets.size());
+    EXPECT_EQ(0U, vbase_offsets.size());
+  }
 }
Index: lldb/source/Symbol/ClangASTImporter.cpp
===================================================================
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -525,26 +525,23 @@
     llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits>
         &base_offsets,
     llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits>
-        &vbase_offsets) {
-  RecordDeclToLayoutMap::iterator pos =
+        &vbase_offsets) const {
+  RecordDeclToLayoutMap::const_iterator pos =
       m_record_decl_to_layout_map.find(record_decl);
-  bool success = false;
   base_offsets.clear();
   vbase_offsets.clear();
   if (pos != m_record_decl_to_layout_map.end()) {
     bit_size = pos->second.bit_size;
     alignment = pos->second.alignment;
-    field_offsets.swap(pos->second.field_offsets);
-    base_offsets.swap(pos->second.base_offsets);
-    vbase_offsets.swap(pos->second.vbase_offsets);
-    m_record_decl_to_layout_map.erase(pos);
-    success = true;
-  } else {
-    bit_size = 0;
-    alignment = 0;
-    field_offsets.clear();
+    field_offsets = pos->second.field_offsets;
+    base_offsets = pos->second.base_offsets;
+    vbase_offsets = pos->second.vbase_offsets;
+    return true;
   }
-  return success;
+  bit_size = 0;
+  alignment = 0;
+  field_offsets.clear();
+  return false;
 }
 
 void ClangASTImporter::InsertRecordDecl(clang::RecordDecl *decl,
Index: lldb/include/lldb/Symbol/ClangASTImporter.h
===================================================================
--- lldb/include/lldb/Symbol/ClangASTImporter.h
+++ lldb/include/lldb/Symbol/ClangASTImporter.h
@@ -67,7 +67,7 @@
       llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits>
           &base_offsets,
       llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits>
-          &vbase_offsets);
+          &vbase_offsets) const;
 
   bool CanImport(const CompilerType &type);
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D71603.234279.patch
Type: text/x-patch
Size: 3831 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20191217/6b839848/attachment-0001.bin>