[AST] Fix bug in UnresolvedSet::erase of last element

[john.brawn]

Summary

UnresolvedSet::erase works by popping the last element then replacing the element to be erased with that element. When the element to be erased is itself the last element this leads to writing past the end of the set, causing an assertion failure.

Fix this by making erase of the last element just pop that element.

[dexonsmith]

LGTM.

[john.brawn]

The test I added here caused failures in buildbots that build with -Wall -Werror, fixed in https://reviews.llvm.org/rG258322105892.

[john.brawn]

Unfortunately my fix then broke builds using MSVC. Hopefully fixed for real in https://reviews.llvm.org/rG25784cd6a962.

[jroelofs]

clang/unittests/AST/UnresolvedSetTest.cpp

11

This ODR violation broke the build for me because it's picking up the definition from Decl.h instead of this one.

/Users/jonathan_roelofs/llvm-upstream/clang/unittests/AST/UnresolvedSetTest.cpp:30:1: error: call to implicitly-deleted default constructor of 'UnresolvedSetTest'
TEST_F(UnresolvedSetTest, Size) { EXPECT_EQ(set.size(), 4u); }
^
/Users/jonathan_roelofs/llvm-upstream/third-party/unittest/googletest/include/gtest/gtest.h:2368:3: note: expanded from macro 'TEST_F'
  GTEST_TEST_(test_fixture, test_name, test_fixture, \
  ^
/Users/jonathan_roelofs/llvm-upstream/third-party/unittest/googletest/include/gtest/internal/gtest-internal.h:1362:5: note: expanded from macro 'GTEST_TEST_'
    GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)() {}                   \
    ^
/Users/jonathan_roelofs/llvm-upstream/third-party/unittest/googletest/include/gtest/internal/gtest-internal.h:1351:3: note: expanded from macro 'GTEST_TEST_CLASS_NAME_'
  test_suite_name##_##test_name##_Test
  ^
<scratch space>:30:1: note: expanded from here
UnresolvedSetTest_Size_Test
^
/Users/jonathan_roelofs/llvm-upstream/clang/unittests/AST/UnresolvedSetTest.cpp:19:13: note: default constructor of 'UnresolvedSetTest' is implicitly deleted because field 'n0' has no default constructor
  NamedDecl n0, n1, n2, n3;
            ^

Maybe it needs to be something like this instead?

diff --git a/clang/unittests/AST/UnresolvedSetTest.cpp b/clang/unittests/AST/UnresolvedSetTest.cpp
index ada857e0e382..3c9077ddc3ec 100644
--- a/clang/unittests/AST/UnresolvedSetTest.cpp
+++ b/clang/unittests/AST/UnresolvedSetTest.cpp
@@ -1,22 +1,23 @@
 #include "clang/AST/UnresolvedSet.h"
+#include "clang/AST/Decl.h"
 #include "gtest/gtest.h"
 
-namespace clang {
-class NamedDecl {
+namespace dummy {
+class NamedDecl : public clang::NamedDecl {
   // DeclAccessPair assumes that NamedDecl is at least 4-byte aligned, so we
   // we need to have a dummy value to make this dummy NamedDecl also be aligned.
   [[maybe_unused]] int dummy;
 
 public:
-  NamedDecl() {}
+  NamedDecl() : clang::NamedDecl(clang::NamedDecl::Kind(0), nullptr, clang::SourceLocation::getFromRawEncoding(0), clang::DeclarationName()) {}
 };
-} // namespace clang
+} // namespace dummy
 
 using namespace clang;
 
 class UnresolvedSetTest : public ::testing::Test {
 protected:
-  NamedDecl n0, n1, n2, n3;
+  dummy::NamedDecl n0, n1, n2, n3;
   UnresolvedSet<2> set;
 
   void SetUp() override {

[jroelofs]

clang/unittests/AST/UnresolvedSetTest.cpp

11

You might need to build with -DLLVM_ENABLE_MODULES=On to see this.

[john.brawn]

clang/unittests/AST/UnresolvedSetTest.cpp

11

https://reviews.llvm.org/rG626c789d92bc should fix this.

clang/include/clang/AST/UnresolvedSet.h

public:
   /// Replaces the declaration at the given iterator with the new one,
   /// preserving the original access bits.
   void replace(iterator I, NamedDecl *New) { I.I->setDecl(New); }
 
   void replace(iterator I, NamedDecl *New, AccessSpecifier AS) {
     I.I->set(New, AS);
   }
 
-  void erase(unsigned I) { decls()[I] = decls().pop_back_val(); }
+  void erase(unsigned I) {
+    auto val = decls().pop_back_val();
+    if (I < size())
+      decls()[I] = val;
+  }
 
-  void erase(iterator I) { *I.I = decls().pop_back_val(); }
+  void erase(iterator I) {
+    auto val = decls().pop_back_val();
+    if (I != end())
+      *I.I = val;
+  }
 
   void setAccess(iterator I, AccessSpecifier AS) { I.I->setAccess(AS); }
 
   void clear() { decls().clear(); }
   void truncate(unsigned N) { decls().truncate(N); }
 
   bool empty() const { return decls().empty(); }
   unsigned size() const { return decls().size(); }

clang/unittests/AST/CMakeLists.txt

add_clang_unittest(ASTTests
   RandstructTest.cpp
   RecursiveASTVisitorTest.cpp
   SizelessTypesTest.cpp
   SourceLocationTest.cpp
   StmtPrinterTest.cpp
   StructuralEquivalenceTest.cpp
   TemplateNameTest.cpp
   TypePrinterTest.cpp
+  UnresolvedSetTest.cpp
   )
 
 clang_target_link_libraries(ASTTests
   PRIVATE
   clangAST
   clangASTMatchers
   clangBasic
   clangFrontend

clang/unittests/AST/UnresolvedSetTest.cpp

+#include "clang/AST/UnresolvedSet.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+class NamedDecl {
+  int dummy;
+
+public:
+  NamedDecl() {}
+};
+} // namespace clang
+
+using namespace clang;
+
+class UnresolvedSetTest : public ::testing::Test {
+protected:
+  NamedDecl n0, n1, n2, n3;
+  UnresolvedSet<2> set;
+
+  void SetUp() override {
+    set.addDecl(&n0);
+    set.addDecl(&n1);
+    set.addDecl(&n2);
+    set.addDecl(&n3);
+  }
+};
+
+TEST_F(UnresolvedSetTest, Size) { EXPECT_EQ(set.size(), 4u); }
+
+TEST_F(UnresolvedSetTest, ArrayOperator) {
+  EXPECT_EQ(set[0].getDecl(), &n0);
+  EXPECT_EQ(set[1].getDecl(), &n1);
+  EXPECT_EQ(set[2].getDecl(), &n2);
+  EXPECT_EQ(set[3].getDecl(), &n3);
+}
+
+TEST_F(UnresolvedSetTest, EraseIntegerFromStart) {
+  set.erase(0);
+  EXPECT_EQ(set.size(), 3u);
+  EXPECT_EQ(set[0].getDecl(), &n3);
+  EXPECT_EQ(set[1].getDecl(), &n1);
+  EXPECT_EQ(set[2].getDecl(), &n2);
+
+  set.erase(0);
+  EXPECT_EQ(set.size(), 2u);
+  EXPECT_EQ(set[0].getDecl(), &n2);
+  EXPECT_EQ(set[1].getDecl(), &n1);
+
+  set.erase(0);
+  EXPECT_EQ(set.size(), 1u);
+  EXPECT_EQ(set[0].getDecl(), &n1);
+
+  set.erase(0);
+  EXPECT_EQ(set.size(), 0u);
+}
+
+TEST_F(UnresolvedSetTest, EraseIntegerFromEnd) {
+  set.erase(3);
+  EXPECT_EQ(set.size(), 3u);
+  EXPECT_EQ(set[0].getDecl(), &n0);
+  EXPECT_EQ(set[1].getDecl(), &n1);
+  EXPECT_EQ(set[2].getDecl(), &n2);
+
+  set.erase(2);
+  EXPECT_EQ(set.size(), 2u);
+  EXPECT_EQ(set[0].getDecl(), &n0);
+  EXPECT_EQ(set[1].getDecl(), &n1);
+
+  set.erase(1);
+  EXPECT_EQ(set.size(), 1u);
+  EXPECT_EQ(set[0].getDecl(), &n0);
+
+  set.erase(0);
+  EXPECT_EQ(set.size(), 0u);
+}
+
+TEST_F(UnresolvedSetTest, EraseIteratorFromStart) {
+  set.erase(set.begin());
+  EXPECT_EQ(set.size(), 3u);
+  EXPECT_EQ(set[0].getDecl(), &n3);
+  EXPECT_EQ(set[1].getDecl(), &n1);
+  EXPECT_EQ(set[2].getDecl(), &n2);
+
+  set.erase(set.begin());
+  EXPECT_EQ(set.size(), 2u);
+  EXPECT_EQ(set[0].getDecl(), &n2);
+  EXPECT_EQ(set[1].getDecl(), &n1);
+
+  set.erase(set.begin());
+  EXPECT_EQ(set.size(), 1u);
+  EXPECT_EQ(set[0].getDecl(), &n1);
+
+  set.erase(set.begin());
+  EXPECT_EQ(set.size(), 0u);
+}
+
+TEST_F(UnresolvedSetTest, EraseIteratorFromEnd) {
+  set.erase(--set.end());
+  EXPECT_EQ(set.size(), 3u);
+  EXPECT_EQ(set[0].getDecl(), &n0);
+  EXPECT_EQ(set[1].getDecl(), &n1);
+  EXPECT_EQ(set[2].getDecl(), &n2);
+
+  set.erase(--set.end());
+  EXPECT_EQ(set.size(), 2u);
+  EXPECT_EQ(set[0].getDecl(), &n0);
+  EXPECT_EQ(set[1].getDecl(), &n1);
+
+  set.erase(--set.end());
+  EXPECT_EQ(set.size(), 1u);
+  EXPECT_EQ(set[0].getDecl(), &n0);
+
+  set.erase(--set.end());
+  EXPECT_EQ(set.size(), 0u);
+}