[clang] ASTImporter: Fix importing of va_list types and declarations

[mizvekov]

Summary

This fixes a problem where __va_list_tag was not correctly imported,
possibly leading to multiple definitions with different types.

This adds __va_list_tag to it's proper scope, so that the ASTImporter
can find it.

Signed-off-by: Matheus Izvekov <[e-mail]>

[balazske]

(gdb) p Decl->getUnderlyingType().dump()
ConstantArrayType 0x118ea470 'struct __va_list_tag[1]' 1 
`-RecordType 0x118ea2b0 'struct __va_list_tag'
  `-Record 0x118ea228 '__va_list_tag'
$2 = void
(gdb) p Underlying
$3 = {Value = {Value = 295046112}}
(gdb) p Underlying.dump()
ConstantArrayType 0x11960be0 'struct __va_list_tag[1]' 1 
`-RecordType 0x11960a20 'struct __va_list_tag'
  `-Record 0x119609a0 '__va_list_tag'

Is it possible that __va_list_tag is not imported correctly (duplicated), there should be only one instance of this record?

[mizvekov]

I agree having two different val_list types is suspect, it might not have been imported and is simply referring to the va_list of the original context.

One other note, is that if a problem exists for TypedefType, it probably exists for UsingType as well, as they are very closely related.

The one difference is that TypedefType still uses the TypeDecl scheme of uniquing, instead of using a FoldingSet as UsingType does, and the former is a bit unfriendly to the ASTImporter/ASTReader as there is a cycle there, the Decl holds a reference to the type and vice versa.

[balazske]

ASTImporterLookupTable do not contain an entry for __va_list_tag, I do not know why it is missing. If it is added "manually" the crash disappears (without fix in VisitTypedefType). Following code was used to add VaListTagDecl:

ASTImporterLookupTable::ASTImporterLookupTable(TranslationUnitDecl &TU) {
  Builder B(*this);
  B.TraverseDecl(&TU);
  // Add __va_list_tag to the table, it is not visited by the builder.
  if (NamedDecl *D = dyn_cast_or_null<NamedDecl>(TU.getASTContext().getVaListTagDecl()))
    add(&TU, D);
}

The problem probably existed before but did not have visible effect until the new assertion was added.

[vabridgers]

This comment was removed by vabridgers.

[vabridgers]

Adding more information, seems this patch's "hack" returns the following

QualType(T, 0).getDesugaredType(T->getDecl()->getASTContext()); ->

ConstantArrayType 0x11413090 'struct __va_list_tag[1]' imported 1 
`-RecordType 0x11412ed0 'struct __va_list_tag' imported
  `-Record 0x11412e38 '__va_list_tag'

The recent change from @mizvekov returns
T->desugar(); ->

ElaboratedType 0x11442870 '__builtin_va_list' sugar imported
`-TypedefType 0x11442840 '__builtin_va_list' sugar imported
  |-Typedef 0x114130e8 '__builtin_va_list'
  `-ConstantArrayType 0x11413090 'struct __va_list_tag[1]' imported 1 
    `-RecordType 0x11412ed0 'struct __va_list_tag' imported
      `-Record 0x11412e38 '__va_list_tag'

And the code prior to @mizvekov code returns the following...
T->getDecl()->getASTContext().getTypeDeclType(*ToDeclOrErr); ->

TypedefType 0x11442bf0 'va_list' sugar
|-Typedef 0x113d4ad0 'va_list'
`-ElaboratedType 0x1136dbc0 '__builtin_va_list' sugar
  `-TypedefType 0x1136db90 '__builtin_va_list' sugar
    |-Typedef 0x1136db38 '__builtin_va_list'
    `-ConstantArrayType 0x1136dae0 'struct __va_list_tag[1]' 1 
      `-RecordType 0x1136d920 'struct __va_list_tag'
        `-Record 0x1136d898 '__va_list_tag'

Should import desugar types to a canonical underlying type free of syntactic sugar? That's not clear to me at least. @gamesh411 pointed out in our debug session that there are sections of code that iteratively strip sugar until there's no more sugar left wrapping the type. But there's at least one instance where only a single level of sugar is removed.

OTOH, I did find a section of code where va_list is not desugared - looks like for diagnostic purposes - maybe this is a reason to use a solution following @balazske's finding.

Any suggestions on how to proceed with this?

Thanks

[mizvekov]

It's not clear to me there is anything wrong besides our not importing of va_list_tag.
The imported TypedefType should have the same underlying type as the original one, and I did not find anything different is happening on that test case.

I will post a patch fixing the import situation soon, let me know if there is still anything amiss.

[vabridgers]

@mizvekov, thanks for posting a fix. LGTM, but someone else must approve. Best!

[aaron.ballman]

clang/lib/Sema/Sema.cpp

452

The API changed names recently.

460–462

Is it valid to use a local variable as a Scope object? I would assume that scope goes away at the end of this compound statement? I didn't spot other cases where we did this -- usually we call Parser::EnterScope() which allocates a new Scope that Sema then uses.

clang/test/AST/ast-dump-overloaded-operators.cpp

27

This looks like a benign typo -- we still match the line because FileCheck will match partial lines, but I'm pretty sure nothing in your patch would have necessitated this change. Then again, you make this change in a lot of tests, so maybe I'm wrong -- in which case, what changed?

clang/test/AST/ast-dump-traits.cpp

55

Typo?

clang/test/AST/fixed_point.c

405

Typo?

clang/test/PCH/stmt-openmp_structured_block-bit.cpp

16

Typo?

[mizvekov]

clang/test/AST/ast-dump-overloaded-operators.cpp

27

What is happening here (and in all the other such instances) is that on the import case, this declaration stops being the last one on the TU. So the beginning of the line would match on |- instead of `-, but the non-import case this remains the last one.

So I simply relaxed the match.

[mizvekov]

clang/lib/Sema/Sema.cpp

460–462

I don't see anything problematic about allocating it on the stack like this, implementation-wise.

I thought it would be a bit counter-intuitive to use parser functions here, since these declarations are entirely synthetic.

[aaron.ballman]

clang/test/AST/ast-dump-overloaded-operators.cpp

27

Hmmm, I think it'd help to show what new lines are now showing up so we can validate that they make sense in context. WDYT?

[erichkeane]

clang/lib/Sema/Sema.cpp

460–462

I dont know much about "Scope", as it is particularly used in Parsing, but I DO see the comment says: "A scope is a transient data structure that is used while parsing the program. It assists with resolving identifiers to the appropriate declaration."

Which means to me it doesn't need to out-live its parsing time. I THINK it gets replaced with a CXXScope at one point, but I dont know when/where that happens.

[mizvekov]

clang/test/AST/ast-dump-overloaded-operators.cpp

27

It's the new lines from the synthesized va_list_tag. I think they would be noise on this these tests, they are testing something completely unrelated.

But they do show up on the ast-json tests where we are basically dumping the whole TU.

[aaron.ballman]

clang/test/AST/ast-dump-overloaded-operators.cpp

27

Oh, so we're adding those to the *end* of the translation unit, not at the beginning?

[aaron.ballman]

clang/lib/Sema/Sema.cpp

460–462

The situation I'm worried about (and maybe it's not a valid concern) is that Parser::EnterScope uses a cache of scopes that this bypasses and I'm not certain whether that's a good thing or not. I *think* it might be reasonable (Scope is just a holder for a DeclContext and that's the object which has all the declarations added to and removed from, and that context outlives the Scope object), but it's worth double-checking.

[mizvekov]

clang/test/AST/ast-dump-overloaded-operators.cpp

27

We are adding them at the beginning, but it seems we import the other stuff before adding them, so on the import tests they show up on the end.

The thing here is that we end up importing them, but then adding new ones from the current Sema anyway. But this is fine as most other import things, they get separate declarations but merged in any case so they have the same canonical decl.

The way we avoid doing that for the other synthesized builtins around it, is that we just skip creating them if we find their Identifier has been created somehow, which is a fairly strange way to test this.

This seemed even less appropriate for va_list_tag, since some ABIs put it into std namespace.

And it seems to me that avoiding creating them again for the current Sema is a fairly minimal optimization, and it could make we miss diagnosing imports where those things are inconsistent between contexts.

WDYT?

[mizvekov]

clang/lib/Sema/Sema.cpp

460–462

Yeah I checked that. The scope object is fairly innocuous, the EnterScope function uses a cache to stack them, which seems useful in performance intensive workloads, and in case the object needs to outlive the function that creates it.

But here I see no problem just making it clear that this Scope is only used here, by simply allocating it on the stack.

[aaron.ballman]

clang/lib/Sema/Sema.cpp

460–462

Great, thank you for double-checking, I appreciate it!

clang/test/AST/ast-dump-overloaded-operators.cpp

27

We are adding them at the beginning, but it seems we import the other stuff before adding them, so on the import tests they show up on the end.

That really surprises me -- I thought we would export the AST in order and import it in order. I'm a bit worried that out-of-order AST nodes may break invariants.

This seemed even less appropriate for va_list_tag, since some ABIs put it into std namespace.

Yeah, identifying things by name in the frontend is rarely a trivial exercise, we try to avoid it whenever possible.

And it seems to me that avoiding creating them again for the current Sema is a fairly minimal optimization, and it could make we miss diagnosing imports where those things are inconsistent between contexts.

Yeah, it doesn't strike me as a significant optimization to be worth worrying about. My concern is more that we expect import order to match export order and we're not matching up.

[mizvekov]

clang/test/AST/ast-dump-overloaded-operators.cpp

27

We are importing in the right order.

The problem is that we import the pch before we create the builtins here.

Ie look at what the test is doing, it first creates a TU, then imports it into an empty TU /dev/null.
In that last case, we end up adding the declaration for this Sema's va_list_tag after the imported stuff, but the one from the original context is still imported in the right order.

It doesn't seem problematic to me, but would you rather we changed the import order around wrt the creation of the builtins?

[mizvekov]

clang/test/AST/ast-dump-overloaded-operators.cpp

27

I think changing that order is weirder in fact, it would cause part of the current TU to come before the imported one, and then part afterwards.

[aaron.ballman]

LGTM!

clang/test/AST/ast-dump-overloaded-operators.cpp

27

It doesn't seem problematic to me, but would you rather we changed the import order around wrt the creation of the builtins?

Ahhh, I understand now what's going on there. I don't think we need to change the order now that I understand why the behavior is what it is (I was forgetting that we import the PCH and then create builtins).

[mizvekov]

I'll need to have another look at this, apparently the synthesized std namespace breaks some tests on arm platforms.

[chrish_ericsson_atx]

@mizvekov , I'm willing/trying to assist in addressing the buildbot failures that caused you to revert this change. We are eager to see this fix land stably upstream as the crash it addresses is breaking a number of our internal tests. Main thing I lack is any reasonable way to test code changes on aarch64, where it's breaking in buildbot. Your last comment suggests to me that you believe this is a namespace issue; however, I don't see an obvious connection to namespace from the location where the crash occurs in the failing tests (at clang/lib/Serialization/ASTWriter.cpp:5455 in all the failed tests I reviewed). Can you give a bit of context?

[balazske]

There is a problem in one of the tests with DontModifyStdNamespaceCheck:

******************** TEST 'Clang Tools :: clang-tidy/checkers/cert/dcl58-cpp.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /usr/bin/python3.8 /home/tcwg-buildbot/worker/clang-aarch64-quick/llvm/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py -std=c++17-or-later /home/tcwg-buildbot/worker/clang-aarch64-quick/llvm/clang-tools-extra/test/clang-tidy/checkers/cert/dcl58-cpp.cpp cert-dcl58-cpp /home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/tools/clang/tools/extra/test/clang-tidy/checkers/cert/Output/dcl58-cpp.cpp.tmp -- -- -I /home/tcwg-buildbot/worker/clang-aarch64-quick/llvm/clang-tools-extra/test/../test/clang-tidy/checkers/Inputs/Headers
--
Exit Code: 1
Command Output (stdout):
--
Running ['clang-tidy', '/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/tools/clang/tools/extra/test/clang-tidy/checkers/cert/Output/dcl58-cpp.cpp.tmp.cpp', '-fix', '--checks=-*,cert-dcl58-cpp', '-config={}', '--', '-I', '/home/tcwg-buildbot/worker/clang-aarch64-quick/llvm/clang-tools-extra/test/../test/clang-tidy/checkers/Inputs/Headers', '-std=c++17', '-nostdinc++']...
clang-tidy /home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/tools/clang/tools/extra/test/clang-tidy/checkers/cert/Output/dcl58-cpp.cpp.tmp.cpp -fix --checks=-*,cert-dcl58-cpp -config={} -- -I /home/tcwg-buildbot/worker/clang-aarch64-quick/llvm/clang-tools-extra/test/../test/clang-tidy/checkers/Inputs/Headers -std=c++17 -nostdinc++ failed:
clang-tidy: ../llvm/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:177: clang::DiagnosticBuilder clang::tidy::ClangTidyContext::diag(llvm::StringRef, clang::SourceLocation, llvm::StringRef, DiagnosticIDs::Level): Assertion `Loc.isValid()' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: clang-tidy /home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/tools/clang/tools/extra/test/clang-tidy/checkers/cert/Output/dcl58-cpp.cpp.tmp.cpp -fix --checks=-*,cert-dcl58-cpp -config={} -- -I /home/tcwg-buildbot/worker/clang-aarch64-quick/llvm/clang-tools-extra/test/../test/clang-tidy/checkers/Inputs/Headers -std=c++17 -nostdinc++
1.	<eof> parser at end of file
2.	ASTMatcher: Processing 'cert-dcl58-cpp' against:
	CXXRecordDecl std::__va_list : <<invalid sloc>>
--- Bound Nodes Begin ---
    decl - { CXXRecordDecl std::__va_list : <<invalid sloc>> }
    nmspc - { NamespaceDecl std : <<invalid sloc>> }
--- Bound Nodes End ---
 #0 0x0000aaaacd9fe54c llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x352554c)
 #1 0x0000aaaacd9fc664 llvm::sys::RunSignalHandlers() (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x3523664)
 #2 0x0000aaaacd9fead8 SignalHandler(int) Signals.cpp:0:0
 #3 0x0000ffffbe3e05c0 (linux-vdso.so.1+0x5c0)
 #4 0x0000ffffbdec1d78 raise /build/glibc-RIFKjK/glibc-2.31/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #5 0x0000ffffbdeaeaac abort /build/glibc-RIFKjK/glibc-2.31/stdlib/abort.c:81:7
 #6 0x0000ffffbdebb490 __assert_fail_base /build/glibc-RIFKjK/glibc-2.31/assert/assert.c:89:7
 #7 0x0000ffffbdebb4f4 (/lib/aarch64-linux-gnu/libc.so.6+0x2d4f4)
 #8 0x0000aaaacb809afc clang::tidy::ClangTidyContext::diag(llvm::StringRef, clang::SourceLocation, llvm::StringRef, clang::DiagnosticIDs::Level) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x1330afc)
 #9 0x0000aaaacb2d18a4 clang::tidy::cert::DontModifyStdNamespaceCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0xdf88a4)
#10 0x0000aaaaccd1bad8 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes const&) ASTMatchFinder.cpp:0:0
#11 0x0000aaaaccd51b8c clang::ast_matchers::internal::BoundNodesTreeBuilder::visitMatches(clang::ast_matchers::internal::BoundNodesTreeBuilder::Visitor*) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x2878b8c)
#12 0x0000aaaaccd1b170 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::matchWithFilter(clang::DynTypedNode const&) ASTMatchFinder.cpp:0:0
#13 0x0000aaaaccd1e5f0 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) ASTMatchFinder.cpp:0:0
#14 0x0000aaaaccd20c80 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseNamespaceDecl(clang::NamespaceDecl*) ASTMatchFinder.cpp:0:0
#15 0x0000aaaaccd1e790 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) ASTMatchFinder.cpp:0:0
#16 0x0000aaaaccd2820c clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseTranslationUnitDecl(clang::TranslationUnitDecl*) ASTMatchFinder.cpp:0:0
#17 0x0000aaaaccd1ecb8 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) ASTMatchFinder.cpp:0:0
#18 0x0000aaaaccce9ff8 clang::ast_matchers::MatchFinder::matchAST(clang::ASTContext&) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x2810ff8)
#19 0x0000aaaacbede4f0 clang::MultiplexConsumer::HandleTranslationUnit(clang::ASTContext&) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x1a054f0)
#20 0x0000aaaacc0ba9d8 clang::ParseAST(clang::Sema&, bool, bool) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x1be19d8)
#21 0x0000aaaacbea352c clang::FrontendAction::Execute() (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x19ca52c)
#22 0x0000aaaacbe2c40c clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x195340c)
#23 0x0000aaaacb8342f0 clang::tooling::FrontendActionFactory::runInvocation(std::shared_ptr<clang::CompilerInvocation>, clang::FileManager*, std::shared_ptr<clang::PCHContainerOperations>, clang::DiagnosticConsumer*) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x135b2f0)
#24 0x0000aaaacb7ffdac clang::tidy::runClangTidy(clang::tidy::ClangTidyContext&, clang::tooling::CompilationDatabase const&, llvm::ArrayRef<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem>, bool, bool, llvm::StringRef)::ActionFactory::runInvocation(std::shared_ptr<clang::CompilerInvocation>, clang::FileManager*, std::shared_ptr<clang::PCHContainerOperations>, clang::DiagnosticConsumer*) ClangTidy.cpp:0:0
#25 0x0000aaaacb833fe0 clang::tooling::ToolInvocation::runInvocation(char const*, clang::driver::Compilation*, std::shared_ptr<clang::CompilerInvocation>, std::shared_ptr<clang::PCHContainerOperations>) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x135afe0)
#26 0x0000aaaacb832b78 clang::tooling::ToolInvocation::run() (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x1359b78)
#27 0x0000aaaacb835a90 clang::tooling::ClangTool::run(clang::tooling::ToolAction*) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x135ca90)
#28 0x0000aaaacb7fba74 clang::tidy::runClangTidy(clang::tidy::ClangTidyContext&, clang::tooling::CompilationDatabase const&, llvm::ArrayRef<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem>, bool, bool, llvm::StringRef) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0x1322a74)
#29 0x0000aaaacb0c6598 clang::tidy::clangTidyMain(int, char const**) (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0xbed598)
#30 0x0000ffffbdeaee10 __libc_start_main /build/glibc-RIFKjK/glibc-2.31/csu/../csu/libc-start.c:342:3
#31 0x0000aaaacb0c16a8 _start (/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/bin/clang-tidy+0xbe86a8)

Likely the unless(isImplicit()) should be added to some of the AST matchers in the test.

[balazske]

This test fails because FirstDeclMatcher does not find a node, probably FromNs. Probably adding hasName("x") fixes the problem.

TEST_P(ASTImporterOptionSpecificTestBase, ImportCorrectTemplatedDecl) {
  auto Code =
        R"(
        namespace x {
          template<class X> struct S1{};
          template<class X> struct S2{};
          template<class X> struct S3{};
        }
        )";
  Decl *FromTU = getTuDecl(Code, Lang_CXX03);
  auto FromNs =
      FirstDeclMatcher<NamespaceDecl>().match(FromTU, namespaceDecl());
  auto ToNs = cast<NamespaceDecl>(Import(FromNs, Lang_CXX03));
  ASSERT_TRUE(ToNs);
  auto From =
      FirstDeclMatcher<ClassTemplateDecl>().match(FromTU,
                                                  classTemplateDecl(
                                                      hasName("S2")));
  auto To =
      FirstDeclMatcher<ClassTemplateDecl>().match(ToNs,
                                                  classTemplateDecl(
                                                      hasName("S2")));

[balazske]

The other tests except Clang::complete-preamble.cpp and Clang::preamble.c fail with this assertion:

./llvm/clang/lib/Serialization/ASTWriter.cpp:5455: clang::serialization::DeclID clang::ASTWriter::getDeclID(const clang::Decl *): Assertion `DeclIDs.find(D) != DeclIDs.end() && "Declaration not emitted!"' failed.

This was obtained from a new rebuild of previously failing tests (Builders/clang-aarch64-quick):
https://lab.llvm.org/buildbot/#/builders/188/builds/23143

[DavidSpickett]

Hi folks, I'm one of the owners of the AArch64 bot you're seeing the failure on. I will try to reproduce it myself and see if I can find what's going on.

[DavidSpickett]

I was able to reproduce on AArch64 and Arm (32 bit), did not reproduce on an Intel machine which matches what you've seen so far. 62 failures on AArch64, 21 on Arm. Broadly two kinds of failure.

Errors of the form error: reference to 'std' is ambiguous, which happens on both platforms.

Asserts, which only show up on AArch64:

clang: /home/david.spickett/llvm-project/clang/lib/Serialization/ASTWriter.cpp:5455: clang::serialization::DeclID clang::ASTWriter::getDeclID(const clang::Decl *): Assertion `DeclIDs.find(D) != DeclIDs.end() && "Declaration not emitted!"' failed.

If you have some inclination of what might be going on I can check for specific things. Failing that I can get you access to a development container to try it yourself.

[jrtc27]

Arm and AArch64 are special because they're the architectures where va_list is std::__va_list, not a bare __va_list_tag or similar.

[jrtc27]

This should be readily reproducible with clang -target aarch64 on any machine, though? The current lit tests are just x86 ones, which isn't great coverage of this.

[balazske]

At least a part of the failures is reproducible with -target aarch64 option. I try to fix the failures.

[DavidSpickett]

I am attempting to reduce the compiler crash on 32 bit, it's a slow process.

Ignore me, wrong change.

[vabridgers]

It appears to me this change https://reviews.llvm.org/D116774 is responsible for the unexpected behavior. Question for @jrtc27 : do you think if we could make this change consistent with https://reviews.llvm.org/D116774 that the problem would be addressed? Looks like we could make consistent changes in Sema.cpp and perhaps the problem would be addressed?

I'm assuming that keeping __va_list in std for aarch6 and arm is a requirement for the abi and not open to change?
Best

[vabridgers]

I've reproduced the crash locally on an x86 machine. Working on a fix.

[vabridgers]

I've made a very simple reproducer for this crash, clang-tidy executes on an x86 machine. Requires aarch64 and/or arm support compiled in.

crashes - clang-tidy crash.cpp -checks="cert-dcl58-cpp" -- -target arm
crashes - clang-tidy crash.cpp -checks="cert-dcl58-cpp" -- -target aarch64
no crash - clang-tidy crash.cpp -checks="cert-dcl58-cpp" -- -target x86_64

crash.cpp ...

namespace std {
} // namespace std

The partial crash stack ...

$ clang-tidy crash.cpp -checks="cert-dcl58-cpp" --  -target arm 
clang-tidy: <root>/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:178: clang::DiagnosticBuilder clang::tidy::ClangTidyContext::diag(llvm::StringRef, clang::SourceLocation, llvm::StringRef, clang::DiagnosticIDs::Level): Assertion `Loc.isValid()' failed.

Stack dump:
0.    Program arguments: clang-tidy crash.cpp -checks=cert-dcl58-cpp -- -target arm
1.    <eof> parser at end of file
2.    ASTMatcher: Processing 'cert-dcl58-cpp' against:
      CXXRecordDecl std::__va_list : <<invalid sloc>>
--- Bound Nodes Begin ---
    decl - { CXXRecordDecl std::__va_list : <<invalid sloc>> }
    nmspc - { NamespaceDecl std : <<invalid sloc>> }
--- Bound Nodes End ---
 #0 0x0000000006dcfe46 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) <root>/llvm/lib/Support/Unix/Signals.inc:567:22
 #1 0x0000000006dd0232 PrintStackTraceSignalHandler(void*) <root>/llvm/lib/Support/Unix/Signals.inc:641:1
 #2 0x0000000006dcde75 llvm::sys::RunSignalHandlers() <root>/llvm/lib/Support/Signals.cpp:104:20
 #3 0x0000000006dcf884 SignalHandler(int) <root>/llvm/lib/Support/Unix/Signals.inc:412:1
 #4 0x00007fa725903630 __restore_rt sigaction.c:0:0
 #5 0x00007fa722aac387 raise (/lib64/libc.so.6+0x36387)
 #6 0x00007fa722aada78 abort (/lib64/libc.so.6+0x37a78)
 #7 0x00007fa722aa51a6 __assert_fail_base (/lib64/libc.so.6+0x2f1a6)
 #8 0x00007fa722aa5252 (/lib64/libc.so.6+0x2f252)
 #9 0x0000000003132a80 clang::tidy::ClangTidyContext::diag(llvm::StringRef, clang::SourceLocation, llvm::StringRef, clang::DiagnosticIDs::Level) <root>/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:179:17
#10 0x000000000312ea92 clang::tidy::ClangTidyCheck::diag(clang::SourceLocation, llvm::StringRef, clang::DiagnosticIDs::Level) <root>/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:27:54
#11 0x000000000269e015 clang::tidy::cert::DontModifyStdNamespaceCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&) <root>/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp:121:10
#12 0x000000000312eb96 clang::tidy::ClangTidyCheck::run(clang::ast_matchers::MatchFinder::MatchResult const&) <root>/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:46:1
#13 0x0000000005873615 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes const&) <root>/clang/lib/ASTMatchers/ASTMatchFinder.cpp:1260:34

[jrtc27]

In D136886#4083762, @vabridgers wrote:

It appears to me this change https://reviews.llvm.org/D116774 is responsible for the unexpected behavior. Question for @jrtc27 : do you think if we could make this change consistent with https://reviews.llvm.org/D116774 that the problem would be addressed? Looks like we could make consistent changes in Sema.cpp and perhaps the problem would be addressed?

I'm assuming that keeping __va_list in std for aarch6 and arm is a requirement for the abi and not open to change?
Best

I don't understand the question. va_list needs to be in std on C++ aarch64/arm as that's part of the ABI. va_list cannot be in std on C aarch64/arm as namespaces do not exist in C and having the AST say they do causes all manner of problems (as outlined in my prior comments on those revisions).

[balazske]

In my previous comments I indicated how to fix the problem in the lang-tidy check, and I could reproduce the crash in ImportCorrectTemplatedDecl and have the fix for it. I can reproduce the assertion "Declaration not emitted!" but did not found the fix, the AST contains 2 unlinked instances of struct va_list definition in this case for unknown reason (I think there should be one of these declarations).

[balazske]

The "Declaration not emitted" crash is reproducible by change of file "test/Index/targeted-cursor.c", the similar lines should be replaced to this (only the target aarch64 is added):

// RUN: env CINDEXTEST_FAILONERROR=1 CINDEXTEST_EDITING=1 CINDEXTEST_COMPLETION_NO_CACHING=1 \
// RUN:   c-index-test -cursor-at=%s:5:10 %s -target aarch64 -include %t.h \
// RUN:    -Xclang -error-on-deserialized-decl=PreambleVar  \
// RUN:    -Xclang -error-on-deserialized-decl=NestedVar1  \
// RUN:    -Xclang -error-on-deserialized-decl=TopVar  \
// RUN:  | FileCheck %s -check-prefix=LOCAL-CURSOR1

The other "run" lines in the file can be removed to simplify the test. The run lines that are above these lines are not producing crash.

[balazske]

I am not an expert in the frontend or ASTContext area, but I think that if a builtin declaration is created (ASTContext::getBuiltinVaListDecl) the identifier name for it should not be loaded from an external source (in function IdentifierTable::get). This load of identifier causes it to be get from ASTReader which in turn gets va_list it again from ASTContext because this is a predefined declaration. In this way 2 instances of va_list are added to the AST. The following call chain shows the problem (first create of va_list happens when Sema is initialized, second create happens when the __va_list is loaded by ASTReader as result of the first attempt to create it):

#0  clang::RecordDecl::RecordDecl (this=0x7fffd407a1c8, DK=clang::Decl::Record, TK=clang::TTK_Struct, C=..., DC=0x7fffd4041ec0, StartLoc=..., IdLoc=..., Id=0x7fffd407abb8, PrevDecl=0x0) at /local/clang/llvm2/llvm-project/clang/lib/AST/Decl.cpp:4700
#1  0x00007ffff4f26568 in clang::RecordDecl::Create (C=..., TK=clang::TTK_Struct, DC=0x7fffd4041ec0, StartLoc=..., IdLoc=..., Id=0x7fffd407abb8, PrevDecl=0x0) at /local/clang/llvm2/llvm-project/clang/lib/AST/Decl.cpp:4720
#2  0x00007ffff4ac92f6 in clang::ASTContext::buildImplicitRecord (this=0x7fffd402b110, Name=..., TK=clang::TTK_Struct) at /local/clang/llvm2/llvm-project/clang/lib/AST/ASTContext.cpp:1216
#3  0x00007ffff4b0a8e3 in CreateAArch64ABIBuiltinVaListDecl (Context=0x7fffd402b110) at /local/clang/llvm2/llvm-project/clang/lib/AST/ASTContext.cpp:8757
#4  0x00007ffff4af4be6 in CreateVaListDecl (Context=0x7fffd402b110, Kind=clang::TargetInfo::AArch64ABIBuiltinVaList) at /local/clang/llvm2/llvm-project/clang/lib/AST/ASTContext.cpp:9097
#5  0x00007ffff4af4b14 in clang::ASTContext::getBuiltinVaListDecl (this=0x7fffd402b110) at /local/clang/llvm2/llvm-project/clang/lib/AST/ASTContext.cpp:9117
#6  0x00007ffff4af4cab in clang::ASTContext::getVaListTagDecl (this=0x7fffd402b110) at /local/clang/llvm2/llvm-project/clang/lib/AST/ASTContext.cpp:9128
#7  0x00007ffff58b4461 in getPredefinedDecl (Context=..., ID=clang::serialization::PREDEF_DECL_VA_LIST_TAG) at /local/clang/llvm2/llvm-project/clang/lib/Serialization/ASTReader.cpp:7496
#8  0x00007ffff58a38e5 in clang::ASTReader::GetExistingDecl (this=0x7fffd40436e0, ID=10) at /local/clang/llvm2/llvm-project/clang/lib/Serialization/ASTReader.cpp:7525
#9  0x00007ffff58acabd in clang::ASTReader::GetDecl (this=0x7fffd40436e0, ID=10) at /local/clang/llvm2/llvm-project/clang/lib/Serialization/ASTReader.cpp:7549
#10 0x00007ffff5887f51 in clang::ASTReader::SetGloballyVisibleDecls (this=0x7fffd40436e0, II=0x7fffd407abb8, DeclIDs=..., Decls=0x7fffd4079198) at /local/clang/llvm2/llvm-project/clang/lib/Serialization/ASTReader.cpp:8596
#11 0x00007ffff58bafb5 in clang::ASTReader::finishPendingActions (this=0x7fffd40436e0) at /local/clang/llvm2/llvm-project/clang/lib/Serialization/ASTReader.cpp:9276
#12 0x00007ffff58be073 in clang::ASTReader::FinishedDeserializing (this=0x7fffd40436e0) at /local/clang/llvm2/llvm-project/clang/lib/Serialization/ASTReader.cpp:9783
#13 0x00007ffff58d093f in clang::ExternalASTSource::Deserializing::~Deserializing (this=0x7fffe2c990b0) at /local/clang/llvm2/llvm-project/clang/include/clang/AST/ExternalASTSource.h:86
#14 0x00007ffff58b7427 in clang::ASTReader::get (this=0x7fffd40436e0, Name=...) at /local/clang/llvm2/llvm-project/clang/lib/Serialization/ASTReader.cpp:8126
#15 0x00007ffff4b18c4b in clang::IdentifierTable::get (this=0x7fffd401b720, Name=...) at /local/clang/llvm2/llvm-project/clang/include/clang/Basic/IdentifierTable.h:605
#16 0x00007ffff4ac92bd in clang::ASTContext::buildImplicitRecord (this=0x7fffd402b110, Name=..., TK=clang::TTK_Struct) at /local/clang/llvm2/llvm-project/clang/lib/AST/ASTContext.cpp:1217
#17 0x00007ffff4b0a8e3 in CreateAArch64ABIBuiltinVaListDecl (Context=0x7fffd402b110) at /local/clang/llvm2/llvm-project/clang/lib/AST/ASTContext.cpp:8757
#18 0x00007ffff4af4be6 in CreateVaListDecl (Context=0x7fffd402b110, Kind=clang::TargetInfo::AArch64ABIBuiltinVaList) at /local/clang/llvm2/llvm-project/clang/lib/AST/ASTContext.cpp:9097
#19 0x00007ffff4af4b14 in clang::ASTContext::getBuiltinVaListDecl (this=0x7fffd402b110) at /local/clang/llvm2/llvm-project/clang/lib/AST/ASTContext.cpp:9117
#20 0x00007ffff4af4cab in clang::ASTContext::getVaListTagDecl (this=0x7fffd402b110) at /local/clang/llvm2/llvm-project/clang/lib/AST/ASTContext.cpp:9128
#21 0x00007fffee43a586 in clang::Sema::Initialize (this=0x7fffd406a7e0) at /local/clang/llvm2/llvm-project/clang/lib/Sema/Sema.cpp:453
#22 0x00007fffe773b432 in clang::Parser::Initialize (this=0x7fffd40753c0) at /local/clang/llvm2/llvm-project/clang/lib/Parse/Parser.cpp:562
#23 0x00007fffe75d3fd4 in clang::ParseAST (S=..., PrintStats=false, SkipFunctionBodies=false) at /local/clang/llvm2/llvm-project/clang/lib/Parse/ParseAST.cpp:155
#24 0x00007ffff6169622 in clang::ASTFrontendAction::ExecuteAction (this=0x7fffd4014c70) at /local/clang/llvm2/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1162
#25 0x00007ffff6168fe8 in clang::FrontendAction::Execute (this=0x7fffd4014c70) at /local/clang/llvm2/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1055
#26 0x00007ffff60093fc in clang::ASTUnit::Parse (this=0x7fffd40118a0, PCHContainerOps=std::shared_ptr<clang::PCHContainerOperations> (empty) = {...}, OverrideMainBuffer=std::unique_ptr<llvm::MemoryBuffer> = {...}, VFS=...)
    at /local/clang/llvm2/llvm-project/clang/lib/Frontend/ASTUnit.cpp:1237
#27 0x00007ffff600cf5f in clang::ASTUnit::LoadFromCompilerInvocation (this=0x7fffd40118a0, PCHContainerOps=std::shared_ptr<clang::PCHContainerOperations> (empty) = {...}, PrecompilePreambleAfterNParses=2, VFS=...)
    at /local/clang/llvm2/llvm-project/clang/lib/Frontend/ASTUnit.cpp:1696
#28 0x00007ffff600dfc0 in clang::ASTUnit::LoadFromCommandLine (ArgBegin=0x7fffd4003440, ArgEnd=0x7fffd40034c0, PCHContainerOps=std::shared_ptr<clang::PCHContainerOperations> (empty) = {...}, Diags=..., ResourceFilesPath=..., OnlyLocalDecls=true,
    CaptureDiagnostics=clang::CaptureDiagsKind::All, RemappedFiles=..., RemappedFilesKeepOriginalName=true, PrecompilePreambleAfterNParses=2, TUKind=clang::TU_Complete, CacheCodeCompletionResults=false, IncludeBriefCommentsInCodeCompletion=false,
    AllowPCHWithCompilerErrors=true, SkipFunctionBodies=clang::SkipFunctionBodiesScope::None, SingleFileParse=false, UserFilesAreVolatile=true, ForSerialization=false, RetainExcludedConditionalBlocks=false, ModuleFormat=std::optional<llvm::StringRef> = {...},
    ErrAST=0x7fffe2c9ba20, VFS=...) at /local/clang/llvm2/llvm-project/clang/lib/Frontend/ASTUnit.cpp:1820
#29 0x00007ffff79ed60c in clang_parseTranslationUnit_Impl (CIdx=0x7fffdc0151f0, source_filename=0x7fffffffe797 "-error-on-deserialized-decl=TopVar", command_line_args=0x7fffdc015520, num_command_line_args=11, unsaved_files=..., options=5, out_TU=0x7fffe349cc38)
    at /local/clang/llvm2/llvm-project/clang/tools/libclang/CIndex.cpp:3890
#30 0x00007ffff79ecbef in clang_parseTranslationUnit2FullArgv::$_1::operator() (this=0x7fffe349c9b8) at /local/clang/llvm2/llvm-project/clang/tools/libclang/CIndex.cpp:3973
#31 0x00007ffff79ecb45 in llvm::function_ref<void ()>::callback_fn<clang_parseTranslationUnit2FullArgv::$_1>(long) (callable=140737006651832) at /local/clang/llvm2/llvm-project/llvm/include/llvm/ADT/STLFunctionalExtras.h:45
#32 0x00007ffff359b119 in llvm::function_ref<void ()>::operator()() const (this=0x7fffe2c9bcf8) at /local/clang/llvm2/llvm-project/llvm/include/llvm/ADT/STLFunctionalExtras.h:68
#33 0x00007ffff35af7c4 in llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) (this=0x7fffe349c9a0, Fn=...) at /local/clang/llvm2/llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:426
#34 0x00007ffff35afbcf in RunSafelyOnThread_Dispatch (UserData=0x7fffe349c8a0) at /local/clang/llvm2/llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:507
#35 0x00007ffff35b0469 in _ZZN4llvm6thread18GenericThreadProxyISt5tupleIJPFvPvEPN12_GLOBAL__N_121RunSafelyOnThreadInfoEEEEEvS3_ENKUlOT_DpOT0_E_clIRS5_JRS8_EEEDaSB_SE_ (this=0x7fffe2c9be78, F=@0x7fffdc015588: 0x7ffff35afb80 <RunSafelyOnThread_Dispatch(void*)>,
    Args=@0x7fffdc015580: 0x7fffe349c8a0) at /local/clang/llvm2/llvm-project/llvm/include/llvm/Support/thread.h:43
#36 0x00007ffff35b0417 in _ZSt13__invoke_implIvZN4llvm6thread18GenericThreadProxyISt5tupleIJPFvPvEPN12_GLOBAL__N_121RunSafelyOnThreadInfoEEEEEvS4_EUlOT_DpOT0_E_JRS6_RS9_EESB_St14__invoke_otherOT0_DpOT1_ (__f=..., __args=@0x7fffdc015580: 0x7fffe349c8a0,
    __args=@0x7fffdc015580: 0x7fffe349c8a0) at /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/invoke.h:60
#37 0x00007ffff35b0387 in _ZSt8__invokeIZN4llvm6thread18GenericThreadProxyISt5tupleIJPFvPvEPN12_GLOBAL__N_121RunSafelyOnThreadInfoEEEEEvS4_EUlOT_DpOT0_E_JRS6_RS9_EENSt15__invoke_resultISB_JDpSD_EE4typeESC_SF_ (__fn=..., __args=@0x7fffdc015580: 0x7fffe349c8a0,
    __args=@0x7fffdc015580: 0x7fffe349c8a0) at /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/invoke.h:95
#38 0x00007ffff35b0313 in _ZSt12__apply_implIZN4llvm6thread18GenericThreadProxyISt5tupleIJPFvPvEPN12_GLOBAL__N_121RunSafelyOnThreadInfoEEEEEvS4_EUlOT_DpOT0_E_RSA_JLm0ELm1EEEDcSC_OT0_St16integer_sequenceImJXspT1_EEE (__f=..., __t=std::tuple containing = {...})
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/tuple:1662
#39 0x00007ffff35b0292 in _ZSt5applyIZN4llvm6thread18GenericThreadProxyISt5tupleIJPFvPvEPN12_GLOBAL__N_121RunSafelyOnThreadInfoEEEEEvS4_EUlOT_DpOT0_E_RSA_EDcSC_OT0_ (__f=..., __t=std::tuple containing = {...})
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/tuple:1671
#40 0x00007ffff35b0251 in llvm::thread::GenericThreadProxy<std::tuple<void (*)(void*), (anonymous namespace)::RunSafelyOnThreadInfo*> > (Ptr=0x7fffdc015580) at /local/clang/llvm2/llvm-project/llvm/include/llvm/Support/thread.h:41
#41 0x00007ffff35afee5 in llvm::thread::ThreadProxy<std::tuple<void (*)(void*), (anonymous namespace)::RunSafelyOnThreadInfo*> > (Ptr=0x7fffdc015580) at /local/clang/llvm2/llvm-project/llvm/include/llvm/Support/thread.h:55
#42 0x00007ffff7bbb6db in start_thread (arg=0x7fffe2c9c700) at pthread_create.c:463
#43 0x00007ffff26e261f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

[balazske]

I have a fix for the AArch64 problems in test ImportCorrectTemplatedDecl, dcl-58-cpp, and the "Declaration not emitted!" assertion. I can not find at which tests the error: reference to 'std' is ambiguous appears. Probably the AArch64 (and Arm) tests could be run again with the current fixes applied.

[vabridgers]

@mizvekov, will you be picking this change up and finishing this, or do you mind if I have a go at finishing this patch? Thanks

[erichkeane]

In D136886#4087953, @vabridgers wrote:

@mizvekov, will you be picking this change up and finishing this, or do you mind if I have a go at finishing this patch? Thanks

@mizvekov has been away from the community for a while, and I'm not sure when he's coming back. I'd suspect that if you have interest in fixing it, that you're welcome to take it over!