This is an archive of the discontinued LLVM Phabricator instance.

[lldb][clang][modern-type-lookup] Use ASTImporterSharedState in ExternalASTMerger
ClosedPublic

Authored by teemperor on Sep 27 2019, 7:02 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Sep 27 2019, 7:02 AM

Just putting this up as I don't want to merge that before the weekend. Patch itself is obvious.

shafik added inline comments.Sep 27 2019, 4:01 PM
clang/include/clang/AST/ExternalASTMerger.h
92 ↗(On Diff #222167)

Can you add a comment explaining what this is and why we need it and how it relates to the ASTImpoter.

It is not obvious just looking the local changes what effect adding this has.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 30 2019, 1:51 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2019, 1:51 AM
teemperor marked 2 inline comments as done.Sep 30 2019, 1:56 AM
teemperor added inline comments.
clang/include/clang/AST/ExternalASTMerger.h
92 ↗(On Diff #222167)

Added a comment how we used the shared state in all created ASTImporters, but the exact effects of having the shared state are explained in the class itself (e.g. helping the lookup in some cases).

Raphael, thanks for working on this. Overall, the changes look good to me, but please see my comment for the constructor.

cfe/trunk/lib/AST/ExternalASTMerger.cpp
320

For safety, it would make sense to check that there isn't any ExternalASTSource attached to Target.AST yet.
(assert(Target.AST.getExternalSource() == nullptr) ?)

If that is not the case then the constructor of the ASTImporterSpecificLookup (inited from the SharedState ctor) will traverse through the decls and that would result in consulting with the existing and already set ExternalSource. And that is something that we should avoid because we are right now in the middle of setting up the ExternalSource.

This change has broken a test in the Arm and AArch64 buildbots. Specificallyclang/test/Import/cxx-anon-namespace/test.cpp For example (http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/10542/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Atest.cpp)
From comparing the results on X86_64 and AArch64 it looks like the key difference is that the AArch64 has an extra NamespaceDecl possibly from something implicitly added in Arm and AArch64. I've quoted the output from AArch64 and X86_64, note the AArch64 has implicit __SVInt8_t ... like types followed by a NamespaceDecl.

Can you take a look? I guess the test could be changed, but I don't know whether this change was expected to change anything here?

AArch64

Command Output (stdout):
--
TranslationUnitDecl 0x1fae9ec8 <<invalid sloc>> <invalid sloc>
|-TypedefDecl 0x1faea920 <<invalid sloc>> <invalid sloc> implicit __int128_t '__int128'
| `-BuiltinType 0x1faea460 '__int128'
|-TypedefDecl 0x1faea990 <<invalid sloc>> <invalid sloc> implicit __int128_t '__int128'
| `-BuiltinType 0x1faea460 '__int128'
|-TypedefDecl 0x1faeaa00 <<invalid sloc>> <invalid sloc> implicit __uint128_t 'unsigned __int128'
| `-BuiltinType 0x1faea480 'unsigned __int128'
|-TypedefDecl 0x1faeaa70 <<invalid sloc>> <invalid sloc> implicit __uint128_t 'unsigned __int128'
| `-BuiltinType 0x1faea480 'unsigned __int128'
|-TypedefDecl 0x1faeadf8 <<invalid sloc>> <invalid sloc> implicit __NSConstantString '__NSConstantString_tag'
| `-RecordType 0x1faeab70 '__NSConstantString_tag'
|   `-CXXRecord 0x1faeaac8 '__NSConstantString_tag'
|-CXXRecordDecl 0x1fb111a0 <<invalid sloc>> <invalid sloc> implicit <undeserialized declarations> struct __NSConstantString_tag definition
| |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
| | |-DefaultConstructor exists trivial needs_implicit
| | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveConstructor exists simple trivial needs_implicit
| | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveAssignment exists simple trivial needs_implicit
| | `-Destructor simple irrelevant trivial needs_implicit
| `-TypeVisibilityAttr 0x1fb11270 <<invalid sloc>> Implicit Default
|-TypedefDecl 0x1fb11340 <<invalid sloc>> <invalid sloc> implicit __NSConstantString '__NSConstantString_tag'
| `-RecordType 0x1fb11250 '__NSConstantString_tag'
|   `-CXXRecord 0x1fb111a0 '__NSConstantString_tag'
|-TypedefDecl 0x1fb113a8 <<invalid sloc>> <invalid sloc> implicit __SVInt8_t '__SVInt8_t'
| `-BuiltinType 0x1faea720 '__SVInt8_t'
|-TypedefDecl 0x1fb11410 <<invalid sloc>> <invalid sloc> implicit __SVInt8_t '__SVInt8_t'
| `-BuiltinType 0x1faea720 '__SVInt8_t'
|-TypedefDecl 0x1fb11478 <<invalid sloc>> <invalid sloc> implicit __SVInt16_t '__SVInt16_t'
| `-BuiltinType 0x1faea740 '__SVInt16_t'
|-TypedefDecl 0x1fb114e0 <<invalid sloc>> <invalid sloc> implicit __SVInt16_t '__SVInt16_t'
| `-BuiltinType 0x1faea740 '__SVInt16_t'
|-TypedefDecl 0x1fb11548 <<invalid sloc>> <invalid sloc> implicit __SVInt32_t '__SVInt32_t'
| `-BuiltinType 0x1faea760 '__SVInt32_t'
|-TypedefDecl 0x1fb115b0 <<invalid sloc>> <invalid sloc> implicit __SVInt32_t '__SVInt32_t'
| `-BuiltinType 0x1faea760 '__SVInt32_t'
|-TypedefDecl 0x1fb11618 <<invalid sloc>> <invalid sloc> implicit __SVInt64_t '__SVInt64_t'
| `-BuiltinType 0x1faea780 '__SVInt64_t'
|-TypedefDecl 0x1fb11680 <<invalid sloc>> <invalid sloc> implicit __SVInt64_t '__SVInt64_t'
| `-BuiltinType 0x1faea780 '__SVInt64_t'
|-TypedefDecl 0x1fb116e8 <<invalid sloc>> <invalid sloc> implicit __SVUint8_t '__SVUint8_t'
| `-BuiltinType 0x1faea7a0 '__SVUint8_t'
|-TypedefDecl 0x1fb11750 <<invalid sloc>> <invalid sloc> implicit __SVUint8_t '__SVUint8_t'
| `-BuiltinType 0x1faea7a0 '__SVUint8_t'
|-TypedefDecl 0x1fb117b8 <<invalid sloc>> <invalid sloc> implicit __SVUint16_t '__SVUint16_t'
| `-BuiltinType 0x1faea7c0 '__SVUint16_t'
|-TypedefDecl 0x1fb11820 <<invalid sloc>> <invalid sloc> implicit __SVUint16_t '__SVUint16_t'
| `-BuiltinType 0x1faea7c0 '__SVUint16_t'
|-TypedefDecl 0x1fb11888 <<invalid sloc>> <invalid sloc> implicit __SVUint32_t '__SVUint32_t'
| `-BuiltinType 0x1faea7e0 '__SVUint32_t'
|-TypedefDecl 0x1fb118f0 <<invalid sloc>> <invalid sloc> implicit __SVUint32_t '__SVUint32_t'
| `-BuiltinType 0x1faea7e0 '__SVUint32_t'
|-TypedefDecl 0x1fb11958 <<invalid sloc>> <invalid sloc> implicit __SVUint64_t '__SVUint64_t'
| `-BuiltinType 0x1faea800 '__SVUint64_t'
|-TypedefDecl 0x1fb119c0 <<invalid sloc>> <invalid sloc> implicit __SVUint64_t '__SVUint64_t'
| `-BuiltinType 0x1faea800 '__SVUint64_t'
|-TypedefDecl 0x1fb11a28 <<invalid sloc>> <invalid sloc> implicit __SVFloat16_t '__SVFloat16_t'
| `-BuiltinType 0x1faea820 '__SVFloat16_t'
|-TypedefDecl 0x1fb11a90 <<invalid sloc>> <invalid sloc> implicit __SVFloat16_t '__SVFloat16_t'
| `-BuiltinType 0x1faea820 '__SVFloat16_t'
|-TypedefDecl 0x1fb11af8 <<invalid sloc>> <invalid sloc> implicit __SVFloat32_t '__SVFloat32_t'
| `-BuiltinType 0x1faea840 '__SVFloat32_t'
|-TypedefDecl 0x1fb11b60 <<invalid sloc>> <invalid sloc> implicit __SVFloat32_t '__SVFloat32_t'
| `-BuiltinType 0x1faea840 '__SVFloat32_t'
|-TypedefDecl 0x1fb11bc8 <<invalid sloc>> <invalid sloc> implicit __SVFloat64_t '__SVFloat64_t'
| `-BuiltinType 0x1faea860 '__SVFloat64_t'
|-TypedefDecl 0x1fb11c30 <<invalid sloc>> <invalid sloc> implicit __SVFloat64_t '__SVFloat64_t'
| `-BuiltinType 0x1faea860 '__SVFloat64_t'
|-TypedefDecl 0x1fb11c98 <<invalid sloc>> <invalid sloc> implicit __SVBool_t '__SVBool_t'
| `-BuiltinType 0x1faea880 '__SVBool_t'
|-TypedefDecl 0x1fb11d00 <<invalid sloc>> <invalid sloc> implicit __SVBool_t '__SVBool_t'
| `-BuiltinType 0x1faea880 '__SVBool_t'
|-TypedefDecl 0x1fb11da0 <<invalid sloc>> <invalid sloc> implicit __builtin_ms_va_list 'char *'
| `-PointerType 0x1fb11d60 'char *'
|   `-BuiltinType 0x1fae9f60 'char'
|-TypedefDecl 0x1fb11e10 <<invalid sloc>> <invalid sloc> implicit __builtin_ms_va_list 'char *'
| `-PointerType 0x1fb11d60 'char *'
|   `-BuiltinType 0x1fae9f60 'char'
|-TypedefDecl 0x1fb14f90 <<invalid sloc>> <invalid sloc> implicit __builtin_va_list 'std::__va_list'
| `-RecordType 0x1fb11f10 'std::__va_list'
|   `-CXXRecord 0x1fb11e68 '__va_list'
|-NamespaceDecl 0x1fb14fe8 <<invalid sloc>> <invalid sloc> implicit std
| `-CXXRecordDecl 0x1fb15070 <<invalid sloc>> <invalid sloc> implicit <undeserialized declarations> struct __va_list definition
|   |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
|   | |-DefaultConstructor exists trivial needs_implicit
|   | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
|   | |-MoveConstructor exists simple trivial needs_implicit
|   | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
|   | |-MoveAssignment exists simple trivial needs_implicit
|   | `-Destructor simple irrelevant trivial needs_implicit
|   `-TypeVisibilityAttr 0x1fb15140 <<invalid sloc>> Implicit Default
|-TypedefDecl 0x1fb15210 <<invalid sloc>> <invalid sloc> implicit __builtin_va_list 'std::__va_list'
| `-RecordType 0x1fb15120 'std::__va_list'
|   `-CXXRecord 0x1fb15070 '__va_list'
|-NamespaceDecl 0x1fb152b8 </home/peter.smith/llvm/repos/clang/test/Import/cxx-anon-namespace/Inputs/F.cpp:1:1, <invalid sloc>> col:11
| |-NamespaceDecl 0x1fb15398 <line:21:1, <invalid sloc>> col:11
| | `-FunctionDecl 0x1fb15de0 <line:22:1, line:23:1> line:22:6 used func4 'void ()'
| |   `-CompoundStmt 0x1fb15e80 <col:14, line:23:1>
| |-UsingDirectiveDecl 0x1fb15420 <line:21:11, <invalid sloc>> <invalid sloc> implicit Namespace 0x1fb15398 ''
| `-FunctionDecl 0x1fb15598 <line:2:1, line:3:1> line:2:6 used func1 'void ()'
|   `-CompoundStmt 0x1fb15638 <col:14, line:3:1>
|-UsingDirectiveDecl 0x1fb15340 <line:1:11, <invalid sloc>> <invalid sloc> implicit Namespace 0x1fb152b8 ''
|-FunctionDecl 0x1fb15478 </home/peter.smith/llvm/repos/clang/test/Import/cxx-anon-namespace/test.cpp:41:1, line:46:1> line:41:6 expr 'void ()'
| `-CompoundStmt 0x1fb486c0 <col:13, line:46:1>
|   |-CallExpr 0x1fb15710 <line:42:3, col:9> 'void'
|   | `-ImplicitCastExpr 0x1fb156f8 <col:3> 'void (*)()' <FunctionToPointerDecay>
|   |   `-DeclRefExpr 0x1fb15690 <col:3> 'void ()' lvalue Function 0x1fb15598 'func1' 'void ()'
|   |-CallExpr 0x1fb15a00 <line:43:3, col:26> 'void'
|   | `-ImplicitCastExpr 0x1fb159e8 <col:3, col:20> 'void (*)()' <FunctionToPointerDecay>
|   |   `-DeclRefExpr 0x1fb159a0 <col:3, col:20> 'void ()' lvalue Function 0x1fb158d0 'func2' 'void ()'
|   |-CallExpr 0x1fb15da0 <line:44:3, col:43> 'void'
|   | `-ImplicitCastExpr 0x1fb15d88 <col:3, col:37> 'void (*)()' <FunctionToPointerDecay>
|   |   `-DeclRefExpr 0x1fb15d40 <col:3, col:37> 'void ()' lvalue Function 0x1fb15c60 'func3' 'void ()'
|   `-CallExpr 0x1fb486a0 <line:45:3, col:9> 'void'
|     `-ImplicitCastExpr 0x1fb48688 <col:3> 'void (*)()' <FunctionToPointerDecay>
|       `-DeclRefExpr 0x1fb48650 <col:3> 'void ()' lvalue Function 0x1fb15de0 'func4' 'void ()'
|-NamespaceDecl 0x1fb15730 </home/peter.smith/llvm/repos/clang/test/Import/cxx-anon-namespace/Inputs/F.cpp:6:1, <invalid sloc>> col:11 test_namespace1
| |-NamespaceDecl 0x1fb157d0 <line:7:1, <invalid sloc>> col:11
| | `-FunctionDecl 0x1fb158d0 <line:8:1, col:15> col:6 used func2 'void ()'
| |   `-CompoundStmt 0x1fb15970 <col:14, col:15>
| `-UsingDirectiveDecl 0x1fb15858 <line:7:11, <invalid sloc>> <invalid sloc> implicit Namespace 0x1fb157d0 ''
`-NamespaceDecl 0x1fb15a20 <line:12:1, <invalid sloc>> col:11 test_namespace2
  |-NamespaceDecl 0x1fb15ac0 <line:13:1, <invalid sloc>> col:11
  | `-NamespaceDecl 0x1fb15ba0 <line:14:1, <invalid sloc>> col:11 test_namespace3
  |   `-FunctionDecl 0x1fb15c60 <line:15:1, col:15> col:6 used func3 'void ()'
  |     `-CompoundStmt 0x1fb15d00 <col:14, col:15>
  `-UsingDirectiveDecl 0x1fb15b48 <line:13:11, <invalid sloc>> <invalid sloc> implicit Namespace 0x1fb15ac0 ''

X86_64

TranslationUnitDecl 0x2515818 <<invalid sloc>> <invalid sloc>
|-TypedefDecl 0x25160f0 <<invalid sloc>> <invalid sloc> implicit __int128_t '__int128'
| `-BuiltinType 0x2515db0 '__int128'
|-TypedefDecl 0x2516160 <<invalid sloc>> <invalid sloc> implicit __int128_t '__int128'
| `-BuiltinType 0x2515db0 '__int128'
|-TypedefDecl 0x25161d0 <<invalid sloc>> <invalid sloc> implicit __uint128_t 'unsigned __int128'
| `-BuiltinType 0x2515dd0 'unsigned __int128'
|-TypedefDecl 0x2516240 <<invalid sloc>> <invalid sloc> implicit __uint128_t 'unsigned __int128'
| `-BuiltinType 0x2515dd0 'unsigned __int128'
|-TypedefDecl 0x25165c8 <<invalid sloc>> <invalid sloc> implicit __NSConstantString '__NSConstantString_tag'
| `-RecordType 0x2516340 '__NSConstantString_tag'
|   `-CXXRecord 0x2516298 '__NSConstantString_tag'
|-CXXRecordDecl 0x2516620 <<invalid sloc>> <invalid sloc> implicit <undeserialized declarations> struct __NSConstantString_tag definition
| |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
| | |-DefaultConstructor exists trivial needs_implicit
| | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveConstructor exists simple trivial needs_implicit
| | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveAssignment exists simple trivial needs_implicit
| | `-Destructor simple irrelevant trivial needs_implicit
| `-TypeVisibilityAttr 0x25166f0 <<invalid sloc>> Implicit Default
|-TypedefDecl 0x253ca70 <<invalid sloc>> <invalid sloc> implicit __NSConstantString '__NSConstantString_tag'
| `-RecordType 0x25166d0 '__NSConstantString_tag'
|   `-CXXRecord 0x2516620 '__NSConstantString_tag'
|-TypedefDecl 0x253cb10 <<invalid sloc>> <invalid sloc> implicit __builtin_ms_va_list 'char *'
| `-PointerType 0x253cad0 'char *'
|   `-BuiltinType 0x25158b0 'char'
|-TypedefDecl 0x253cb80 <<invalid sloc>> <invalid sloc> implicit __builtin_ms_va_list 'char *'
| `-PointerType 0x253cad0 'char *'
|   `-BuiltinType 0x25158b0 'char'
|-TypedefDecl 0x253cef8 <<invalid sloc>> <invalid sloc> implicit __builtin_va_list '__va_list_tag [1]'
| `-ConstantArrayType 0x253cea0 '__va_list_tag [1]' 1 
|   `-RecordType 0x253cc80 '__va_list_tag'
|     `-CXXRecord 0x253cbd8 '__va_list_tag'
|-CXXRecordDecl 0x253cf50 <<invalid sloc>> <invalid sloc> implicit <undeserialized declarations> struct __va_list_tag definition
| |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
| | |-DefaultConstructor exists trivial needs_implicit
| | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveConstructor exists simple trivial needs_implicit
| | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveAssignment exists simple trivial needs_implicit
| | `-Destructor simple irrelevant trivial needs_implicit
| `-TypeVisibilityAttr 0x253d020 <<invalid sloc>> Implicit Default
|-TypedefDecl 0x253d138 <<invalid sloc>> <invalid sloc> implicit __builtin_va_list '__va_list_tag [1]'
| `-ConstantArrayType 0x253d0e0 '__va_list_tag [1]' 1 
|   `-RecordType 0x253d000 '__va_list_tag'
|     `-CXXRecord 0x253cf50 '__va_list_tag'
|-NamespaceDecl 0x253d1d8 </work/llvm-project/clang/test/Import/cxx-anon-namespace/Inputs/F.cpp:1:1, <invalid sloc>> col:11
| |-NamespaceDecl 0x253d2b8 <line:21:1, <invalid sloc>> col:11
| | `-FunctionDecl 0x256d0d8 <line:22:1, line:23:1> line:22:6 used func4 'void ()'
| |   `-CompoundStmt 0x256d178 <col:14, line:23:1>
| |-UsingDirectiveDecl 0x253d340 <line:21:11, <invalid sloc>> <invalid sloc> implicit Namespace 0x253d2b8 ''
| `-FunctionDecl 0x253d4b8 <line:2:1, line:3:1> line:2:6 used func1 'void ()'
|   `-CompoundStmt 0x253d558 <col:14, line:3:1>
|-UsingDirectiveDecl 0x253d260 <line:1:11, <invalid sloc>> <invalid sloc> implicit Namespace 0x253d1d8 ''
|-FunctionDecl 0x253d398 </work/llvm-project/clang/test/Import/cxx-anon-namespace/test.cpp:41:1, line:46:1> line:41:6 expr 'void ()'
| `-CompoundStmt 0x256d240 <col:13, line:46:1>
|   |-CallExpr 0x253d630 <line:42:3, col:9> 'void'
|   | `-ImplicitCastExpr 0x253d618 <col:3> 'void (*)()' <FunctionToPointerDecay>
|   |   `-DeclRefExpr 0x253d5b0 <col:3> 'void ()' lvalue Function 0x253d4b8 'func1' 'void ()'
|   |-CallExpr 0x253d920 <line:43:3, col:26> 'void'
|   | `-ImplicitCastExpr 0x253d908 <col:3, col:20> 'void (*)()' <FunctionToPointerDecay>
|   |   `-DeclRefExpr 0x253d8c0 <col:3, col:20> 'void ()' lvalue Function 0x253d7f0 'func2' 'void ()'
|   |-CallExpr 0x256d098 <line:44:3, col:43> 'void'
|   | `-ImplicitCastExpr 0x256d080 <col:3, col:37> 'void (*)()' <FunctionToPointerDecay>
|   |   `-DeclRefExpr 0x256d038 <col:3, col:37> 'void ()' lvalue Function 0x256cf58 'func3' 'void ()'
|   `-CallExpr 0x256d220 <line:45:3, col:9> 'void'
|     `-ImplicitCastExpr 0x256d208 <col:3> 'void (*)()' <FunctionToPointerDecay>
|       `-DeclRefExpr 0x256d1d0 <col:3> 'void ()' lvalue Function 0x256d0d8 'func4' 'void ()'
|-NamespaceDecl 0x253d650 </work/llvm-project/clang/test/Import/cxx-anon-namespace/Inputs/F.cpp:6:1, <invalid sloc>> col:11 test_namespace1
| |-NamespaceDecl 0x253d6f0 <line:7:1, <invalid sloc>> col:11
| | `-FunctionDecl 0x253d7f0 <line:8:1, col:15> col:6 used func2 'void ()'
| |   `-CompoundStmt 0x253d890 <col:14, col:15>
| `-UsingDirectiveDecl 0x253d778 <line:7:11, <invalid sloc>> <invalid sloc> implicit Namespace 0x253d6f0 ''
`-NamespaceDecl 0x253d940 <line:12:1, <invalid sloc>> col:11 test_namespace2
  |-NamespaceDecl 0x253d9e0 <line:13:1, <invalid sloc>> col:11
  | `-NamespaceDecl 0x256ce98 <line:14:1, <invalid sloc>> col:11 test_namespace3
  |   `-FunctionDecl 0x256cf58 <line:15:1, col:15> col:6 used func3 'void ()'
  |     `-CompoundStmt 0x256cff8 <col:14, col:15>
  `-UsingDirectiveDecl 0x256ce40 <line:13:11, <invalid sloc>> <invalid sloc> implicit Namespace 0x253d9e0 ''