ASTImporterLookupTable.cpp:99 -- Assertion `EraseResult == true && "Trying to remove not contained Decl"'

[vabridgers]

Bugzilla Link	48701
Version	trunk
OS	Windows NT
CC	@steakhal,@devincoughlin,@Teemperor

Extended Description

We came across a crash in ASTImporterLookupTable.cpp:99, Assertion `EraseResult == true && "Trying to remove not contained Decl"'. We narrowed this down due to a regression caused by the following commit:

commit 3f6c856bb5ae4426a586426bca9f1ef2848a2b12 Author: Raphael Isemann teemperor@gmail.com Date: Thu Nov 26 17:02:31 2020 +0100

This conclusion is inferred since the crash does not occur if this commit is reverted.

crash.sh

clang -emit-ast -std=c++11 hpp_libc.cpp -o hpp_libc.cpp.ast

clang -emit-ast -std=c++11 hpp_state.cpp -o hpp_state.cpp.ast

clang --analyze -Xclang -analyzer-checker=core -Xclang -analyzer-config -Xclang experimental-enable-naive-ctu-analysis=true -Xclang -analyzer-config -Xclang ctu-dir=pwd -Xclang -analyzer-config -Xclang display-ctu-progress=true -std=c++11 hpp_libc.cpp

externalDefMap.txt

c:@F@__assertfail#*1C#S0#i#S0_# hpp_libc.cpp.ast c:@F@BHPassertfail#*1C#S0#S0#S0#1i#1i# hpp_state.cpp.ast

hpp_libc.cpp

void BHP_assertfail(const char str, const char assertFunction, const char p, const char assertFile, const unsigned int assertLine, const unsigned int pe);

void assert_fail(const char __assertion, const char file, unsigned int line, const char* __function) { BHP_assertfail("Assert in %s: %s, file %s, line %d\n", function, assertion, file, __line, 0); }

hpp_state.cpp

namespace std { typedef long unsigned int size_t; }

namespace std attribute ((visibility ("default"))) { template struct char_traits;

template<> struct char_traits; }

namespace std attribute ((visibility ("default"))) { template<typename _CharT, typename _Traits = char_traits<_CharT> > class istreambuf_iterator;

template<typename _CharT, typename _Traits = char_traits<_CharT> > class ostreambuf_iterator;

template<typename _CharT, typename _Traits = char_traits<_CharT> > class basic_fstream;

typedef basic_fstream fstream; }

extern "C++" {

namespace std { class exception { public: exception() noexcept { } virtual ~exception() noexcept; }; } // namespace std

namespace std attribute ((visibility ("default"))) { struct __true_type { }; struct __false_type { };

template struct is_char { enum { value = 0 }; typedef false_type type; };

template struct is_move_iterator { enum { value = 0 }; typedef __false_type __type; };

template inline _Iterator miter_base(_Iterator it) { return __it; } } // namespace

namespace __gnu_cxx attribute ((visibility ("default"))) { template<bool, typename> struct __enable_if { };

template struct __enable_if<true, _Tp> { typedef _Tp __type; }; } // namespace } // extern C++

namespace std attribute ((visibility ("default"))) {

template struct char_traits;

template<typename _CharT, typename _Traits> class istreambuf_iterator;

template<bool _IsMove, typename _CharT> typename gnu_cxx::__enable_if<is_char<_CharT>::value, ostreambuf_iterator<_CharT, char_traits<_CharT> > >::type __copy_move_a2(_CharT, _CharT, ostreambuf_iterator<_CharT, char_traits<_CharT> >);

template<bool _IsMove, typename _CharT> typename gnu_cxx::__enable_if<is_char<_CharT>::__value, _CharT>::type copy_move_a2(istreambuf_iterator<_CharT, char_traits<_CharT> >, istreambuf_iterator<_CharT, char_traits<_CharT> >, _CharT);

template<typename _II, typename _OI> inline _OI copy(_II first, _II __last, _OI result) { return (std::copy_move_a2<is_move_iterator<_II>::value> (std::miter_base(first), std::miter_base(last), result)); }

template<typename _II, typename _OI> inline _OI move(_II first, _II __last, _OI result) { } } // namespace

namespace gnu_cxx attribute ((visibility ("default"))) { template struct char_traits { typedef _CharT char_type; static char_type copy(char_type __s1, const char_type* s2, std::size_t __n); };

template typename char_traits<_CharT>::char_type char_traits<_CharT>:: copy(char_type s1, const char_type* __s2, std::size_t n) {

  std::copy(__s2, __s2 + __n, __s1);
  return __s1;
}

} // namespace

namespace std attribute ((visibility ("default"))) { template struct char_traits : public __gnu_cxx::char_traits<_CharT> { };

template<> struct char_traits { typedef char char_type; };

class runtime_error : public exception { public: explicit runtime_error(const char * __arg) ; }; } // namespace

namespace std attribute ((visibility ("default"))) { class mutex { public: mutex() noexcept = default; ~mutex() = default; };

template class unique_lock { public: unique_lock& operator=(unique_lock&& __u) noexcept { unique_lock(std::move(__u)).swap(this); return this; } }; } // namespace

void BHP_assertfail(const char str, const char assertFunction, const char p, const char assertFile, const unsigned int assertLine, const unsigned int pe);

pragma clang diagnostic ignored "-Wunused-parameter"

void BHP_assertfail(const char str, const char assertFunction, const char p, const char assertFile, const unsigned int assertLine, const unsigned int pe) { throw(std::runtime_error("Assert failed")); }

Top of crash stack (Edited)

CTU loaded AST file: hpp_state.cpp.ast clang-12: /clang/lib/AST/ASTImporterLookupTable.cpp:99: void clang::ASTImporterLookupTable::remove(clang::DeclContext, clang::NamedDecl): Assertion `EraseResult == true && "Trying to remove not contained Decl"' failed. PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script. Stack dump:

0. Program arguments: clang-12 --analyze -Xclang -analyzer-checker=core -Xclang -analyzer-config -Xclang experimental-enable-naive-ctu-analysis=true -Xclang -analyzer-config -Xclang ctu-dir=/repo/einvbri/llvm-project-open-famfix/ctu-crash -Xclang -analyzer-config -Xclang display-ctu-progress=true -std=c++11 hpp_libc.cpp
1. parser at end of file
2. While analyzing stack:

   0 Calling __assert_fail

3. hpp_libc.cpp:6:3: Error evaluating statement
4. hpp_libc.cpp:6:3: Error evaluating statement

   ​0 0x0000000003d75077 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /llvm/lib/Support/Unix/Signals.inc:563:22

   ​1 0x0000000003d7512e PrintStackTraceSignalHandler(void*) /llvm/lib/Support/Unix/Signals.inc:630:1

   ​2 0x0000000003d73130 llvm::sys::RunSignalHandlers() /llvm/lib/Support/Signals.cpp:71:20

   ​3 0x0000000003d749eb llvm::sys::CleanupOnSignal(unsigned long) /llvm/lib/Support/Unix/Signals.inc:361:31

   ​4 0x0000000003cc0c38 (anonymous namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) /llvm/lib/Support/CrashRecoveryContext.cpp:75:5

   ​5 0x0000000003cc10d8 CrashRecoverySignalHandler(int) /llvm/lib/Support/CrashRecoveryContext.cpp:389:1

   ​6 0x00007f36edff4630 __restore_rt (/lib64/libpthread.so.0+0xf630)

   ​7 0x00007f36eb41c387 raise (/lib64/libc.so.6+0x36387)

   ​8 0x00007f36eb41da78 abort (/lib64/libc.so.6+0x37a78)

   ​9 0x00007f36eb4151a6 __assert_fail_base (/lib64/libc.so.6+0x2f1a6)

   ​10 0x00007f36eb415252 (/lib64/libc.so.6+0x2f252)

   ​11 0x0000000007fb4c21 clang::ASTImporterLookupTable::remove(clang::DeclContext, clang::NamedDecl) /clang/lib/AST/ASTImporterLookupTable.cpp:100:1

   ​12 0x0000000007fb4d2b clang::ASTImporterLookupTable::remove(clang::NamedDecl*) /clang/lib/AST/ASTImporterLookupTable.cpp:115:64

   ​13 0x0000000007f727b9 clang::ASTImporterSharedState::removeDeclFromLookup(clang::Decl*) /clang/include/clang/AST/ASTImporterSharedState.h:65:3

   ​14 0x0000000007f6a7c8 clang::ASTImporter::Import(clang::Decl*) /clang/lib/AST/ASTImporter.cpp:8295:32

   ​15 0x0000000007f7b994 llvm::Expected<clang::NamedDecl> clang::ASTNodeImporter::import(clang::NamedDecl) /clang/lib/AST/ASTImporter.cpp:165:11

   ​16 0x0000000007f8cf33 llvm::Error clang::ASTNodeImporter::ImportArrayChecked<clang::NamedDecl const, clang::NamedDecl>(clang::NamedDecl const, clang::NamedDecl const, clang::NamedDecl) /clang/lib/AST/ASTImporter.cpp:653:13

[Teemperor]

While making the fix for 1., I realized that https://reviews.llvm.org/D94067 just fixed the issue (I still had my build from before the weekend when debugging this...).

The import error is the same as in the linked patch and can be reproduced with this:

namespace a { // 5. context of 'e' gets imported.
  class e{}; // 4. default argument type of d gets imported here.
  template <typename = e> class d { // 6. namespace children are imported. When searching for conflicting decls we find the 'd' record from 3 without a describing template. structural match fails -> import error.
  };
}

namespace a { // 2. most recent namespace and its children get imported.
  template <typename> class d; // 3. we create an 'd' record decl without the described template, then we import the argument 'e' before setting the describing template.
}

void imported_fn() {
  a::d<int> var; // 1. imports starts here and imports the variable type.
}

My commit causes the regression because it causes step 4 to be triggered from step 3 (before we didn't import e as we ignored default arguments).

The wrong decl in the lookup map was already happening before my commit, but we didn't have that import error that causes us to remove that specific template parameter (and then failing doing so).

So issue 1. is already fixed and issue 2. is still happening (but not crashing anything as we no longer try to remove the decl).

Not sure what's the proper way to fix 2. I assume we can't create the template parameters in the right context because the context doesn't exist yet?

[Teemperor]

hpp_state.cpp can be slightly reduced to:

namespace a {
  template <class> struct b;
  template <> struct b<char>;
} // namespace a

namespace a {
  template <typename c, typename = b<c>> class d;
  typedef b<char> something;
  class e;
} // namespace a

namespace a {
  template <typename, typename Crash> class d;
  template <class> struct b {
    template <typename j> void k() { e *var; d<j> x; }
  };
  template <> struct b<char> {};
} // namespace a

void imported_fn() {
  a::b<char> *var;
}

So from what I can see we have two issues:

1. The import failure of d as the ASTImporter detects a conflict between the two declarations.
2. That causes that we try to remove the Crash parameter from the lookup map for d but it can't be found there.

Not sure what's exactly causing 1, but it seems 2 is caused by us using the TU Context for template parameter as per comment:

ExpectedDecl
ASTNodeImporter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
  // For template arguments, we adopt the translation unit as our declaration
  // context. This context will be fixed when the actual template declaration
  // is created.

We create the parameter in the TU context (and that's where we apparently add it to the lookup map), but then it gets 'fixed' and moved to the template. And then when we hit the import failure and try to remove it again, the new DeclContext (= d) isn't the DeclContext we used in the SharedState lookup map (=TU).

[steakhal]

I forgot to mention that the externalDefMap.txt is this:

c:@F@e# hpp_libc.cpp.ast
c:@N@a@S@l@F@l#&&$@N@a@S@l# hpp_state.cpp.ast
c:@F@imported_fn# hpp_state.cpp.ast
c:@N@a@F@fun# hpp_state.cpp.ast

[steakhal]

The first declaration of class d and class g both refer to the b<c>. I think they should refer to the same entity, but the debugger shows differently.

When I break at https://github.com/llvm/llvm-project/blob/0877b963ef2d3c384b47f7a6161c9d9ab106a7f2/clang/lib/AST/ASTImporter.cpp#L5250 with the following gdb command:

break ASTImporter.cpp:5250
commands
print D->dump()
continue
end

I can see that the sub-ast-tree TemplateArgument type 'b<c>' appears twice, with different addresses. Is it the expected behavior?

/CC teemperor

[steakhal]

I tried to reduce it further, but I could not achieve much TBH.

hpp_libc.cpp

void imported_fn(); void e() { imported_fn(); }

hpp_state.cpp

namespace a { template struct b; template <> struct b; } // namespace a

namespace a { template <typename c, typename = b> class d; class e; template <typename c, typename = b> class g; typedef g something; } // namespace a

namespace a { template <typename, typename> class d; template e h(d); void i(); } // namespace a

template j copyzz() { return a::h; }

namespace a { template struct b { void k() { copyzz; } }; template <> struct b {}; class l {}; } // namespace a

namespace a { void fun() { i; } } // namespace a

void imported_fn() { throw a::l(); }

[vabridgers]

I also meant to add, the above reproducer was minimized from a very specific version of gcc system headers. I worked at minimizing the reproducer as much as possible, but maybe could be minimized more. I think this should serve well enough for further minimization and/or debug.

[vabridgers]

assigned to @devincoughlin