[ASTImporter] Fix 'isVirtual()' assert failure while importing overridden methods

[danix800]

Summary

CXXMethodDecl::isVirtual() count the number of overridden methods.
This assertion is not true before overridden methods are fully loaded.
The body of this CXXMethodDecl can introduce deps on a derived class
which contains a method overriding this method, causing the assertion failure.

ImportOverriddenMethods() is moved before body loading to fix this issue.

Testcase is contributed by Balázs Kéri (balazske)

[balazske]

It is possible to reproduce the same problem with this test added to ASTImporterTest.cpp:

TEST_P(ASTImporterOptionSpecificTestBase, ImportVirtualOverriddenMethodTest) {
  const char *Code =
      R"(
      void f1();
      class A {
        virtual void f(){}
      };
      class B: public A {
        void f() override {
          f1();
        }
      };
      class C: public B {
        void f() override {}
      };
      void f1() { C c; }
      )";
  Decl *FromTU = getTuDecl(Code, Lang_CXX11);

  auto *FromF = FirstDeclMatcher<CXXMethodDecl>().match(
      FromTU, cxxMethodDecl(hasName("B::f")));

  auto *ToBF = Import(FromF, Lang_CXX11);
  EXPECT_TRUE(ToBF->isVirtual());

  auto *ToCF = FirstDeclMatcher<CXXMethodDecl>().match(
      ToBF->getTranslationUnitDecl(), cxxMethodDecl(hasName("C::f")));
  EXPECT_TRUE(ToCF->isVirtual());
}

I am not opposed to removal of the assertion as fix because it looks not very important (probably it can be replaced by another assertion for example to not allow constructors here, see this commit) but another person in the AST area should check this.

clang/test/Analysis/ctu-astimport-virtual-assertion/main.cpp

22

Such tests are not in the Analysis folder but in the ASTMerge folder instead. I would say that this test is not necessary if the other test (in my added note) is inserted.

[danix800]

clang/test/Analysis/ctu-astimport-virtual-assertion/main.cpp

22

Thanks for the testcase! The orignal one will be removed.

[danix800]

The root cause is on a circular dependency loop. Importing overridden methods before method body can fix this.

clang/lib/AST/DeclCXX.cpp

   assert(MD->isCanonicalDecl() && "Method is not canonical!");
   assert(!MD->getParent()->isDependentContext() &&
          "Can't add an overridden method to a class template!");
-  assert(MD->isVirtual() && "Method is not virtual!");
 
   getASTContext().addOverriddenMethod(this, MD);
 }

clang/test/Analysis/ctu-astimport-virtual-assertion/Inputs/externalDefMap.txt

+71:c:@N@art@F@operator<<#&$@N@std@S@basic_ostream>#C#&1$@N@art@S@Location# input.cpp.ast

clang/test/Analysis/ctu-astimport-virtual-assertion/Inputs/input.h

+#ifndef ART_COMPILER_OPTIMIZING_LOCATIONS_H_
+#define ART_COMPILER_OPTIMIZING_LOCATIONS_H_
+
+namespace std {
+
+template <typename _CharT>
+class basic_ostream {};
+
+typedef basic_ostream<char> ostream;
+
+} // namespace std
+
+namespace art {
+
+class Location;
+std::ostream &operator<<(std::ostream &os, const Location &location);
+
+class RegisterSet {
+public:
+  void Remove(std::ostream &OS, const Location &loc) { OS << loc; }
+};
+
+} // namespace art
+
+#endif // ART_COMPILER_OPTIMIZING_LOCATIONS_H_

clang/test/Analysis/ctu-astimport-virtual-assertion/Inputs/input.cpp

+#include "input.h"
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template <typename _Tp, _Tp __v>
+struct integral_constant {
+  static constexpr _Tp value = __v;
+  typedef _Tp value_type;
+  typedef integral_constant<_Tp, __v> type;
+};
+using false_type = integral_constant<bool, false>;
+using true_type = integral_constant<bool, true>;
+
+template <typename _Base, typename _Derived>
+struct is_base_of
+    : public integral_constant<bool, __is_base_of(_Base, _Derived)> {};
+
+template <typename _Tp>
+struct is_enum : public integral_constant<bool, __is_enum(_Tp)> {};
+
+template <typename _Result, typename _Arg>
+struct __hash_base {
+  typedef _Result result_type;
+  typedef _Arg argument_type;
+};
+
+template <typename _Tp, bool = is_enum<_Tp>::value>
+struct __hash_enum {
+private:
+  // Private rather than deleted to be non-trivially-copyable.
+  __hash_enum(__hash_enum &&);
+  ~__hash_enum();
+};
+
+template <typename _Tp, bool = is_enum<_Tp>::value>
+struct __underlying_type_impl {
+  using type = __underlying_type(_Tp);
+};
+
+template <typename _Tp>
+struct __underlying_type_impl<_Tp, false> {};
+/// @endcond
+
+/// The underlying type of an enum.
+template <typename _Tp>
+struct underlying_type : public __underlying_type_impl<_Tp> {};
+
+/// Primary class template hash, usable for enum types only.
+// Use with non-enum types still SFINAES.
+template <typename _Tp>
+struct hash : __hash_enum<_Tp> {};
+
+// Helper struct for hash with enum types.
+template <typename _Tp>
+struct __hash_enum<_Tp, true> : public __hash_base<size_t, _Tp> {
+  size_t operator()(_Tp __val) const noexcept {
+    using __type = typename underlying_type<_Tp>::type;
+    return hash<__type>{}(static_cast<__type>(__val));
+  }
+};
+
+template <typename _Tp>
+struct hash<_Tp *> : public __hash_base<size_t, _Tp *> {
+  size_t operator()(_Tp *__p) const noexcept {
+    return reinterpret_cast<size_t>(__p);
+  }
+};
+
+template <typename _Tp, typename _Up>
+struct is_same : public false_type {};
+
+template <typename _Tp>
+struct is_same<_Tp, _Tp> : public true_type {};
+
+template <bool, typename _Tp = void>
+struct enable_if {};
+
+template <typename _Tp>
+struct enable_if<true, _Tp> {
+  typedef _Tp type;
+};
+
+} // namespace std
+
+namespace art {
+
+template <typename Derived, typename Tag = void>
+struct IntrusiveForwardListNode {};
+
+template <typename T>
+class IntrusiveForwardList;
+
+template <typename T>
+class IntrusiveForwardListIterator {
+private:
+  template <typename OtherT1, typename OtherT2>
+  friend
+      typename std::enable_if<std::is_same<const OtherT1, const OtherT2>::value,
+                              bool>::type
+      operator==(const IntrusiveForwardListIterator<OtherT1> &lhs,
+                 const IntrusiveForwardListIterator<OtherT2> &rhs);
+};
+
+template <typename T, typename OtherT>
+typename std::enable_if<std::is_same<const T, const OtherT>::value, bool>::type
+operator==(const IntrusiveForwardListIterator<T> &lhs,
+           const IntrusiveForwardListIterator<OtherT> &rhs) {
+  return false;
+}
+
+template <typename T, typename OtherT>
+typename std::enable_if<std::is_same<const T, const OtherT>::value, bool>::type
+operator!=(const IntrusiveForwardListIterator<T> &lhs,
+           const IntrusiveForwardListIterator<OtherT> &rhs) {
+  return false;
+}
+
+template <typename T>
+class IntrusiveForwardList {
+public:
+  typedef IntrusiveForwardListIterator<T> iterator;
+};
+
+} // namespace art
+
+namespace std {
+template <typename _Tp>
+struct numeric_limits {};
+
+template <>
+struct numeric_limits<unsigned short> {
+  static unsigned short max() { return __SHRT_MAX__ * 2U + 1; }
+};
+
+} // namespace std
+namespace art {
+namespace dex {
+
+template <typename T>
+class DexIndex {
+public:
+  T index_;
+
+  constexpr DexIndex() : index_(0) {}
+  explicit constexpr DexIndex(T idx) : index_(idx) {}
+
+  bool IsValid() const {
+    return index_ != std::numeric_limits<unsigned short>::max();
+  }
+  static constexpr DexIndex Invalid() { return DexIndex(0); }
+};
+
+class ProtoIndex : public DexIndex<unsigned short> {
+public:
+  ProtoIndex() {}
+  explicit constexpr ProtoIndex(unsigned short index)
+      : DexIndex<unsigned short>(index) {}
+  static constexpr ProtoIndex Invalid() { return ProtoIndex(0); }
+};
+
+} // namespace dex
+} // namespace art
+
+namespace std {
+
+template <typename>
+struct hash;
+
+template <>
+struct hash<unsigned short> : public __hash_base<size_t, unsigned short> {
+  size_t operator()(unsigned short __val) const noexcept {
+    return static_cast<size_t>(__val);
+  }
+};
+
+template <>
+struct hash<art::dex::ProtoIndex> {
+  size_t operator()(const art::dex::ProtoIndex &index) const {
+    return hash<decltype(index.index_)>()(index.index_);
+  }
+};
+
+} // namespace std
+
+namespace art {
+
+class HGraphVisitor;
+
+template <typename T>
+class HUseListNode : public IntrusiveForwardListNode<HUseListNode<T>> {};
+
+template <typename T>
+using HUseList = IntrusiveForwardList<HUseListNode<T>>;
+
+class HInstruction {
+public:
+  virtual ~HInstruction() {}
+
+  virtual void Accept(HGraphVisitor *visitor) = 0;
+  virtual const char *DebugName() const = 0;
+
+  bool IsVecOperation() const;
+  virtual bool InstructionDataEquals(const HInstruction *other) const {
+    return false;
+  }
+
+private:
+  void FixUpUserRecordsAfterUseInsertion(
+      HUseList<HInstruction *>::iterator fixup_end) {}
+};
+
+class HVecOperation : public HInstruction {
+public:
+  bool InstructionDataEquals(const HInstruction *other) const override {
+    other->IsVecOperation();
+    return false;
+  }
+};
+
+class HVecReduce final : public HVecOperation {
+public:
+  bool InstructionDataEquals(const HInstruction *other) const override {
+    return false;
+  }
+};
+
+class HGraphVisitor {
+public:
+  virtual ~HGraphVisitor() {}
+  virtual void VisitVecOperation(HVecOperation *instr) {}
+};
+
+inline bool HInstruction::IsVecOperation() const {
+  using BaseType = HVecOperation;
+  static constexpr bool results[] = {
+      std::is_base_of<BaseType, HVecReduce>::value,
+  };
+  return false;
+}
+
+} // namespace art
+
+namespace art {
+
+std::ostream &operator<<(std::ostream &os, const Location &location) {
+  return os;
+}
+
+} // namespace art

clang/test/Analysis/ctu-astimport-virtual-assertion/main.cpp

+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+
+// RUN: %clang --target=armv7a-linux-androideabi10000 -c -x c++ \
+// RUN:   -mthumb -std=gnu++17 %p/Inputs/input.cpp -emit-ast \
+// RUN:   -o %t/ctudir/input.cpp.ast
+
+// RUN: cp %p/Inputs/externalDefMap.txt %t/ctudir/externalDefMap.txt
+
+// RUN: %clang_analyze_cc1 -triple thumbv7-unknown-linux-android10000 \
+// RUN:   -std=gnu++17 \
+// RUN:   -analyzer-checker=core.DivideZero \
+// RUN:   -analyzer-opt-analyze-headers \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config display-ctu-progress=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -fno-signed-char \
+// RUN:   -verify %s
+
+// expected-no-diagnostics
+
+#include "Inputs/input.h"