[Lldb-commits] [PATCH] D54863: [ASTImporter] Set MustBuildLookupTable on PrimaryContext

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

> Can you write an lldb test for this?

There is already an existing test for that:

  2: test_expr_dwarf (TestSharedLib.SharedLibTestCase)
 Test that types work when defined in a shared library and forward-declared 
in the main executable ... python: 
../../git/llvm/tools/clang/include/clang/AST/DeclBase.h:2298: void 
clang::DeclContext::setMustBuildLookupTable(): Assertion `this == 
getPrimaryContext() && "should only be called on primary context"' failed.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54863/new/

https://reviews.llvm.org/D54863



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54863: [ASTImporter] Set MustBuildLookupTable on PrimaryContext

[Gabor Marton via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL347575: [ASTImporter] Set MustBuildLookupTable on 
PrimaryContext (authored by martong, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54863/new/

https://reviews.llvm.org/D54863

Files:
  lldb/trunk/source/Symbol/ClangASTImporter.cpp


Index: lldb/trunk/source/Symbol/ClangASTImporter.cpp
===
--- lldb/trunk/source/Symbol/ClangASTImporter.cpp
+++ lldb/trunk/source/Symbol/ClangASTImporter.cpp
@@ -1050,7 +1050,7 @@
 TagDecl *to_tag_decl = dyn_cast(to);
 
 to_tag_decl->setHasExternalLexicalStorage();
-to_tag_decl->setMustBuildLookupTable();
+to_tag_decl->getPrimaryContext()->setMustBuildLookupTable();
 
 if (log)
   log->Printf(


Index: lldb/trunk/source/Symbol/ClangASTImporter.cpp
===
--- lldb/trunk/source/Symbol/ClangASTImporter.cpp
+++ lldb/trunk/source/Symbol/ClangASTImporter.cpp
@@ -1050,7 +1050,7 @@
 TagDecl *to_tag_decl = dyn_cast(to);
 
 to_tag_decl->setHasExternalLexicalStorage();
-to_tag_decl->setMustBuildLookupTable();
+to_tag_decl->getPrimaryContext()->setMustBuildLookupTable();
 
 if (log)
   log->Printf(
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54863: [ASTImporter] Set MustBuildLookupTable on PrimaryContext

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

Thank you all for the review! :)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54863/new/

https://reviews.llvm.org/D54863



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D54863: [ASTImporter] Set MustBuildLookupTable on PrimaryContext

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

>   Is this failing only on linux? Do you happen to have a buildbot link that 
> shows the failure?

This LLDB patch is to prevent the buildbot failure when we commit this 
Clang/ASTImporter patch : https://reviews.llvm.org/D53655


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54863/new/

https://reviews.llvm.org/D54863



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D57956: [www] Add ASTImporter fuzzer project.

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

> This project is about writing a fuzzer to proactively discover these

  ASTImporter bugs and provide minimal reproducers which make understanding
  and fixing the underlying bug easier.

Raphael, this is a good goal, I can only support this!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57956/new/

https://reviews.llvm.org/D57956



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59485: [ASTImporter] Allow adding a import strategy to the ASTImporter

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

> We can't reliable import templates with the ASTImporter, so actually 
> reconstructing the template in the debug info AST and then importing it 
> doesn't work.

Could you please elaborate how the import of templates fails in ASTImporter? Is 
it because the AST you build from the DWARF is incomplete? Or is there a lookup 
problem? Is there an assertion in one of the CodeGen functions once the import 
is finished?
If we could fix that error in the ASTImporter then other clients of it (like 
CTU) could benefit from the fix too. Perhaps, the best would be to have a test 
which reproduces the template import related errors, maybe a lit test with 
clang-import-test?

If the crux of the problem is because of the incomplete AST from DWARF then 
probably we cannot fix that here in clang::ASTImporter. However, if that is not 
the case then I think we should try to fix the error here instead of 
duplicating lookup and import logic in LLDB. (As for the lookup problems, quite 
recently I just  have discovered, that lookup during expression evaluation in 
LLDB does not work properly, I'll communicate this in a separate email/patch.)

> It would also be much slower.

Could you please explain why it would be slower?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59485/new/

https://reviews.llvm.org/D59485



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

> @martong It's not related to lookup or anything like that, it's just that we 
> don't have enough information in our debug info AST to allow people to use 
> meta programming. The custom logic we have in LLDB looks into the std C++ 
> module to fill out these gaps in the AST by hand when they are used in an 
> expression.
> 
> The remark about the alternative being slow just means that fixing all the 
> templates we have in our debug info AST seems like a costly approach. I'm 
> also not sure how well it would work, as I never tried mixing a C++ module in 
> an ASTContext that isn't currently being parsed by Clang.

Well, I still don't understand how LLDB synthesis the AST. 
So you have a C++ module in a .pcm file. This means you could create an AST 
from that with ASTReader from it's .clang_ast section, right? In that case the 
AST should be complete with all type information. If this was the case then I 
don't see why it is not possible to use clang::ASTImporter to import templates 
and specializations, since we do exactly that in CTU.

Or do you guys create the AST from the debug info, from the .debug_info section 
of .pcm module file? And this is why AST is incomplete? I've got this from 
https://www.youtube.com/watch?v=EgkZ8PTNSHQ&list=PL85Cf-ok7HpppFWlp2wX_-A1dkyFVlaEB&index=2&t=5s
If this is the case, then comes my naiive quiestion: Why don't you use the 
ASTReader?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59485/new/

https://reviews.llvm.org/D59485



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

In D59485#1439390 , @teemperor wrote:

> > Well, I still don't understand how LLDB synthesis the AST. 
> >  So you have a C++ module in a .pcm file. This means you could create an 
> > AST from that with ASTReader from it's .clang_ast section, right? In that 
> > case the AST should be complete with all type information. If this was the 
> > case then I don't see why it is not possible to use clang::ASTImporter to 
> > import templates and specializations, since we do exactly that in CTU.
> > 
> > Or do you guys create the AST from the debug info, from the .debug_info 
> > section of .pcm module file? And this is why AST is incomplete? I've got 
> > this from 
> > https://youtu.be/EgkZ8PTNSHQ?list=PL85Cf-ok7HpppFWlp2wX_-A1dkyFVlaEB&t=1565
> >  If this is the case, then comes my naiive quiestion: Why don't you use the 
> > ASTReader?
>
> There are no C++ modules involved in our use case beside the generic `std` 
> module. No user-written code is read from a module and we don't have any PCM 
> file beside the `std` module we build for the expression evaluator.
>
> We use the ASTReader to directly read the `std` module contents into the 
> expression evaluation ASTContext, but this doesn't give us the decls for the 
> instantiations the user has made (e.g. `std::vector`). We only have 
> these user instantiations in the 'normal' debug info where we have a rather 
> minimal AST (again, no modules are not involved in building this debug info 
> AST). The `InternalImport` in the LLDB patch just stitches the module AST and 
> the debug info AST together when we import something that we also have (in 
> better quality from the C++ module) in the target ASTContext.


Thank you for the explanation.
Now I understand you directly create specializations from the std module and 
intercept the import to avoid importing broken specializations from the debug 
info, this makes sense.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59485/new/

https://reviews.llvm.org/D59485



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59537: Instantiate 'std' templates explicitly in the expression evaluator

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: lldb/include/lldb/Symbol/StdModuleHandler.h:22
+/// process is setup to use C++ modules and has the 'std' module attached.
+class StdModuleHandler {
+  clang::ASTImporter *m_importer = nullptr;

I think it would worth to extend this description with something like this:  

`tryInstantiateStdTemplate` directly *creates* (or finds existing) 
specializations from the std module and intercept the import from the debug 
info AST to avoid having broken specializations in the expression evaluation 
context.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59537/new/

https://reviews.llvm.org/D59537



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

In D59485#1439570 , @martong wrote:

> In D59485#1439390 , @teemperor wrote:
>
> > > Well, I still don't understand how LLDB synthesis the AST. 
> > >  So you have a C++ module in a .pcm file. This means you could create an 
> > > AST from that with ASTReader from it's .clang_ast section, right? In that 
> > > case the AST should be complete with all type information. If this was 
> > > the case then I don't see why it is not possible to use 
> > > clang::ASTImporter to import templates and specializations, since we do 
> > > exactly that in CTU.
> > > 
> > > Or do you guys create the AST from the debug info, from the .debug_info 
> > > section of .pcm module file? And this is why AST is incomplete? I've got 
> > > this from 
> > > https://youtu.be/EgkZ8PTNSHQ?list=PL85Cf-ok7HpppFWlp2wX_-A1dkyFVlaEB&t=1565
> > >  If this is the case, then comes my naiive quiestion: Why don't you use 
> > > the ASTReader?
> >
> > There are no C++ modules involved in our use case beside the generic `std` 
> > module. No user-written code is read from a module and we don't have any 
> > PCM file beside the `std` module we build for the expression evaluator.
> >
> > We use the ASTReader to directly read the `std` module contents into the 
> > expression evaluation ASTContext, but this doesn't give us the decls for 
> > the instantiations the user has made (e.g. `std::vector`). We only 
> > have these user instantiations in the 'normal' debug info where we have a 
> > rather minimal AST (again, no modules are not involved in building this 
> > debug info AST). The `InternalImport` in the LLDB patch just stitches the 
> > module AST and the debug info AST together when we import something that we 
> > also have (in better quality from the C++ module) in the target ASTContext.
>
>
> Thank you for the explanation.
>  Now I understand you directly create specializations from the std module and 
> intercept the import to avoid importing broken specializations from the debug 
> info, this makes sense.


Really, just one last question to see the whole picture: If clang::ASTImporter 
were capable of handling a specialization/instantiation with missing info then 
we could use that. So the reason for this interception is that 
clang::ASTImporter::VisitClassTemplateSpecializationDecl cannot handle the 
specialization it receives because that or its dependent nodes has too many 
missing data, right?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59485/new/

https://reviews.llvm.org/D59485



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59537: Instantiate 'std' templates explicitly in the expression evaluator

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: lldb/source/Symbol/StdModuleHandler.cpp:242
+// Instantiate the template.
+found_decl = ClassTemplateSpecializationDecl::Create(
+m_sema->getASTContext(),

Is there any guarantee that the before any subsequent 
clang::ASTImporter::Import call this newly created node is registered as 
imported (via ASTImporter::MapImported)?

The reason why I am asking this is because we have to enforce in 
clang::ASTImporter that after every Decl is created then immediately it is 
registered as an imported Decl, see `ASTImporter::GetImportedOrCreateDecl`. 
This we we make sure that no subsequent import calls will create the same node 
again.
The cycles in the AST are handled properly only this way.

This is one reason which makes me worried about exposing the import mechanism 
via `ImportInternal`.  Would it be working if `GetImportedOrCreateDecl` was 
virtual and overridden here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59537/new/

https://reviews.llvm.org/D59537



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59537: Instantiate 'std' templates explicitly in the expression evaluator

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: lldb/source/Symbol/StdModuleHandler.cpp:242
+// Instantiate the template.
+found_decl = ClassTemplateSpecializationDecl::Create(
+m_sema->getASTContext(),

martong wrote:
> Is there any guarantee that the before any subsequent 
> clang::ASTImporter::Import call this newly created node is registered as 
> imported (via ASTImporter::MapImported)?
> 
> The reason why I am asking this is because we have to enforce in 
> clang::ASTImporter that after every Decl is created then immediately it is 
> registered as an imported Decl, see `ASTImporter::GetImportedOrCreateDecl`. 
> This we we make sure that no subsequent import calls will create the same 
> node again.
> The cycles in the AST are handled properly only this way.
> 
> This is one reason which makes me worried about exposing the import mechanism 
> via `ImportInternal`.  Would it be working if `GetImportedOrCreateDecl` was 
> virtual and overridden here?
> Would it be working if GetImportedOrCreateDecl was virtual and overridden 
> here?

Sorry, that would certainly not work, but perhaps a virtual hook inside that 
function could work...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59537/new/

https://reviews.llvm.org/D59537



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59826: [expression] Explicitly build the lookup table of any TagDecl's context

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

There is a lookup failure within the
ASTImporter during the expression evaluation. The clang::ASTImporter cannot
lookup previously created declarations thus it creates duplicated and redundant
declarations. These duplicated declarations then later can cause different
assertions.

More specifically, the problem occurs in the presence of a LinkageSpecDecl.
Here are the steps to reproduce:

- Compile a simple test binary (a.out)
- Launch LLDB with a.out
- set a breakpoint
- run
- expr -l objc++ -- @import Darwin; 3
- expr *fopen("/dev/zero", "w")

When we evaluate the `fopen` expression then we import the `__sFILE`
struct several times to the same ASTContext.
(To see these we have to attach to the original lldb process with
another debugger and add breakpoints to
ASTNodeImporter::VisitRecordDecl.)
After the first import we have this in the AST:

  TranslationUnitDecl
  `-LinkageSpecDecl 0x7ff9eab97618 <:76:1, line:78:1> 
line:76:8 C
|-CXXRecordDecl 0x7ff9eab97668
  
 col:16  struct __sFILE 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
`-TypedefDecl 0x7ff9eab977f8  col:3 FILE 'struct
  __sFILE':'__sFILE'
  `-ElaboratedType 0x7ff9eab977a0 'struct __sFILE' sugar
`-RecordType 0x7ff9eab97710 '__sFILE'
  `-CXXRecord 0x7ff9eab97668 '__sFILE'

During the second import the lookup fails thus we end up with a
duplicated CXXRecordDecl of `__sFILE` in the AST:

  TranslationUnitDecl
  |-LinkageSpecDecl 0x7ff9eab97618 <:76:1, line:78:1> 
line:76:8 C
  | |-CXXRecordDecl 0x7ff9eab97668
  
 col:16  struct __sFILE 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
  | `-TypedefDecl 0x7ff9eab977f8  col:3 FILE 'struct
  __sFILE':'__sFILE'
  |   `-ElaboratedType 0x7ff9eab977a0 'struct __sFILE' sugar
  | `-RecordType 0x7ff9eab97710 '__sFILE'
  |   `-CXXRecord 0x7ff9eab97668 '__sFILE'
  `-LinkageSpecDecl 0x7ff9eab97888 <:76:1, line:78:1> 
line:76:8 C
`-CXXRecordDecl 0x7ff9eab978d8
  
 col:16  struct __sFILE 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

The reason behind this is the following:
ASTImporter uses DeclContext::localUncachedLookup on a redecl context,
because we must not perform a lookup in a transparent context (there
is an assertion for that in DeclContext).
The redecl context of the `__sFILE` CXXRecordDecl is the
TranslationUnitDecl, while the normal DC is the LinkageSpecDecl (which
is a transparent context).
localUncachedLookup searches via the lookup table (lookupPtr) of the
given DeclContext, but only if that DC does not have external lexical
storage!
However, in the LLDB case it does have, so we end up in the Slow case
of the localUncachedLookup:

  // Slow case: grovel through the declarations in our chain looking for
  // matches.
  // FIXME: If we have lazy external declarations, this will not find them!
  // FIXME: Should we CollectAllContexts and walk them all here?
  for (Decl *D = FirstDecl; D; D = D->getNextDeclInContext()) {
if (auto *ND = dyn_cast(D))
  if (ND->getDeclName() == Name)
Results.push_back(ND);
  }

Since the DC is the redecl DC (TranslationUnitDecl) and not the normal
DC (LinkageSpecDecl), we will not find the CXXRecordDecl with the name
`__sFILE`.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59826/new/

https://reviews.llvm.org/D59826



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/list

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

In D59485#1444673 , @teemperor wrote:

> In D59485#1439628 , @martong wrote:
>
> > In D59485#1439570 , @martong wrote:
> >
> > > In D59485#1439390 , @teemperor 
> > > wrote:
> > >
> > > > > Well, I still don't understand how LLDB synthesis the AST. 
> > > > >  So you have a C++ module in a .pcm file. This means you could create 
> > > > > an AST from that with ASTReader from it's .clang_ast section, right? 
> > > > > In that case the AST should be complete with all type information. If 
> > > > > this was the case then I don't see why it is not possible to use 
> > > > > clang::ASTImporter to import templates and specializations, since we 
> > > > > do exactly that in CTU.
> > > > > 
> > > > > Or do you guys create the AST from the debug info, from the 
> > > > > .debug_info section of .pcm module file? And this is why AST is 
> > > > > incomplete? I've got this from 
> > > > > https://youtu.be/EgkZ8PTNSHQ?list=PL85Cf-ok7HpppFWlp2wX_-A1dkyFVlaEB&t=1565
> > > > >  If this is the case, then comes my naiive quiestion: Why don't you 
> > > > > use the ASTReader?
> > > >
> > > > There are no C++ modules involved in our use case beside the generic 
> > > > `std` module. No user-written code is read from a module and we don't 
> > > > have any PCM file beside the `std` module we build for the expression 
> > > > evaluator.
> > > >
> > > > We use the ASTReader to directly read the `std` module contents into 
> > > > the expression evaluation ASTContext, but this doesn't give us the 
> > > > decls for the instantiations the user has made (e.g. 
> > > > `std::vector`). We only have these user instantiations in the 
> > > > 'normal' debug info where we have a rather minimal AST (again, no 
> > > > modules are not involved in building this debug info AST). The 
> > > > `InternalImport` in the LLDB patch just stitches the module AST and the 
> > > > debug info AST together when we import something that we also have (in 
> > > > better quality from the C++ module) in the target ASTContext.
> > >
> > >
> > > Thank you for the explanation.
> > >  Now I understand you directly create specializations from the std module 
> > > and intercept the import to avoid importing broken specializations from 
> > > the debug info, this makes sense.
> >
> >
> > Really, just one last question to see the whole picture: If 
> > clang::ASTImporter were capable of handling a specialization/instantiation 
> > with missing info then we could use that. So the reason for this 
> > interception is that 
> > clang::ASTImporter::VisitClassTemplateSpecializationDecl cannot handle the 
> > specialization it receives because that or its dependent nodes has too many 
> > missing data, right?
>
>
> Feel free to ask! I'm just traveling so my internet access (and my reply 
> rate) is a bit low at the moment.
>
> Not sure if we can easily merge the logic of D59537 
>  into the ASTImporter. It has no logic at 
> all to actually determine if a source AST node has missing data but solely 
> relies on our knowledge that our AST in LLDB isn't very useful for std 
> templates. Also instantiating a template instead of importing an existing 
> instantiation seems like a very rare scenario. I'm not even sure how we would 
> test having a broken AST with Clang (the parser won't produce one, so we 
> would have to hack our own AST builder for broken nodes).
>
> If there is a reasonable way to get this logic into the ASTImporter I have no 
> objection against doing so. The only problem I see is that I can't come up 
> with a reasonable way of merging/testing the logic in the ASTImporter (and 
> what other application would benefit from it).


Raphael, thank you for your answer. This explanation makes so much more easier 
for me to understand the need for this patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59485/new/

https://reviews.llvm.org/D59485



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:590
+  new RedirectingImporter(ToContext, ToFileManager, FromContext,
+  FromFileManager, MinimalImport, LookupTabl));
+};

teemperor wrote:
> a_sidorin wrote:
> > LookupTable?
> Not sure if I can follow?
I think Alexey's comment relates to the parameter in 
`ASTImporterOptionSpecificTestBase::ImporterConstructor`. There is a typo in 
the name of the parameter an 'e' is missing: `ASTImporterLookupTable 
*LookupTabl`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59485/new/

https://reviews.llvm.org/D59485



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.

In D59485#1446950 , @a_sidorin wrote:

> Hello Raphael,
>  I think we should accept this change. I don't see an easy way to merge the 
> LLDB stuff into ASTImporter; also, this patch provides a good extension point 
> for ASTImporter since it is designed to be a parent class. @martong @shafik 
> Gabor, Shafik, what do you think?


Yes, I agree.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59485/new/

https://reviews.llvm.org/D59485



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59826: [expression] Explicitly build the lookup table of any TagDecl's context

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

Ping


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59826/new/

https://reviews.llvm.org/D59826



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59826: [expression] Explicitly build the lookup table of any TagDecl's context

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

In D59826#1451536 , @clayborg wrote:

> Any way to dump the AST in a test to verify we don't create multiple?


I think I might be able to use the `log` object to dump the AST and then from 
python it would be possible to check for duplicates. Is that a good direction?


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59826/new/

https://reviews.llvm.org/D59826



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133944: [lldb][tests][gmodules] Test for expression evaluator crash for types with template base class

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

Looks good to me!




Comment at: 
lldb/test/API/lang/cpp/gmodules/base-template-with-same-arg/module1.h:7
+struct ClassInMod1 {
+  ClassInMod3 VecInMod1;
+};

I was wondering whether you would have the crash if you used the directly the 
base class? I.e. if you had the `ClassInMod3Base VecInMod1` here and 
`ClassInMod3Base VecInMod2` in the other module.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133944/new/

https://reviews.llvm.org/D133944

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133945: [clang][ASTImporter] DeclContext::localUncachedLookup: Continue lookup into decl chain when regular lookup fails

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

LGTM, Thanks!




Comment at: clang/lib/AST/DeclBase.cpp:1781
   if (Name && !hasLazyLocalLexicalLookups() &&
   !hasLazyExternalLexicalLookups()) {
 if (StoredDeclsMap *Map = LookupPtr) {

Michael137 wrote:
> Michael137 wrote:
> > Could merge the two if-blocks now I suppose
> Nevermind, not true
Yeah, we set HasLazyExternalLexicalLookups to true in 
https://reviews.llvm.org/D61333 exactly for the reason to continue the lookup 
into decl chain.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:4927-4929
+  // Finds via linear search of its LexicalDC (A).
   FooLexicalDC->getRedeclContext()->localUncachedLookup(FooName, FoundDecls);
+  EXPECT_EQ(FoundDecls.size(), 1u);

Very good, now the behavior is the same that we have in case of the 
`ASTImporterLookupTable`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133945/new/

https://reviews.llvm.org/D133945

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D83972: Modify ImportDefiniton for ObjCInterfaceDecl so that we always the ImportDeclContext one we start the definition

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

> Unlike RecordDecl case which uses DefinitionCompleter to force 
> completeDefinition we don't seem to have a similar mechanism for 
> ObjCInterfaceDecl.

It is unfortunate that the AST does not have something similar for ObjC 
interfaces. 
So, this seems to be the right way unless we want to modify DeclObjC.h.
LGTM!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83972/new/

https://reviews.llvm.org/D83972



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:1737
 
 // If we are in the process of ImportDefinition(...) for a RecordDecl we
 // want to make sure that we are also completing each FieldDecl. There

`ImportDefinition(...)` here refers to `ASTImporter::ImportDefinition(Decl*)` 
and not any of the `ASTNodeImporter::ImportDefinition` funcitons. This is a 
very important distinction, because the former is part of the public interface, 
while the latter are private implementation functions. Could you please fix 
this in the comment?



Comment at: clang/lib/AST/ASTImporter.cpp:1755-1759
+  QualType FromTy = ArrayFrom->getElementType();
+  QualType ToTy = ArrayTo->getElementType();
+
+  FromRecordDecl = FromTy->getAsRecordDecl();
+  ToRecordDecl = ToTy->getAsRecordDecl();

labath wrote:
> What about 2- or n-dimensional arrays?
@labath, this is a very good question! And made me realize that D71378 is 
fundamentally flawed (@shafik, please take no offense). Let me explain.

So, with D71378, we added the `if (ImportedOrErr) { ... }` block to import 
definitions specifically of fields' Record types. But we forget to handle 
arrays. Now we may forget to handle multidimensional arrays ... and we may 
forget to handle other language constructs. So, we would finally end up in 
re-implementing the logic of `ASTNodeImporter::VisitFieldDecl`.

So all this should have been handled properly by the preceding import call of 
the FieldDecl! Here
```
1735: ExpectedDecl ImportedOrErr = import(From);
```
I have a suspicion that real reason why this import call fails in case of the 
public ASTImporter::ImportDefinition() is that we fail to drive through the 
import kind (`IDK_Everything`) during the import process.
Below we set IDK_Everything and start a complete import process.
```
  8784   if (auto *ToRecord = dyn_cast(To)) {
  8785 if (!ToRecord->getDefinition()) {
  8786   return Importer.ImportDefinition(   // 
ASTNodeImporter::ImportDefinition !
  8787   cast(FromDC), ToRecord,
  8788   ASTNodeImporter::IDK_Everything);
  8789 }
  8790   }
```
However, there may be many places where we fail to channel through that we want 
to do a complete import. E.g here
```
1957   ImportDefinitionIfNeeded(Base1.getType()->getAsCXXRecordDecl()))
```
we do another definition import and IDK_Everything is not set. So we may have a 
wrong kind of import since the "minimal" flag is set.

The thing is, it is really confusing and error-prone to have both the 
`ASTImporter::Minimal` flag and the `ImportDefinitionKind`. They seem to be in 
contradiction to each other some times.
I think we should get rid of the Minimal flag. And Kind should be either a full 
completion (IDK_Everythink) or just a minimal (IDK_Basic). So, I'd scrap the 
IDK_Default too. Alternatively we could have a Kind member in 
AST**//Node//**Importer.
I think we should go into this direction to avoid similar problems during 
CodeGen in the future. WDYT?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86660/new/

https://reviews.llvm.org/D86660

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:1755-1759
+  QualType FromTy = ArrayFrom->getElementType();
+  QualType ToTy = ArrayTo->getElementType();
+
+  FromRecordDecl = FromTy->getAsRecordDecl();
+  ToRecordDecl = ToTy->getAsRecordDecl();

labath wrote:
> shafik wrote:
> > martong wrote:
> > > labath wrote:
> > > > What about 2- or n-dimensional arrays?
> > > @labath, this is a very good question! And made me realize that D71378 is 
> > > fundamentally flawed (@shafik, please take no offense). Let me explain.
> > > 
> > > So, with D71378, we added the `if (ImportedOrErr) { ... }` block to 
> > > import definitions specifically of fields' Record types. But we forget to 
> > > handle arrays. Now we may forget to handle multidimensional arrays ... 
> > > and we may forget to handle other language constructs. So, we would 
> > > finally end up in re-implementing the logic of 
> > > `ASTNodeImporter::VisitFieldDecl`.
> > > 
> > > So all this should have been handled properly by the preceding import 
> > > call of the FieldDecl! Here
> > > ```
> > > 1735: ExpectedDecl ImportedOrErr = import(From);
> > > ```
> > > I have a suspicion that real reason why this import call fails in case of 
> > > the public ASTImporter::ImportDefinition() is that we fail to drive 
> > > through the import kind (`IDK_Everything`) during the import process.
> > > Below we set IDK_Everything and start a complete import process.
> > > ```
> > >   8784   if (auto *ToRecord = dyn_cast(To)) {
> > >   8785 if (!ToRecord->getDefinition()) {
> > >   8786   return Importer.ImportDefinition(   // 
> > > ASTNodeImporter::ImportDefinition !
> > >   8787   cast(FromDC), ToRecord,
> > >   8788   ASTNodeImporter::IDK_Everything);
> > >   8789 }
> > >   8790   }
> > > ```
> > > However, there may be many places where we fail to channel through that 
> > > we want to do a complete import. E.g here
> > > ```
> > > 1957   
> > > ImportDefinitionIfNeeded(Base1.getType()->getAsCXXRecordDecl()))
> > > ```
> > > we do another definition import and IDK_Everything is not set. So we may 
> > > have a wrong kind of import since the "minimal" flag is set.
> > > 
> > > The thing is, it is really confusing and error-prone to have both the 
> > > `ASTImporter::Minimal` flag and the `ImportDefinitionKind`. They seem to 
> > > be in contradiction to each other some times.
> > > I think we should get rid of the Minimal flag. And Kind should be either 
> > > a full completion (IDK_Everythink) or just a minimal (IDK_Basic). So, I'd 
> > > scrap the IDK_Default too. Alternatively we could have a Kind member in 
> > > AST**//Node//**Importer.
> > > I think we should go into this direction to avoid similar problems during 
> > > CodeGen in the future. WDYT?
> > No offense, you definitely understand the Importer better than I, so I 
> > value your input here. I don't believe that should have other fields where 
> > we could have a record that effects the layout but the concern is still 
> > valid and yes I did miss multi-dimensional arrays.
> > 
> > Your theory on `IDK_Everything` not be pushed through everywhere, I did a 
> > quick look and it seems pretty reasonable. 
> > 
> > I agree that the `ASTImporter::Minimal` flag and the `ImportDefinitionKind` 
> > seem to inconsistent or perhaps a work-around. That seems like a bigger 
> > change with a lot more impact beyond this fix but worth looking into longer 
> > term. 
> > 
> > If pushing through `IDK_Everything` everywhere fixes the underlying issue I 
> > am very happy to take that approach. If not we can discuss alternatives. 
> I've been looking at this code, but I'm still not very familiar with it, so 
> what I am asking may be obvious, but... What is the expected behavior for 
> non-minimal imports for code like this?
> ```
> struct A { ...};
> struct B { A* a; }; // Note the pointer
> ```
> Should importing B also import the definition of the A struct ? As I think 
> that should not happen in the minimal import... If we get rid of the minimal 
> flag, and rely solely on argument passing, we will need to be careful, as it 
> shouldn't be passed _everywhere_ (it should stop at pointers for instance). 
> But then it may not be possible to reproduce the current non-minimal import, 
> as it (I think) expects that A will be fully imported too...
> 
> > I don't believe that should have other fields where we could have a record 
> > that effects the layout
> This isn't exactly layout related, but there is the question of covariant 
> methods. If a method is covariant, then its return type must be complete. 
> Currently we handle the completion of these in LLDB, but that solution is a 
> bit hacky. It might be interesting if that could be handled by the ast 
> importer as well.
> What is the expected behavior for non-minimal imports for code like this?
In this case we import the definition of

[Lldb-commits] [PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

The key question is this: Why exactly does `1735: ExpectedDecl ImportedOrErr = 
import(From);` fails to do the import properly when called from 
ASTImporter::ImportDefinition? Would it be possible to debug that from your 
LLDB test cases? Maybe starting with the simpler tests in D71378 
?

In D86660#2255360 , @shafik wrote:

> @martong I have been experimenting w/ how we pass around 
> `ImportDefinitionKind` and pushing it through to more places does not help. I 
> also tried defaulting most locations to `IDK_Everything` to experiment to see 
> if maybe I was missing some crucial point and no luck. So while it seemed 
> like a good direction it has not worked out, unless I missed something in 
> your idea.

These are bad news, probably I am missing something. Still, we should not give 
up the investigation, or not at least in a long term. I really think we have to 
fix this sooner or later. What about the other approaches (trying to get rid of 
the minimal flag, trying to get rid of the kind, moving the minimal flag to the 
nodeimporter, and could be other ideas as well)?

> So my current approach may be the best we have until we can do some larger 
> refactoring.

Okay, I understand this fixes the immediate issue and we don't have any other 
solution yet without significant more efforts.

> I had explored trying to fix this from the codegen and sema side of things 
> but after discussing this w/ @rsmith there is no simple fix there that would 
> not require some large refactoring on the LLDB side.

There must be a way to fix this in ASTImporter and/or in LLDB. We should be 
able to provide an AST that is meaningful for the codegen. But yeah, some 
refactoring might be needed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86660/new/

https://reviews.llvm.org/D86660

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

In D86660#2278025 , @shafik wrote:

> We are going to move forward with this approach (after dealing with the 
> multi-dimensional array case) temporarily. We are seeing crash bugs from this 
> from users and we want to fix it while we explore the solution space more.

Okay, I understand the business reasons and I am fine with this patch if this 
is really a temporary solution. Just please add a FIXME that indicates this and 
D71378  are temporary, plus that this should 
be really handled by the preceding `import` call.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86660/new/

https://reviews.llvm.org/D86660

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D68140: [lldb][clang][modern-type-lookup] Use ASTImporterSharedState in ExternalASTMerger

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

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




Comment at: cfe/trunk/lib/AST/ExternalASTMerger.cpp:320
+  SharedState = std::make_shared(
+  *Target.AST.getTranslationUnitDecl());
   AddSources(Sources);

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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68140/new/

https://reviews.llvm.org/D68140



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D68326: [lldb][modern-type-lookup] No longer import temporary declarations into the persistent AST

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

With [modern-type-lookup], we completely evade the use of 
`ASTImporterDelegate`? That would be a wonderful thing to use only the the 
ExternalASTMerger on a long term...

> ...  a bunch of duplicated declarations

This popped a few thoughts into my head:
One way to discover duplicated declarations is to set the ODRViolation handling 
strategy to "conservative" in clang::ASTImporter. However, minimal imported 
decls will be structurally non-equivalent with those which are imported in 
normal mode. Thus, it may make sense only if the normal import mode is used 
exclusively.

> The next step is to run the ASTImporter for these temporary expressions with 
> the MinimalImport mode disabled, but that's a follow up patch.

👍 I think this is a very good direction.




Comment at: clang/include/clang/AST/ExternalASTMerger.h:120
+  /// declaration. If any ASTImporter did import the given declaration,
+  /// then this functions returns the declaration that FromD was imported from.
+  /// Returns nullptr if no ASTImporter did import import FromD.

typo: `functions` -> `function`
`FromD` -> `D` ?



Comment at: clang/lib/AST/DeclBase.cpp:95
  DeclContext *Parent, std::size_t Extra) {
+  if (!(!Parent || &Parent->getParentASTContext() == &Ctx)) {
+llvm::errs() << Parent << " | " << &Parent->getParentASTContext()

Left over debug printout?



Comment at: clang/lib/AST/ExternalASTMerger.cpp:108
+  /// from.
+  llvm::DenseMap ToOrigin;
+  /// @see ExternalASTMerger::ImporterSource::Merger

I was thinking about that `ToOrigin` could be in the SharedState, but then I 
realized it is better to have it here, because the merger is the one which 
encapsulates the handling of several importers.



Comment at: clang/lib/AST/ExternalASTMerger.cpp:141
+// that doesn't cause having minimally imported declarations in the target
+// ASTContext that no connected ASTImporter has imported (and can 
complete).
+//

This line/sentence is hard to parse for me.
I get this part: 

```
This way the ExternalASTMerger can safely do a minimal import that doesn't 
cause having minimally imported declarations in the target ASTContext.
```

But this I don't: 
```
that no connected ASTImporter has imported.
```



Comment at: clang/lib/AST/ExternalASTMerger.cpp:149
+// the other (persistent) ASTImporter to this (temporary) ASTImporter.
+// The steps can be visualized like this:
+//

:+1:
I like this approach.



Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68326/new/

https://reviews.llvm.org/D68326



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D68326: [lldb][modern-type-lookup] No longer import temporary declarations into the persistent AST

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

LGTM!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68326/new/

https://reviews.llvm.org/D68326



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 217135.
martong added a comment.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

- Use resulting Name from HandleNameConflict if set
- Add ODRHandling strategies
- Refactor tests, to avoid some code repetition


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.c
  clang/unittests/AST/ASTImporterFixtures.cpp
  clang/unittests/AST/ASTImporterFixtures.h
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/include/lldb/Symbol/ClangASTImporter.h

Index: lldb/include/lldb/Symbol/ClangASTImporter.h
===
--- lldb/include/lldb/Symbol/ClangASTImporter.h
+++ lldb/include/lldb/Symbol/ClangASTImporter.h
@@ -252,7 +252,9 @@
 : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx,
  master.m_file_manager, true /*minimal*/),
   m_decls_to_deport(nullptr), m_decls_already_deported(nullptr),
-  m_master(master), m_source_ctx(source_ctx) {}
+  m_master(master), m_source_ctx(source_ctx) {
+setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+  }
 
 /// Scope guard that attaches a CxxModuleHandler to an ASTImporterDelegate
 /// and deattaches it at the end of the scope. Supports being used multiple
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1879,7 +1879,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2432,6 +2432,62 @@
   EXPECT_EQ(ToD1, ToD2);
 }
 
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5127,15 +5183,6 @@
   }
 }
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
-DefaultTestValuesForRunOptions, );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
-::testing::Values(ArgVector()), );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
-::testing::Values(ArgVector()), );
-
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
   R"(
@@ -5233,8 +5280,8 @@
   // prototype (inside 'friend') for it comes first in the AST and is not
   // linked to the definition.
   EXPECT_EQ(ImportedDef, ToClassDef);
-}  
-  
+}
+
 struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
   LLDBLookupTest() {
 Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
@@ -5362,10 +5409,388 @@
   EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate());
 }
 
+// FIXME Put ODR handling strategy related tests into their own test file. And
+// create type parameterized tests for them like we do in
+// ASTImporterGenericRedeclTest.cpp
+struct ConflictingDeclsWithConservativeStrategy
+: ASTImporterOptionSpecificTestBase {};
+
+struct ConflictingDeclsWithLiberalStrategy : ASTImporterOptionSpecificTestBase {
+  ConflictingDeclsWithLiberalStrateg

[Lldb-commits] [PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

@shafik I have updated the patch with ODR handling strategies as per our 
discusson.

For the record I copy our discussion here:

On Sat, Aug 24, 2019 at 12:50 AM Shafik Yaghmour  
wrote:

> Apologies, I missed this earlier!
> 
> On Wed, Aug 21, 2019 at 2:44 AM Gábor Márton  wrote:
>  >
>  > Hi Shafik,
>  >
>  > > Right now you will end up w/ an arbitrary one of them but we do want to 
> support
>  > a way to choose between them eventually.
>  >
>  > Actually, right now (if the patch is not merged) we end up having both of 
> them in the AST. Some parts of the AST reference the existing definition, 
> while some other parts reference the new definition. Also, the regular lookup 
> will find both definitions.
>  >
>  > If the patch is merged then only the first (the existing) definition is 
> kept and an error is reported.
>  >
>  > > AFAICT this would prevent such a solution. At least that is how the
>  > new test case for RecordDecl make it appear
>  >
>  > Yes, you will never be able to remove an existing node from the AST, so I 
> don't think an either-or choosing mechanism is feasible. But you may be able 
> to decide whether you want to add the new and conflicting node. And you may 
> be able to use a different name for the new conflicting node. This may help 
> clients to see which node they are using.
>  > I want to create an additional patch which builds on this patch. Here is 
> the essence of what I'd like to have:
>  >   if (!ConflictingDecls.empty()) {
>  > Expected Resolution = Importer.HandleNameConflict(
>  > Name, DC, Decl::IDNS_Member, ConflictingDecls.data(),
>  > ConflictingDecls.size());
>  > if (Resolution)
>  >   Name = Resolution.get();
>  > else
>  >   return Resolution.takeError();
>  >   }
>  > Consider the "else" branch. I'd like to have such an "else" branch 
> everywhere. The point is to use the result of HandleNameConflict (if it is 
> set to something). This way it is possible to create any kind of ODR handling 
> by overwriting HandleNameConflict in the descendant classes.
>  >
>  > We identified 3 possible strategies so far:
>  >
>  > Conservative. In case of name conflict propagate the error. This should be 
> the default behavior.
>  > Liberal. In case of name conflict create a new node with the same name and 
> do not propagate the error.
>  > Rename. In case of name conflict create a new node with a different name 
> (e.g. with a prefix of the TU's name). Do not propagate the error.
>  >
>  >
>  > If we add a new conflicting node beside the exiting one, then some clients 
> of the AST which rely on lookup will be confused. The CTU client does not 
> rely on lookup so that is not really a problem there, but I don't know what 
> this could cause with LLDB. Perhaps the renaming strategy could work there 
> too.
>  > The Conservative and Liberal strategies are very easy to implement, and I 
> am going to create patches for that if we have consensus.
> 
> We know currently we do have cases where we have ODR violations w/
>  RecordDecl due to use of an opaque struct in the API headers and a
>  concrete instance internally e.g.:
> 
> //opaque
>  struct A {
> 
>   char buf[16];
> 
> };
> 
> //concrete
>  struct A {
> 
>   double d;
>   int64_t x;
> 
> };
> 
> and we don't want this to become an error.
> 
> I think we would at least one the choice of Conservative or Liberal to
>  be configurable and maybe LLDB would default to Liberal. This would
>  enable us to keep the status quo and not break existing cases any
>  worse than they already are.
> 
> I would prefer that would be done in this PR since I don't want to be
>  in a situation where we branch internally and we have this change but
>  not the follow-up change.
> 
>> >  I don't see how like you comment says this does not effect CXXRecordDecl
>  >
>  > In my comment I do not say that CXXRecordDecls are exceptions from the 
> general way of handling ODR violations.
>  > The comment is about that we shall not report ODR errors if we are not 
> 100% sure that we face one.
>  > So we should report an ODR error only if the found Decl and the newly 
> imported Decl have the same kind.
>  > I.e. both are CXXRecordDecls.
>  > For example, let's assume we import a CXXRecordDecl and we find an 
> existing ClassTemplateDecl with the very same Name.
>  > Then we should not report an ODR violation.
> 
> Thank you for the clarification, I misunderstood the comment, now it
>  makes more sense.
> 
>> Thanks,
>  > Gabor
>  >
>  >
>  > On Mon, Aug 19, 2019 at 5:46 PM Shafik Yaghmour 
>  wrote:
>  >>
>  >> Have a nice vacation :-)
>  >>
>  >> On Mon, Aug 19, 2019 at 8:40 AM Gábor Márton  
> wrote:
>  >> >
>  >> > Hi Shafik,
>  >> >
>  >> > I'll have an answer for you on Wednesday, I'm on vacation until then.
>  >> >
>  >> > Thanks,
>  >> > Gábor
>  >> >
>  >> > On Sat, 17 Aug 2019, 04:30 Shafik Yaghmour,  
> wrote:
>  >> >>
>  >> >> Hello Gábor,
>  >> >>
>  >> >> I was looking at

[Lldb-commits] [PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 217330.
martong marked 2 inline comments as done.
martong added a comment.

- Revert changes in VisitFieldDecl and in VisitIndirectFieldDecl
- Remove test suite ConflictingDeclsWithConservativeStrategy


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.c
  clang/unittests/AST/ASTImporterFixtures.cpp
  clang/unittests/AST/ASTImporterFixtures.h
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/include/lldb/Symbol/ClangASTImporter.h

Index: lldb/include/lldb/Symbol/ClangASTImporter.h
===
--- lldb/include/lldb/Symbol/ClangASTImporter.h
+++ lldb/include/lldb/Symbol/ClangASTImporter.h
@@ -252,7 +252,9 @@
 : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx,
  master.m_file_manager, true /*minimal*/),
   m_decls_to_deport(nullptr), m_decls_already_deported(nullptr),
-  m_master(master), m_source_ctx(source_ctx) {}
+  m_master(master), m_source_ctx(source_ctx) {
+setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+  }
 
 /// Scope guard that attaches a CxxModuleHandler to an ASTImporterDelegate
 /// and deattaches it at the end of the scope. Supports being used multiple
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1879,7 +1879,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2432,6 +2432,62 @@
   EXPECT_EQ(ToD1, ToD2);
 }
 
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5127,15 +5183,6 @@
   }
 }
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
-DefaultTestValuesForRunOptions, );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
-::testing::Values(ArgVector()), );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
-::testing::Values(ArgVector()), );
-
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
   R"(
@@ -5233,8 +5280,8 @@
   // prototype (inside 'friend') for it comes first in the AST and is not
   // linked to the definition.
   EXPECT_EQ(ImportedDef, ToClassDef);
-}  
-  
+}
+
 struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
   LLDBLookupTest() {
 Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
@@ -5362,10 +5409,205 @@
   EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate());
 }
 
+struct ConflictingDeclsWithLiberalStrategy : ASTImporterOptionSpecificTestBase {
+  ConflictingDeclsWithLiberalStrategy() {
+this->ODRHandling = ASTImporter::ODRHandlingType::Liberal;
+  }
+};
+
+// Check that a Decl has been successfully imported into a standalone redecl
+// chain.
+template 
+static void CheckImportedAsNew(llvm::Expected &Result, Decl *ToTU,
+   PatternTy Pattern) {
+ 

[Lldb-commits] [PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Gabor Marton via Phabricator via lldb-commits]

martong marked 4 inline comments as done.
martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:3452
 << FoundField->getType();
-
-  return make_error(ImportError::NameConflict);
+  ConflictingDecls.push_back(FoundField);
 }

shafik wrote:
> I am a little concerned about this change. I am wondering if this was 
> previously handled differently as a band-aid for some other issues that only 
> showed up in rare cases when they iterated over all the results but not when 
> they errored out on the first. 
> 
> Could we make the changes to `VisitFieldDecl` a separate PR so it is easier 
> to revert later on if we do find this causes a regression we are not 
> currently covering?
Ok, I reverted these hunks. I am going to create a follow-up patch which will 
include these changes.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5615
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, Typedef) {
+  Decl *ToTU = getToTuDecl(

shafik wrote:
> Since the "Liberal" strategy should be the current status quo, I think it 
> might make sense to have a first PR that just has the `*LiberalStrategy` 
> tests to verify that indeed this is the current behavior as we expect.
Ok, I removed the test suite `ConflictingDeclsWithConservativeStrategy`. I am 
going to create a subsequent patch which adds comprehensive testing for both 
strategies.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 217337.
martong added a comment.

- Apply clang-format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.c
  clang/unittests/AST/ASTImporterFixtures.cpp
  clang/unittests/AST/ASTImporterFixtures.h
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/include/lldb/Symbol/ClangASTImporter.h

Index: lldb/include/lldb/Symbol/ClangASTImporter.h
===
--- lldb/include/lldb/Symbol/ClangASTImporter.h
+++ lldb/include/lldb/Symbol/ClangASTImporter.h
@@ -252,7 +252,9 @@
 : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx,
  master.m_file_manager, true /*minimal*/),
   m_decls_to_deport(nullptr), m_decls_already_deported(nullptr),
-  m_master(master), m_source_ctx(source_ctx) {}
+  m_master(master), m_source_ctx(source_ctx) {
+  setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+}
 
 /// Scope guard that attaches a CxxModuleHandler to an ASTImporterDelegate
 /// and deattaches it at the end of the scope. Supports being used multiple
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1879,7 +1879,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2432,6 +2432,62 @@
   EXPECT_EQ(ToD1, ToD2);
 }
 
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5127,15 +5183,6 @@
   }
 }
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
-DefaultTestValuesForRunOptions, );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
-::testing::Values(ArgVector()), );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
-::testing::Values(ArgVector()), );
-
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
   R"(
@@ -5233,8 +5280,8 @@
   // prototype (inside 'friend') for it comes first in the AST and is not
   // linked to the definition.
   EXPECT_EQ(ImportedDef, ToClassDef);
-}  
-  
+}
+
 struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
   LLDBLookupTest() {
 Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
@@ -5362,10 +5409,197 @@
   EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate());
 }
 
+struct ConflictingDeclsWithLiberalStrategy : ASTImporterOptionSpecificTestBase {
+  ConflictingDeclsWithLiberalStrategy() {
+this->ODRHandling = ASTImporter::ODRHandlingType::Liberal;
+  }
+};
+
+// Check that a Decl has been successfully imported into a standalone redecl
+// chain.
+template 
+static void CheckImportedAsNew(llvm::Expected &Result, Decl *ToTU,
+   PatternTy Pattern) {
+  ASSERT_TRUE(isSuccess(Result));
+  Decl *ImportedD = *Result;
+  ASSERT_TRUE(ImportedD);
+  auto *ToD = FirstDeclMatcher().match(ToTU, Pattern);
+  EXPECT_NE(

[Lldb-commits] [PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Gabor Marton via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf035b75d8f07: [ASTImporter] Fix name conflict handling with 
different strategies (authored by martong).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.c
  clang/unittests/AST/ASTImporterFixtures.cpp
  clang/unittests/AST/ASTImporterFixtures.h
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/include/lldb/Symbol/ClangASTImporter.h

Index: lldb/include/lldb/Symbol/ClangASTImporter.h
===
--- lldb/include/lldb/Symbol/ClangASTImporter.h
+++ lldb/include/lldb/Symbol/ClangASTImporter.h
@@ -252,7 +252,9 @@
 : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx,
  master.m_file_manager, true /*minimal*/),
   m_decls_to_deport(nullptr), m_decls_already_deported(nullptr),
-  m_master(master), m_source_ctx(source_ctx) {}
+  m_master(master), m_source_ctx(source_ctx) {
+  setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+}
 
 /// Scope guard that attaches a CxxModuleHandler to an ASTImporterDelegate
 /// and deattaches it at the end of the scope. Supports being used multiple
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1879,7 +1879,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2432,6 +2432,62 @@
   EXPECT_EQ(ToD1, ToD2);
 }
 
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5127,15 +5183,6 @@
   }
 }
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
-DefaultTestValuesForRunOptions, );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
-::testing::Values(ArgVector()), );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
-::testing::Values(ArgVector()), );
-
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
   R"(
@@ -5233,8 +5280,8 @@
   // prototype (inside 'friend') for it comes first in the AST and is not
   // linked to the definition.
   EXPECT_EQ(ImportedDef, ToClassDef);
-}  
-  
+}
+
 struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
   LLDBLookupTest() {
 Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
@@ -5362,10 +5409,197 @@
   EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate());
 }
 
+struct ConflictingDeclsWithLiberalStrategy : ASTImporterOptionSpecificTestBase {
+  ConflictingDeclsWithLiberalStrategy() {
+this->ODRHandling = ASTImporter::ODRHandlingType::Liberal;
+  }
+};
+
+// Check that a Decl has been successfully imported into a standalone redecl
+// chain.
+template 
+static void CheckImportedAsNew(llvm::Expected &Result, Decl *ToTU,
+   PatternTy Pattern) {
+  ASSERT_TRUE(isSuccess(Result));
+  Decl *ImportedD = *

[Lldb-commits] [PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

Jenkins is not green 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/
However, the newly failing test is `TestTargetCommand.py`, which seems to be 
unrelated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61478: Move decl completion out of the ASTImporterDelegate and document it [NFC]

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

Looks good to me, thanks!




Comment at: lldb/source/Symbol/ClangASTImporter.cpp:324
+
+NamedDecl *to_named_decl = dyn_cast(to);
+// Check if we already deported this type.

teemperor wrote:
> martong wrote:
> > Would it make sense to filter out those TagDecls which are already 
> > completed?
> > E.g.:
> > ```
> > if (TagDecl *to_tag_decl = dyn_cast(to))
> >   if (to_tag_decl->isCompleteDefinition()) // skip tags which are already 
> > completed
> > return;
> > ```
> > Or this would not work because there are cases when the tag is completed, 
> > but the members are still missing? If that is the case could you please 
> > document that here too?
> Maybe, but that could make this patch non-NFC :) I can make this as a 
> follow-up.
Okay.



Comment at: lldb/source/Symbol/ClangASTImporter.cpp:328
+RecordDecl *from_record_decl = dyn_cast(from);
+// We don't need to completed injected class name decls.
+if (from_record_decl && from_record_decl->isInjectedClassName())

typo: "to completed" -> "to complete"


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61478/new/

https://reviews.llvm.org/D61478



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67803: [lldb] Fix that importing decls in a TagDecl end up in wrong declaration context (partly reverts D61333)

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

Thanks for the thorough explanation about the different ASTContexts in LLDB.
The hack is indeed hideous, but seems good to me... for now until the real 
solution is born.

> So we go to D-AST to get the std::pair members. The ASTImporter is asked to 
> copy over std::pair members
>  and first checks if std::pair is already in P-AST. However, it only finds 
> the std::pair we got from E-AST, so it
>  can't use it's map of already imported declarations and does a comparison 
> between the std::pair decls we have
>  Because the ASTImporter thinks they are different declarations, it creates a 
> second std::pair

Note that LLDB uses the lenient ODR violation handling strategy 
(`ASTImporter::ODRHandlingType::Liberal`).
With the other, stricter ODR violation handling strategy, when the to be 
imported std::pair turns out to be nonequivalent with the existing one we would 
get an error instead of a new decl.
Would it make sense to change the odr handling strategy before the formatter 
turns to the ExternalASTSource?
It would not solve this particular issue, but perhaps could make discovering 
bugs like this easier.

> My preferred solution would be to complete all declarations in E-AST before 
> they get moved to P-AST

That sounds reasonable to me.

> When doing expr m we do a minimal import of std::map from D-AST to E-AST just 
> do the type checking/codegen.

There are some cases, when codegen do need the completed tagdecl, not just the 
skeleton we get via minimal import. So, there must be cases when in the E-AST 
we have a full, completed type, right? What would happen if we imported 
everything as a full declaration in the first place, instead of  using the 
minimal import? Could that work?
Perhaps it is even more intrusive then your preferred solution, but could 
greatly reduce the import related code and complexity in LLDB.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67803/new/

https://reviews.llvm.org/D67803



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D67803: [lldb] Fix that importing decls in a TagDecl end up in wrong declaration context (partly reverts D61333)

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

BTW, I tried to access the bug reports rdar://55502701 and rdar://55129537, but 
could not.
I tried at openradar, but there the search for the ID was not successful.
I wonder if there is a publicly accessible (for non Apple employees) URL for 
these bugs?


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67803/new/

https://reviews.llvm.org/D67803



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59537: Instantiate 'std' templates explicitly in the expression evaluator

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: lldb/source/Symbol/StdModuleHandler.cpp:242
+// Instantiate the template.
+found_decl = ClassTemplateSpecializationDecl::Create(
+m_sema->getASTContext(),

teemperor wrote:
> martong wrote:
> > martong wrote:
> > > Is there any guarantee that the before any subsequent 
> > > clang::ASTImporter::Import call this newly created node is registered as 
> > > imported (via ASTImporter::MapImported)?
> > > 
> > > The reason why I am asking this is because we have to enforce in 
> > > clang::ASTImporter that after every Decl is created then immediately it 
> > > is registered as an imported Decl, see 
> > > `ASTImporter::GetImportedOrCreateDecl`. This we we make sure that no 
> > > subsequent import calls will create the same node again.
> > > The cycles in the AST are handled properly only this way.
> > > 
> > > This is one reason which makes me worried about exposing the import 
> > > mechanism via `ImportInternal`.  Would it be working if 
> > > `GetImportedOrCreateDecl` was virtual and overridden here?
> > > Would it be working if GetImportedOrCreateDecl was virtual and overridden 
> > > here?
> > 
> > Sorry, that would certainly not work, but perhaps a virtual hook inside 
> > that function could work...
> Sorry for the late reply.
> 
> So I was looking into changing the code to your suggestion, but I don't think 
> we can get this working reliably. The problem is that if we intercept the 
> importing in GetImportedOrCreateDecl then we need to intercept not just 
> `std::vector` but also all related imports that the ASTImporter may make 
> before importing `std::vector` itself, e.g. DeclContext, parent classes, 
> return type, template args, etc. Also we constantly would need to update this 
> list in case the standard library or the ASTImporter change internally to 
> make sure we intercept everything.
> 
> Maybe we could instead implement an mechanism in the LLDB code that ensures 
> we do call MapImported and add the decl to the lookup directly after creating 
> a decl? E.g. by implementing something similar to ASTImporter's 
> `GetImportedOrCreateDecl`?
Yes, I think that could work.
So, we could have a new function:
```
void CreateDeclPostamble(Decl *FromD, Decl* ToD) {  // better naming?
  // Keep track of imported Decls.
  Importer.MapImported(FromD, ToD);
  Importer.AddToLookupTable(ToD);
  InitializeImportedDecl(FromD, ToD);
}
```
And we'd call this from `GetImportedOrCreateDecl` and here after `::Create`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59537/new/

https://reviews.llvm.org/D59537



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59537: Instantiate 'std' templates explicitly in the expression evaluator

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: lldb/source/Symbol/StdModuleHandler.cpp:242
+// Instantiate the template.
+found_decl = ClassTemplateSpecializationDecl::Create(
+m_sema->getASTContext(),

martong wrote:
> teemperor wrote:
> > martong wrote:
> > > martong wrote:
> > > > Is there any guarantee that the before any subsequent 
> > > > clang::ASTImporter::Import call this newly created node is registered 
> > > > as imported (via ASTImporter::MapImported)?
> > > > 
> > > > The reason why I am asking this is because we have to enforce in 
> > > > clang::ASTImporter that after every Decl is created then immediately it 
> > > > is registered as an imported Decl, see 
> > > > `ASTImporter::GetImportedOrCreateDecl`. This we we make sure that no 
> > > > subsequent import calls will create the same node again.
> > > > The cycles in the AST are handled properly only this way.
> > > > 
> > > > This is one reason which makes me worried about exposing the import 
> > > > mechanism via `ImportInternal`.  Would it be working if 
> > > > `GetImportedOrCreateDecl` was virtual and overridden here?
> > > > Would it be working if GetImportedOrCreateDecl was virtual and 
> > > > overridden here?
> > > 
> > > Sorry, that would certainly not work, but perhaps a virtual hook inside 
> > > that function could work...
> > Sorry for the late reply.
> > 
> > So I was looking into changing the code to your suggestion, but I don't 
> > think we can get this working reliably. The problem is that if we intercept 
> > the importing in GetImportedOrCreateDecl then we need to intercept not just 
> > `std::vector` but also all related imports that the ASTImporter may make 
> > before importing `std::vector` itself, e.g. DeclContext, parent classes, 
> > return type, template args, etc. Also we constantly would need to update 
> > this list in case the standard library or the ASTImporter change internally 
> > to make sure we intercept everything.
> > 
> > Maybe we could instead implement an mechanism in the LLDB code that ensures 
> > we do call MapImported and add the decl to the lookup directly after 
> > creating a decl? E.g. by implementing something similar to ASTImporter's 
> > `GetImportedOrCreateDecl`?
> Yes, I think that could work.
> So, we could have a new function:
> ```
> void CreateDeclPostamble(Decl *FromD, Decl* ToD) {  // better naming?
>   // Keep track of imported Decls.
>   Importer.MapImported(FromD, ToD);
>   Importer.AddToLookupTable(ToD);
>   InitializeImportedDecl(FromD, ToD);
> }
> ```
> And we'd call this from `GetImportedOrCreateDecl` and here after `::Create`.
Well, the thing is a bit more complex because this is in `ASTNodeImporter`, so 
we need to wire things up to `ASTImporter`. But that seems doable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59537/new/

https://reviews.llvm.org/D59537



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59537: Instantiate 'std' templates explicitly in the expression evaluator

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

In D59537#1481333 , @teemperor wrote:

> @martong See D59485  for the new ASTImporter 
> method for registering imported decls. I created the method in the 
> ASTImporter as the only dependency on the ASTNodeImporter is 
> `InitializeImportedDecl` which only imports more decl attributes/flags and 
> doesn't update any internal ASTImporter state from what I can tell.


Raphael, thank you for working on this. Looks good to me now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59537/new/

https://reviews.llvm.org/D59537



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportImpl method to allow customizing Import behavior.

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.

Looks good, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59485/new/

https://reviews.llvm.org/D59485



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportImpl method to allow customizing Import behavior.

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:625
+  "class shouldNotBeImported {};", Lang_CXX, "class realDecl {};", 
Lang_CXX,
+  "shouldNotBeImported", RedirectingImporter::Constructor);
+  auto *Imported = cast(To);

I just realized that maybe we could simplify and make this part more elegant:
If `ASTImporterTestBase` had a data member with the type `ImporterConstructor` 
then we could overwrite that data member in the constructor of `CustomImporter`.
(The `ASTImporter` is created lazily when the first `getImportedDecl` is 
called, but by that time the creator is already set, so this will work.)
This way we could elide the last argument here 
(`RedirectingImporter::Constructor`) and in all subsequent `TEST_P` tests, it 
would be enough to set up the creator per test fixture.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59485/new/

https://reviews.llvm.org/D59485



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61299: Rename Minion to ASTImporterDelegate

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: lldb/include/lldb/Symbol/ClangASTImporter.h:328
 clang::ASTContext *m_dst_ctx;
-MinionMap m_minions;
+DelegateMap m_minions;
 OriginMap m_origins;

Would it make sense to rename the variables too? E.g. `m_delegates`?


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61299/new/

https://reviews.llvm.org/D61299



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61299: Rename Minion to ASTImporterDelegate

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: lldb/include/lldb/Symbol/ClangASTImporter.h:259
 public:
-  CxxModuleScope(Minion &minion, clang::ASTContext *dst_ctx)
+  CxxModuleScope(ASTImporterDelegate &minion, clang::ASTContext *dst_ctx)
   : m_minion(minion) {

The name of the parameter is still `minion` ... if the goal is to completely 
wipe out the old name then perhaps these should be renamed. Also with the new 
type name it just feels odd.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61299/new/

https://reviews.llvm.org/D61299



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong created this revision.
martong added reviewers: shafik, teemperor, jingham, clayborg, a_sidorin.
Herald added subscribers: lldb-commits, cfe-commits, gamesh411, Szelethus, 
dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added projects: clang, LLDB.

With LLDB we use localUncachedLookup(), however, that fails to find
Decls when a transparent context is involved and the given DC has
external lexical storage.  The solution is to use noload_lookup, which
works well with transparent contexts.  But, we cannot use only the
noload_lookup since the slow case of localUncachedLookup is still needed
in some other cases.

These other cases are handled in ASTImporterLookupTable, but we cannot
use that with LLDB since that traverses through the AST which initiates
the load of external decls again via DC::decls().

We must avoid loading external decls during the import becuase
ExternalASTSource is implemented with ASTImporter, so external loads
during import results in uncontrolled and faulty import.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61333

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/include/lldb/Utility/Logging.h
  lldb/packages/Python/lldbsuite/test/lang/c/ast/Makefile
  lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
  lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
  lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/source/Utility/Logging.cpp

Index: lldb/source/Utility/Logging.cpp
===
--- lldb/source/Utility/Logging.cpp
+++ lldb/source/Utility/Logging.cpp
@@ -17,6 +17,7 @@
 
 static constexpr Log::Category g_categories[] = {
   {{"api"}, {"log API calls and return values"}, LIBLLDB_LOG_API},
+  {{"ast"}, {"log AST"}, LIBLLDB_LOG_AST},
   {{"break"}, {"log breakpoints"}, LIBLLDB_LOG_BREAKPOINTS},
   {{"commands"}, {"log command argument parsing"}, LIBLLDB_LOG_COMMANDS},
   {{"comm"}, {"log communication activities"}, LIBLLDB_LOG_COMMUNICATION},
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -918,6 +918,28 @@
   if (clang::TagDecl *to_tag = dyn_cast(to)) {
 if (clang::TagDecl *from_tag = dyn_cast(from)) {
   to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
+
+  Log *log_ast(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST));
+  if (log_ast) {
+std::string name_string;
+if (NamedDecl *from_named_decl = dyn_cast(from)) {
+  llvm::raw_string_ostream name_stream(name_string);
+  from_named_decl->printName(name_stream);
+  name_stream.flush();
+}
+log_ast->Printf(" [ClangASTImporter][TUDecl: %p] Imported "
+"(%sDecl*)%p, named %s (from "
+"(Decl*)%p)",
+static_cast(to->getTranslationUnitDecl()),
+from->getDeclKindName(), static_cast(to),
+name_string.c_str(), static_cast(from));
+
+// Log the AST of the TU.
+std::string ast_string;
+llvm::raw_string_ostream ast_stream(ast_string);
+to->getTranslationUnitDecl()->dump(ast_stream);
+log_ast->Printf("%s", ast_string.c_str());
+  }
 }
   }
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -637,18 +637,6 @@
 
 m_ast_importer_sp->RequireCompleteType(copied_field_type);
   }
-
-  DeclContext *decl_context_non_const =
-  const_cast(decl_context);
-
-  if (copied_decl->getDeclContext() != decl_context) {
-if (copied_decl->getDeclContext()->containsDecl(copied_decl))
-  copied_decl->getDeclContext()->removeDecl(copied_decl);
-copied_decl->setDeclContext(decl_context_non_const);
-  }
-
-  if (!decl_context_non_const->containsDecl(copied_decl))
-decl_context_non_const->addDeclInternal(copied_decl);
 }
   }
 
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
@@ -5,11 +5,11 @@
 typedef struct {
 int a;
 int b;
-} FILE;
+} MYFILE;
 
 int main()
 {
-FILE *myFile = malloc(sizeof(FILE));
+MYFILE *myFile = malloc(sizeof(MYFILE));
 
 myFile->a = 5;
 myFile->b = 9;
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
===

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong marked 2 inline comments as done.
martong added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c:8
 int b;
-} FILE;
+} MYFILE;
 

In TestCmodules.py we have `import Darwin` and then `expr *fopen("/dev/zero", 
"w")`. This imports the name `FILE` from the Darwin module. Then when we want 
`expr *myFile`, the two different definition of the `FILE` structs collides.
This happens because now the lookup finds the existing definition for the 
`FILE` in the TU associated with the expression evaluator, and that comes from 
the Darwin module.



Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:640
   }
-
-  DeclContext *decl_context_non_const =

We must remove the below addDeclInternal call, because that may be called from 
another
addDeclInternal:
```
6  clang::CXXConstructorDecl::isDefaultConstructor() const + 87
7  clang::CXXRecordDecl::addedMember(clang::Decl*) + 803
8  clang::DeclContext::addHiddenDecl(clang::Decl*) + 341
9  clang::DeclContext::addDeclInternal(clang::Decl*) + 29
10 lldb_private::ClangASTSource::FindExternalLexicalDecls(clang::DeclContext 
const*, llvm::function_ref, 
llvm::SmallVectorImpl&) + 3917
11 
lldb_private::ClangASTSource::ClangASTSourceProxy::FindExternalLexicalDecls(clang::DeclContext
 const*, llvm::function_ref, 
llvm::SmallVectorImpl&) + 102
12 clang::ExternalASTSource::FindExternalLexicalDecls(clang::DeclContext 
const*, llvm::SmallVectorImpl&) + 101
13 clang::DeclContext::LoadLexicalDeclsFromExternalStorage() const + 269
14 clang::DeclContext::buildLookup() + 336
15 clang::DeclContext::makeDeclVisibleInContextWithFlags(clang::NamedDecl*, 
bool, bool) + 403
16 clang::DeclContext::addDeclInternal(clang::Decl*) + 92
17 clang::ASTNodeImporter::VisitFieldDecl(clang::FieldDecl*) + 3851
```
... and at the second call of addDeclInternal we may add an incomplete Decl 
(CXXConstructorDecl above).
We must always avoid redundant work in lldb which is already handled in 
Clang::ASTImporter.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59826: [expression] Explicitly build the lookup table of any TagDecl's context

[Gabor Marton via Phabricator via lldb-commits]

martong abandoned this revision.
martong added a comment.

During writing the tests, I realized that this is only a partial solution. So I 
abandon this patch in favor for https://reviews.llvm.org/D61333 , please take a 
look at that.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59826/new/

https://reviews.llvm.org/D59826



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong created this revision.
martong added reviewers: shafik, teemperor, aprantl, a_sidorin, balazske.
Herald added subscribers: lldb-commits, cfe-commits, gamesh411, Szelethus, 
dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added projects: clang, LLDB.

This is the final phase of the refactoring towards using llvm::Expected
and llvm::Error in the ASTImporter API.
This involves the following:

- remove old Import functions which returned with a pointer,
- use the Import_New functions (which return with Err or Expected) everywhere 
and handle their return value
- rename Import_New functions to Import

This affects both Clang and LLDB.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61438

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ExternalASTMerger.cpp
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/Frontend/ASTMerge.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Symbol/ClangASTContext.cpp
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/source/Symbol/CxxModuleHandler.cpp

Index: lldb/source/Symbol/CxxModuleHandler.cpp
===
--- lldb/source/Symbol/CxxModuleHandler.cpp
+++ lldb/source/Symbol/CxxModuleHandler.cpp
@@ -218,8 +218,10 @@
   for (const TemplateArgument &arg : foreign_args.asArray()) {
 switch (arg.getKind()) {
 case TemplateArgument::Type: {
-  llvm::Expected type = m_importer->Import_New(arg.getAsType());
+  llvm::Expected type = m_importer->Import(arg.getAsType());
   if (!type) {
+// FIXME log? Indicate error to user?
+lldbassert(0 && "Couldn't import a type!");
 llvm::consumeError(type.takeError());
 return {};
   }
@@ -229,8 +231,10 @@
 case TemplateArgument::Integral: {
   llvm::APSInt integral = arg.getAsIntegral();
   llvm::Expected type =
-  m_importer->Import_New(arg.getIntegralType());
+  m_importer->Import(arg.getIntegralType());
   if (!type) {
+// FIXME log? Indicate error to user?
+lldbassert(0 && "Couldn't import a type!");
 llvm::consumeError(type.takeError());
 return {};
   }
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -62,8 +62,15 @@
 
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
-  if (delegate_sp)
-return delegate_sp->Import(type);
+  if (delegate_sp) {
+if (llvm::Expected ret_or_error = delegate_sp->Import(type))
+  return *ret_or_error;
+else {
+  // FIXME log? Indicate error to user?
+  lldbassert(0 && "Couldn't import a type!");
+  llvm::consumeError(ret_or_error.takeError());
+}
+  }
 
   return QualType();
 }
@@ -106,7 +113,7 @@
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
   if (delegate_sp) {
-clang::Decl *result = delegate_sp->Import(decl);
+llvm::Expected result = delegate_sp->Import(decl);
 
 if (!result) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
@@ -127,9 +134,14 @@
   "metadata 0x%" PRIx64,
   decl->getDeclKindName(), user_id);
   }
+
+  // FIXME log?
+  llvm::consumeError(result.takeError());
+
+  return nullptr;
 }
 
-return result;
+return *result;
   }
 
   return nullptr;
@@ -641,7 +653,11 @@
 TagDecl *origin_tag_decl = llvm::dyn_cast(decl_origin.decl);
 
 for (Decl *origin_child_decl : origin_tag_decl->decls()) {
-  delegate_sp->Import(origin_child_decl);
+  llvm::Expected imported_or_err =
+  delegate_sp->Import(origin_child_decl);
+  if (!imported_or_err)
+// FIXME return with false?
+consumeError(imported_or_err.takeError());
 }
 
 if (RecordDecl *record_decl = dyn_cast(origin_tag_decl)) {
@@ -666,7 +682,11 @@
   llvm::dyn_cast(decl_origin.decl);
 
   for (Decl *origin_child_decl : origin_interface_decl->decls()) {
-delegate_sp->Import(origin_child_decl);
+llvm::Expected imported_or_err =
+delegate_sp->Import(origin_child_decl);
+if (!imported_or_err)
+  // FIXME return with false?
+  consumeError(imported_or_err.takeError());
   }
 
   return true;
@@ -919,7 +939,16 @@
   to_cxx_record->startDefinition();
   */
 
-  ImportDefinition(from);
+  llvm::Error Err = ImportDefinition(from);
+  if (Err) {
+// FIXME Indicate import error to user?
+Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+if (log)
+  log->Printf("[ClangASTImporter] Error during importing definition!");
+lldbassert(0 && "Coul

[Lldb-commits] [PATCH] D61478: Move decl completion out of the ASTImporterDelegate and document it [NFC]

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

This is a good patch, it is good to separate responsibilities and it makes 
cleaner how the clang::ASTImporter is used.




Comment at: lldb/source/Symbol/ClangASTImporter.cpp:250
+/// imported while completing the original Decls).
+class DeportQueueScope : public ClangASTImporter::NewDeclListener {
+  ClangASTImporter::ImporterDelegateSP m_delegate;

The verb `deport` is pretty vague in this context for me. Actually, what this 
class does is importing missing members and methods of classes. Perhaps a 
better name could be `ImportMembersQueueScope` ?
I still don't understand why is it needed to import the members in two steps in 
LLDB: 1. import the class itself without members 2. import the members. So 
perhaps we could have some documentation about that too here.



Comment at: lldb/source/Symbol/ClangASTImporter.cpp:324
+
+NamedDecl *to_named_decl = dyn_cast(to);
+// Check if we already deported this type.

Would it make sense to filter out those TagDecls which are already completed?
E.g.:
```
if (TagDecl *to_tag_decl = dyn_cast(to))
  if (to_tag_decl->isCompleteDefinition()) // skip tags which are already 
completed
return;
```
Or this would not work because there are cases when the tag is completed, but 
the members are still missing? If that is the case could you please document 
that here too?


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61478/new/

https://reviews.llvm.org/D61478



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 197948.
martong added a comment.

- Log and do not assert anywhere


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ExternalASTMerger.cpp
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/Frontend/ASTMerge.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Symbol/ClangASTContext.cpp
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/source/Symbol/CxxModuleHandler.cpp

Index: lldb/source/Symbol/CxxModuleHandler.cpp
===
--- lldb/source/Symbol/CxxModuleHandler.cpp
+++ lldb/source/Symbol/CxxModuleHandler.cpp
@@ -9,6 +9,7 @@
 #include "lldb/Symbol/CxxModuleHandler.h"
 
 #include "lldb/Symbol/ClangASTContext.h"
+#include "lldb/Utility/Log.h"
 #include "clang/Sema/Lookup.h"
 #include "llvm/Support/Error.h"
 
@@ -214,12 +215,16 @@
   // Import the foreign template arguments.
   llvm::SmallVector imported_args;
 
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+
   // If this logic is changed, also update templateArgsAreSupported.
   for (const TemplateArgument &arg : foreign_args.asArray()) {
 switch (arg.getKind()) {
 case TemplateArgument::Type: {
-  llvm::Expected type = m_importer->Import_New(arg.getAsType());
+  llvm::Expected type = m_importer->Import(arg.getAsType());
   if (!type) {
+if (log)
+  log->Printf("Couldn't import type!");
 llvm::consumeError(type.takeError());
 return {};
   }
@@ -229,8 +234,10 @@
 case TemplateArgument::Integral: {
   llvm::APSInt integral = arg.getAsIntegral();
   llvm::Expected type =
-  m_importer->Import_New(arg.getIntegralType());
+  m_importer->Import(arg.getIntegralType());
   if (!type) {
+if (log)
+  log->Printf("Couldn't import type!");
 llvm::consumeError(type.takeError());
 return {};
   }
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -62,8 +62,16 @@
 
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
-  if (delegate_sp)
-return delegate_sp->Import(type);
+  if (delegate_sp) {
+if (llvm::Expected ret_or_error = delegate_sp->Import(type))
+  return *ret_or_error;
+else {
+  if (Log *log =
+  lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS))
+log->Printf("Couldn't import type!");
+  llvm::consumeError(ret_or_error.takeError());
+}
+  }
 
   return QualType();
 }
@@ -106,7 +114,7 @@
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
   if (delegate_sp) {
-clang::Decl *result = delegate_sp->Import(decl);
+llvm::Expected result = delegate_sp->Import(decl);
 
 if (!result) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
@@ -127,9 +135,13 @@
   "metadata 0x%" PRIx64,
   decl->getDeclKindName(), user_id);
   }
+
+  llvm::consumeError(result.takeError());
+
+  return nullptr;
 }
 
-return result;
+return *result;
   }
 
   return nullptr;
@@ -641,7 +653,11 @@
 TagDecl *origin_tag_decl = llvm::dyn_cast(decl_origin.decl);
 
 for (Decl *origin_child_decl : origin_tag_decl->decls()) {
-  delegate_sp->Import(origin_child_decl);
+  llvm::Expected imported_or_err =
+  delegate_sp->Import(origin_child_decl);
+  if (!imported_or_err)
+// FIXME return with false?
+consumeError(imported_or_err.takeError());
 }
 
 if (RecordDecl *record_decl = dyn_cast(origin_tag_decl)) {
@@ -666,7 +682,11 @@
   llvm::dyn_cast(decl_origin.decl);
 
   for (Decl *origin_child_decl : origin_interface_decl->decls()) {
-delegate_sp->Import(origin_child_decl);
+llvm::Expected imported_or_err =
+delegate_sp->Import(origin_child_decl);
+if (!imported_or_err)
+  // FIXME return with false?
+  consumeError(imported_or_err.takeError());
   }
 
   return true;
@@ -919,7 +939,15 @@
   to_cxx_record->startDefinition();
   */
 
-  ImportDefinition(from);
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+
+  llvm::Error Err = ImportDefinition(from);
+  if (Err) {
+if (log)
+  log->Printf("[ClangASTImporter] Error during importing definition!");
+return;
+  }
+
 
   if (clang::TagDecl *to_tag = dyn_cast(to)) {
 if (clang::TagDecl *from_tag = dyn_cast(from)) {
@@ -949,13 +977,18 @@
   if 

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 198618.
martong added a comment.

- Add braces to 'true' cases when 'false' case has braces
- Simplify logging and error handling with LLDB_LOG_ERROR


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ExternalASTMerger.cpp
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/Frontend/ASTMerge.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Symbol/ClangASTContext.cpp
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/source/Symbol/CxxModuleHandler.cpp

Index: lldb/source/Symbol/CxxModuleHandler.cpp
===
--- lldb/source/Symbol/CxxModuleHandler.cpp
+++ lldb/source/Symbol/CxxModuleHandler.cpp
@@ -9,6 +9,7 @@
 #include "lldb/Symbol/CxxModuleHandler.h"
 
 #include "lldb/Symbol/ClangASTContext.h"
+#include "lldb/Utility/Log.h"
 #include "clang/Sema/Lookup.h"
 #include "llvm/Support/Error.h"
 
@@ -214,13 +215,15 @@
   // Import the foreign template arguments.
   llvm::SmallVector imported_args;
 
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+
   // If this logic is changed, also update templateArgsAreSupported.
   for (const TemplateArgument &arg : foreign_args.asArray()) {
 switch (arg.getKind()) {
 case TemplateArgument::Type: {
-  llvm::Expected type = m_importer->Import_New(arg.getAsType());
+  llvm::Expected type = m_importer->Import(arg.getAsType());
   if (!type) {
-llvm::consumeError(type.takeError());
+LLDB_LOG_ERROR(log, type.takeError(), "Couldn't import type: {0}");
 return {};
   }
   imported_args.push_back(TemplateArgument(*type));
@@ -229,9 +232,9 @@
 case TemplateArgument::Integral: {
   llvm::APSInt integral = arg.getAsIntegral();
   llvm::Expected type =
-  m_importer->Import_New(arg.getIntegralType());
+  m_importer->Import(arg.getIntegralType());
   if (!type) {
-llvm::consumeError(type.takeError());
+LLDB_LOG_ERROR(log, type.takeError(), "Couldn't import type: {0}");
 return {};
   }
   imported_args.push_back(
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -62,8 +62,16 @@
 
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
-  if (delegate_sp)
-return delegate_sp->Import(type);
+  if (delegate_sp) {
+if (llvm::Expected ret_or_error = delegate_sp->Import(type)) {
+  return *ret_or_error;
+} else {
+  Log *log =
+  lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+  LLDB_LOG_ERROR(log, ret_or_error.takeError(),
+ "Couldn't import type: {0}");
+}
+  }
 
   return QualType();
 }
@@ -106,7 +114,7 @@
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
   if (delegate_sp) {
-clang::Decl *result = delegate_sp->Import(decl);
+llvm::Expected result = delegate_sp->Import(decl);
 
 if (!result) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
@@ -127,9 +135,13 @@
   "metadata 0x%" PRIx64,
   decl->getDeclKindName(), user_id);
   }
+
+  llvm::consumeError(result.takeError());
+
+  return nullptr;
 }
 
-return result;
+return *result;
   }
 
   return nullptr;
@@ -624,6 +636,8 @@
   if (!RequireCompleteType(type))
 return false;
 
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+
   if (const TagType *tag_type = type->getAs()) {
 TagDecl *tag_decl = tag_type->getDecl();
 
@@ -641,7 +655,12 @@
 TagDecl *origin_tag_decl = llvm::dyn_cast(decl_origin.decl);
 
 for (Decl *origin_child_decl : origin_tag_decl->decls()) {
-  delegate_sp->Import(origin_child_decl);
+  llvm::Expected imported_or_err =
+  delegate_sp->Import(origin_child_decl);
+  if (!imported_or_err)
+// FIXME return with false?
+LLDB_LOG_ERROR(log, imported_or_err.takeError(),
+   "Couldn't import decl: {0}");
 }
 
 if (RecordDecl *record_decl = dyn_cast(origin_tag_decl)) {
@@ -666,7 +685,12 @@
   llvm::dyn_cast(decl_origin.decl);
 
   for (Decl *origin_child_decl : origin_interface_decl->decls()) {
-delegate_sp->Import(origin_child_decl);
+llvm::Expected imported_or_err =
+delegate_sp->Import(origin_child_decl);
+if (!imported_or_err)
+  // FIXME return with false?
+  LLDB_LOG_ERROR(log, imported_or_err.takeError(),
+ 

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 198623.
martong added a comment.

- Use LLDB_LOG_ERROR in ImportDefinitionTo


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ExternalASTMerger.cpp
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/Frontend/ASTMerge.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Symbol/ClangASTContext.cpp
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/source/Symbol/CxxModuleHandler.cpp

Index: lldb/source/Symbol/CxxModuleHandler.cpp
===
--- lldb/source/Symbol/CxxModuleHandler.cpp
+++ lldb/source/Symbol/CxxModuleHandler.cpp
@@ -9,6 +9,7 @@
 #include "lldb/Symbol/CxxModuleHandler.h"
 
 #include "lldb/Symbol/ClangASTContext.h"
+#include "lldb/Utility/Log.h"
 #include "clang/Sema/Lookup.h"
 #include "llvm/Support/Error.h"
 
@@ -214,13 +215,15 @@
   // Import the foreign template arguments.
   llvm::SmallVector imported_args;
 
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+
   // If this logic is changed, also update templateArgsAreSupported.
   for (const TemplateArgument &arg : foreign_args.asArray()) {
 switch (arg.getKind()) {
 case TemplateArgument::Type: {
-  llvm::Expected type = m_importer->Import_New(arg.getAsType());
+  llvm::Expected type = m_importer->Import(arg.getAsType());
   if (!type) {
-llvm::consumeError(type.takeError());
+LLDB_LOG_ERROR(log, type.takeError(), "Couldn't import type: {0}");
 return {};
   }
   imported_args.push_back(TemplateArgument(*type));
@@ -229,9 +232,9 @@
 case TemplateArgument::Integral: {
   llvm::APSInt integral = arg.getAsIntegral();
   llvm::Expected type =
-  m_importer->Import_New(arg.getIntegralType());
+  m_importer->Import(arg.getIntegralType());
   if (!type) {
-llvm::consumeError(type.takeError());
+LLDB_LOG_ERROR(log, type.takeError(), "Couldn't import type: {0}");
 return {};
   }
   imported_args.push_back(
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -62,8 +62,16 @@
 
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
-  if (delegate_sp)
-return delegate_sp->Import(type);
+  if (delegate_sp) {
+if (llvm::Expected ret_or_error = delegate_sp->Import(type)) {
+  return *ret_or_error;
+} else {
+  Log *log =
+  lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+  LLDB_LOG_ERROR(log, ret_or_error.takeError(),
+ "Couldn't import type: {0}");
+}
+  }
 
   return QualType();
 }
@@ -106,7 +114,7 @@
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
   if (delegate_sp) {
-clang::Decl *result = delegate_sp->Import(decl);
+llvm::Expected result = delegate_sp->Import(decl);
 
 if (!result) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
@@ -127,9 +135,13 @@
   "metadata 0x%" PRIx64,
   decl->getDeclKindName(), user_id);
   }
+
+  llvm::consumeError(result.takeError());
+
+  return nullptr;
 }
 
-return result;
+return *result;
   }
 
   return nullptr;
@@ -624,6 +636,8 @@
   if (!RequireCompleteType(type))
 return false;
 
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+
   if (const TagType *tag_type = type->getAs()) {
 TagDecl *tag_decl = tag_type->getDecl();
 
@@ -641,7 +655,12 @@
 TagDecl *origin_tag_decl = llvm::dyn_cast(decl_origin.decl);
 
 for (Decl *origin_child_decl : origin_tag_decl->decls()) {
-  delegate_sp->Import(origin_child_decl);
+  llvm::Expected imported_or_err =
+  delegate_sp->Import(origin_child_decl);
+  if (!imported_or_err)
+// FIXME return with false?
+LLDB_LOG_ERROR(log, imported_or_err.takeError(),
+   "Couldn't import decl: {0}");
 }
 
 if (RecordDecl *record_decl = dyn_cast(origin_tag_decl)) {
@@ -666,7 +685,12 @@
   llvm::dyn_cast(decl_origin.decl);
 
   for (Decl *origin_child_decl : origin_interface_decl->decls()) {
-delegate_sp->Import(origin_child_decl);
+llvm::Expected imported_or_err =
+delegate_sp->Import(origin_child_decl);
+if (!imported_or_err)
+  // FIXME return with false?
+  LLDB_LOG_ERROR(log, imported_or_err.takeError(),
+ "Couldn't import decl: {0}");
   }
 
   r

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

Ping @shafik @teemperor


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong marked 8 inline comments as done.
martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:5039
+  if (!ToOrErr)
+// FIXME: return the error?
+consumeError(ToOrErr.takeError());

aprantl wrote:
> We don't typically commit FIXME's into LLVM code. Why not just deal with the 
> error properly from the start?
Ok, changed that to return with the error.



Comment at: lldb/source/Symbol/ClangASTImporter.cpp:68
+  return *ret_or_error;
+} else {
+  Log *log =

sgraenitz wrote:
> aprantl wrote:
> > The `else` is redundant.
> Here it's necessary for the scope of `ret_or_error`. That's a bit 
> unfortunate. Maybe invert the condition?
Ok, I have inverted the condition and this way the else  became redundant, so I 
removed it.



Comment at: lldb/source/Symbol/ClangASTImporter.cpp:139
+
+  llvm::consumeError(result.takeError());
+

aprantl wrote:
> Can you convert this to an early return instead?
Okay, I have added
```
  if (!delegate_sp)
return nullptr;
```
But we can't get rid of `consumeError` because otherwise the error object 
remains unchecked and will cause run-time assert.
`LLDB_LOG_ERROR` calls `consumeError` too plus logs the error msg, so I changed 
to use that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 198822.
martong marked 3 inline comments as done.
martong added a comment.

- Remove FIXME and return the error
- Use early return where possible and remove redundant else


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ExternalASTMerger.cpp
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/Frontend/ASTMerge.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Symbol/ClangASTContext.cpp
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/source/Symbol/CxxModuleHandler.cpp

Index: lldb/source/Symbol/CxxModuleHandler.cpp
===
--- lldb/source/Symbol/CxxModuleHandler.cpp
+++ lldb/source/Symbol/CxxModuleHandler.cpp
@@ -9,6 +9,7 @@
 #include "lldb/Symbol/CxxModuleHandler.h"
 
 #include "lldb/Symbol/ClangASTContext.h"
+#include "lldb/Utility/Log.h"
 #include "clang/Sema/Lookup.h"
 #include "llvm/Support/Error.h"
 
@@ -214,13 +215,15 @@
   // Import the foreign template arguments.
   llvm::SmallVector imported_args;
 
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+
   // If this logic is changed, also update templateArgsAreSupported.
   for (const TemplateArgument &arg : foreign_args.asArray()) {
 switch (arg.getKind()) {
 case TemplateArgument::Type: {
-  llvm::Expected type = m_importer->Import_New(arg.getAsType());
+  llvm::Expected type = m_importer->Import(arg.getAsType());
   if (!type) {
-llvm::consumeError(type.takeError());
+LLDB_LOG_ERROR(log, type.takeError(), "Couldn't import type: {0}");
 return {};
   }
   imported_args.push_back(TemplateArgument(*type));
@@ -229,9 +232,9 @@
 case TemplateArgument::Integral: {
   llvm::APSInt integral = arg.getAsIntegral();
   llvm::Expected type =
-  m_importer->Import_New(arg.getIntegralType());
+  m_importer->Import(arg.getIntegralType());
   if (!type) {
-llvm::consumeError(type.takeError());
+LLDB_LOG_ERROR(log, type.takeError(), "Couldn't import type: {0}");
 return {};
   }
   imported_args.push_back(
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -62,10 +62,18 @@
 
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
-  if (delegate_sp)
-return delegate_sp->Import(type);
-
-  return QualType();
+  if (!delegate_sp)
+return QualType();
+
+  llvm::Expected ret_or_error = delegate_sp->Import(type);
+  if (!ret_or_error) {
+Log *log =
+  lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+LLDB_LOG_ERROR(log, ret_or_error.takeError(),
+"Couldn't import type: {0}");
+return QualType();
+  }
+  return *ret_or_error;
 }
 
 lldb::opaque_compiler_type_t
@@ -105,34 +113,33 @@
 
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
-  if (delegate_sp) {
-clang::Decl *result = delegate_sp->Import(decl);
-
-if (!result) {
-  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+  if (!delegate_sp)
+return nullptr;
 
-  if (log) {
-lldb::user_id_t user_id = LLDB_INVALID_UID;
-ClangASTMetadata *metadata = GetDeclMetadata(decl);
-if (metadata)
-  user_id = metadata->GetUserID();
-
-if (NamedDecl *named_decl = dyn_cast(decl))
-  log->Printf("  [ClangASTImporter] WARNING: Failed to import a %s "
-  "'%s', metadata 0x%" PRIx64,
-  decl->getDeclKindName(),
-  named_decl->getNameAsString().c_str(), user_id);
-else
-  log->Printf("  [ClangASTImporter] WARNING: Failed to import a %s, "
-  "metadata 0x%" PRIx64,
-  decl->getDeclKindName(), user_id);
-  }
+  llvm::Expected result = delegate_sp->Import(decl);
+  if (!result) {
+Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+LLDB_LOG_ERROR(log, result.takeError(), "Couldn't import decl: {0}");
+if (log) {
+  lldb::user_id_t user_id = LLDB_INVALID_UID;
+  ClangASTMetadata *metadata = GetDeclMetadata(decl);
+  if (metadata)
+user_id = metadata->GetUserID();
+
+  if (NamedDecl *named_decl = dyn_cast(decl))
+log->Printf("  [ClangASTImporter] WARNING: Failed to import a %s "
+"'%s', metadata 0x%" PRIx64,
+decl->getDeclKindName(),
+named_decl->getNameAsString().c_str(), user_id);
+  els

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 198824.
martong added a comment.

- Remove remaining FIXMEs


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ExternalASTMerger.cpp
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/Frontend/ASTMerge.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Symbol/ClangASTContext.cpp
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/source/Symbol/CxxModuleHandler.cpp

Index: lldb/source/Symbol/CxxModuleHandler.cpp
===
--- lldb/source/Symbol/CxxModuleHandler.cpp
+++ lldb/source/Symbol/CxxModuleHandler.cpp
@@ -9,6 +9,7 @@
 #include "lldb/Symbol/CxxModuleHandler.h"
 
 #include "lldb/Symbol/ClangASTContext.h"
+#include "lldb/Utility/Log.h"
 #include "clang/Sema/Lookup.h"
 #include "llvm/Support/Error.h"
 
@@ -214,13 +215,15 @@
   // Import the foreign template arguments.
   llvm::SmallVector imported_args;
 
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+
   // If this logic is changed, also update templateArgsAreSupported.
   for (const TemplateArgument &arg : foreign_args.asArray()) {
 switch (arg.getKind()) {
 case TemplateArgument::Type: {
-  llvm::Expected type = m_importer->Import_New(arg.getAsType());
+  llvm::Expected type = m_importer->Import(arg.getAsType());
   if (!type) {
-llvm::consumeError(type.takeError());
+LLDB_LOG_ERROR(log, type.takeError(), "Couldn't import type: {0}");
 return {};
   }
   imported_args.push_back(TemplateArgument(*type));
@@ -229,9 +232,9 @@
 case TemplateArgument::Integral: {
   llvm::APSInt integral = arg.getAsIntegral();
   llvm::Expected type =
-  m_importer->Import_New(arg.getIntegralType());
+  m_importer->Import(arg.getIntegralType());
   if (!type) {
-llvm::consumeError(type.takeError());
+LLDB_LOG_ERROR(log, type.takeError(), "Couldn't import type: {0}");
 return {};
   }
   imported_args.push_back(
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -62,10 +62,18 @@
 
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
-  if (delegate_sp)
-return delegate_sp->Import(type);
-
-  return QualType();
+  if (!delegate_sp)
+return QualType();
+
+  llvm::Expected ret_or_error = delegate_sp->Import(type);
+  if (!ret_or_error) {
+Log *log =
+  lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+LLDB_LOG_ERROR(log, ret_or_error.takeError(),
+"Couldn't import type: {0}");
+return QualType();
+  }
+  return *ret_or_error;
 }
 
 lldb::opaque_compiler_type_t
@@ -105,34 +113,33 @@
 
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
-  if (delegate_sp) {
-clang::Decl *result = delegate_sp->Import(decl);
-
-if (!result) {
-  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+  if (!delegate_sp)
+return nullptr;
 
-  if (log) {
-lldb::user_id_t user_id = LLDB_INVALID_UID;
-ClangASTMetadata *metadata = GetDeclMetadata(decl);
-if (metadata)
-  user_id = metadata->GetUserID();
-
-if (NamedDecl *named_decl = dyn_cast(decl))
-  log->Printf("  [ClangASTImporter] WARNING: Failed to import a %s "
-  "'%s', metadata 0x%" PRIx64,
-  decl->getDeclKindName(),
-  named_decl->getNameAsString().c_str(), user_id);
-else
-  log->Printf("  [ClangASTImporter] WARNING: Failed to import a %s, "
-  "metadata 0x%" PRIx64,
-  decl->getDeclKindName(), user_id);
-  }
+  llvm::Expected result = delegate_sp->Import(decl);
+  if (!result) {
+Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+LLDB_LOG_ERROR(log, result.takeError(), "Couldn't import decl: {0}");
+if (log) {
+  lldb::user_id_t user_id = LLDB_INVALID_UID;
+  ClangASTMetadata *metadata = GetDeclMetadata(decl);
+  if (metadata)
+user_id = metadata->GetUserID();
+
+  if (NamedDecl *named_decl = dyn_cast(decl))
+log->Printf("  [ClangASTImporter] WARNING: Failed to import a %s "
+"'%s', metadata 0x%" PRIx64,
+decl->getDeclKindName(),
+named_decl->getNameAsString().c_str(), user_id);
+  else
+log->Printf("  [ClangASTImporter] WARNING: Failed to import a %s, "
+"metadata 0x

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: clang/include/clang/AST/ASTImporter.h:203
 /// context, or the import error.
-llvm::Expected Import_New(TypeSourceInfo *FromTSI);
-// FIXME: Remove this version.
-TypeSourceInfo *Import(TypeSourceInfo *FromTSI);
+llvm::Expected Import(TypeSourceInfo *FromTSI);
 

aprantl wrote:
> Wouldn't it make more sense to return `Expected`?
That would not be consistent with the other changes. In this patch we change 
the signature of each functions similarly:
From `T foo(...)` to `Expected foo(...)`.
Also, `TypeSourceInfo` factory functions in `ASTContext` do return with a 
pointer. For example:
```
  TypeSourceInfo *CreateTypeSourceInfo(QualType T, unsigned Size = 0) const;

  /// Allocate a TypeSourceInfo where all locations have been
  /// initialized to a given location, which defaults to the empty
  /// location.
  TypeSourceInfo *
  getTrivialTypeSourceInfo(QualType T,
   SourceLocation Loc = SourceLocation()) const;

```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

@aprantl Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: clang/include/clang/AST/ASTImporter.h:203
 /// context, or the import error.
-llvm::Expected Import_New(TypeSourceInfo *FromTSI);
-// FIXME: Remove this version.
-TypeSourceInfo *Import(TypeSourceInfo *FromTSI);
+llvm::Expected Import(TypeSourceInfo *FromTSI);
 

aprantl wrote:
> martong wrote:
> > aprantl wrote:
> > > Wouldn't it make more sense to return `Expected`?
> > That would not be consistent with the other changes. In this patch we 
> > change the signature of each functions similarly:
> > From `T foo(...)` to `Expected foo(...)`.
> > Also, `TypeSourceInfo` factory functions in `ASTContext` do return with a 
> > pointer. For example:
> > ```
> >   TypeSourceInfo *CreateTypeSourceInfo(QualType T, unsigned Size = 0) const;
> > 
> >   /// Allocate a TypeSourceInfo where all locations have been
> >   /// initialized to a given location, which defaults to the empty
> >   /// location.
> >   TypeSourceInfo *
> >   getTrivialTypeSourceInfo(QualType T,
> >SourceLocation Loc = SourceLocation()) const;
> > 
> > ```
> I believe that LLVM's practice of passing non-null pointers around is bad API 
> design because it's never quite clear from looking at an API which pointer 
> parameters are nullable, so I was hoping that we could correct some of that 
> at least in the cases where we introduce API that return Expected<> objects 
> to make it obvious that this is either an error or a valid object. If you 
> that the perceived inconsistency weighs stronger than the readability 
> improvements let me know and I can reconsider.
I couldn't agree more that having non-null-able pointers is a bad API, and we'd 
be better to have references in these cases. However, I think the situation is 
more complex than that.

If we changed `TypeSourceInfo *` to `TypeSourceInfo &` in this patch, that 
would involve to make similar changes with other importer functions 
(NestedNameSpecifier *, Decl *, Expr *, etc.). That would result in a **huge** 
patch and I am afraid we could not follow the incremental development 
suggestion in the LLVM dev policy. Thus, even if we decide to change to 
references I propose to do that in a separate patch.

Also, some AST nodes are indeed null-able. E.g. the body of a FunctionDecl 
might be null. Or the described class template of a CXXRecordDecl may be null 
(and we rely on this, because first we create the CXXRecordDecl then we import 
the ClassTemplateDecl and only then we set the relation).
The point is that we should identify those AST nodes which are really 
non-nullable in the AST. This does not seem trivial.
Besides, since the ASTImporter class is part of the AST lib, it would look 
strange if we changed to use references only in the ASTImporter. Perhaps, we 
should change in the whole AST API, but that might broke too many dependencies 
I guess. Still, this may worth an RFC on cfe-dev.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

Thank you guys for the review!

In D61438#1501457 , @aprantl wrote:

> Alright, thanks for taking the time to walk me through the thought process!





Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 199569.
martong added a comment.

- Rebase to master


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ExternalASTMerger.cpp
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/Frontend/ASTMerge.cpp
  clang/unittests/AST/ASTImporterFixtures.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Symbol/ClangASTContext.cpp
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/source/Symbol/CxxModuleHandler.cpp

Index: lldb/source/Symbol/CxxModuleHandler.cpp
===
--- lldb/source/Symbol/CxxModuleHandler.cpp
+++ lldb/source/Symbol/CxxModuleHandler.cpp
@@ -9,6 +9,7 @@
 #include "lldb/Symbol/CxxModuleHandler.h"
 
 #include "lldb/Symbol/ClangASTContext.h"
+#include "lldb/Utility/Log.h"
 #include "clang/Sema/Lookup.h"
 #include "llvm/Support/Error.h"
 
@@ -214,13 +215,15 @@
   // Import the foreign template arguments.
   llvm::SmallVector imported_args;
 
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+
   // If this logic is changed, also update templateArgsAreSupported.
   for (const TemplateArgument &arg : foreign_args.asArray()) {
 switch (arg.getKind()) {
 case TemplateArgument::Type: {
-  llvm::Expected type = m_importer->Import_New(arg.getAsType());
+  llvm::Expected type = m_importer->Import(arg.getAsType());
   if (!type) {
-llvm::consumeError(type.takeError());
+LLDB_LOG_ERROR(log, type.takeError(), "Couldn't import type: {0}");
 return {};
   }
   imported_args.push_back(TemplateArgument(*type));
@@ -229,9 +232,9 @@
 case TemplateArgument::Integral: {
   llvm::APSInt integral = arg.getAsIntegral();
   llvm::Expected type =
-  m_importer->Import_New(arg.getIntegralType());
+  m_importer->Import(arg.getIntegralType());
   if (!type) {
-llvm::consumeError(type.takeError());
+LLDB_LOG_ERROR(log, type.takeError(), "Couldn't import type: {0}");
 return {};
   }
   imported_args.push_back(
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -62,10 +62,18 @@
 
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
-  if (delegate_sp)
-return delegate_sp->Import(type);
-
-  return QualType();
+  if (!delegate_sp)
+return QualType();
+
+  llvm::Expected ret_or_error = delegate_sp->Import(type);
+  if (!ret_or_error) {
+Log *log =
+  lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+LLDB_LOG_ERROR(log, ret_or_error.takeError(),
+"Couldn't import type: {0}");
+return QualType();
+  }
+  return *ret_or_error;
 }
 
 lldb::opaque_compiler_type_t
@@ -105,34 +113,33 @@
 
   ASTImporterDelegate::CxxModuleScope std_scope(*delegate_sp, dst_ast);
 
-  if (delegate_sp) {
-clang::Decl *result = delegate_sp->Import(decl);
-
-if (!result) {
-  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+  if (!delegate_sp)
+return nullptr;
 
-  if (log) {
-lldb::user_id_t user_id = LLDB_INVALID_UID;
-ClangASTMetadata *metadata = GetDeclMetadata(decl);
-if (metadata)
-  user_id = metadata->GetUserID();
-
-if (NamedDecl *named_decl = dyn_cast(decl))
-  log->Printf("  [ClangASTImporter] WARNING: Failed to import a %s "
-  "'%s', metadata 0x%" PRIx64,
-  decl->getDeclKindName(),
-  named_decl->getNameAsString().c_str(), user_id);
-else
-  log->Printf("  [ClangASTImporter] WARNING: Failed to import a %s, "
-  "metadata 0x%" PRIx64,
-  decl->getDeclKindName(), user_id);
-  }
+  llvm::Expected result = delegate_sp->Import(decl);
+  if (!result) {
+Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+LLDB_LOG_ERROR(log, result.takeError(), "Couldn't import decl: {0}");
+if (log) {
+  lldb::user_id_t user_id = LLDB_INVALID_UID;
+  ClangASTMetadata *metadata = GetDeclMetadata(decl);
+  if (metadata)
+user_id = metadata->GetUserID();
+
+  if (NamedDecl *named_decl = dyn_cast(decl))
+log->Printf("  [ClangASTImporter] WARNING: Failed to import a %s "
+"'%s', metadata 0x%" PRIx64,
+decl->getDeclKindName(),
+named_decl->getNameAsString().c_str(), user_id);
+  else
+log->Printf("  [ClangASTImporter] WARNING: Failed to import a 

[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL360760: [ASTImporter] Use llvm::Expected and Error in the 
importer API (authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61438?vs=199569&id=199573#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61438/new/

https://reviews.llvm.org/D61438

Files:
  cfe/trunk/include/clang/AST/ASTImporter.h
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/lib/AST/ExternalASTMerger.cpp
  cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
  cfe/trunk/lib/Frontend/ASTMerge.cpp
  cfe/trunk/unittests/AST/ASTImporterFixtures.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/trunk/source/Symbol/ClangASTContext.cpp
  lldb/trunk/source/Symbol/ClangASTImporter.cpp
  lldb/trunk/source/Symbol/CxxModuleHandler.cpp

Index: cfe/trunk/include/clang/AST/ASTImporter.h
===
--- cfe/trunk/include/clang/AST/ASTImporter.h
+++ cfe/trunk/include/clang/AST/ASTImporter.h
@@ -183,7 +183,7 @@
 /// \return Error information (success or error).
 template 
 LLVM_NODISCARD llvm::Error importInto(ImportT &To, const ImportT &From) {
-  auto ToOrErr = Import_New(From);
+  auto ToOrErr = Import(From);
   if (ToOrErr)
 To = *ToOrErr;
   return ToOrErr.takeError();
@@ -193,40 +193,29 @@
 /// context. A null type is imported as a null type (no error).
 ///
 /// \returns The equivalent type in the "to" context, or the import error.
-llvm::Expected Import_New(QualType FromT);
-// FIXME: Remove this version.
-QualType Import(QualType FromT);
+llvm::Expected Import(QualType FromT);
 
 /// Import the given type source information from the
 /// "from" context into the "to" context.
 ///
 /// \returns The equivalent type source information in the "to"
 /// context, or the import error.
-llvm::Expected Import_New(TypeSourceInfo *FromTSI);
-// FIXME: Remove this version.
-TypeSourceInfo *Import(TypeSourceInfo *FromTSI);
+llvm::Expected Import(TypeSourceInfo *FromTSI);
 
 /// Import the given attribute from the "from" context into the
 /// "to" context.
 ///
 /// \returns The equivalent attribute in the "to" context, or the import
 /// error.
-llvm::Expected Import_New(const Attr *FromAttr);
-// FIXME: Remove this version.
-Attr *Import(const Attr *FromAttr);
+llvm::Expected Import(const Attr *FromAttr);
 
 /// Import the given declaration from the "from" context into the
 /// "to" context.
 ///
 /// \returns The equivalent declaration in the "to" context, or the import
 /// error.
-llvm::Expected Import_New(Decl *FromD);
-llvm::Expected Import_New(const Decl *FromD) {
-  return Import_New(const_cast(FromD));
-}
-// FIXME: Remove this version.
-Decl *Import(Decl *FromD);
-Decl *Import(const Decl *FromD) {
+llvm::Expected Import(Decl *FromD);
+llvm::Expected Import(const Decl *FromD) {
   return Import(const_cast(FromD));
 }
 
@@ -251,28 +240,21 @@
 ///
 /// \returns The equivalent expression in the "to" context, or the import
 /// error.
-llvm::Expected Import_New(Expr *FromE);
-// FIXME: Remove this version.
-Expr *Import(Expr *FromE);
+llvm::Expected Import(Expr *FromE);
 
 /// Import the given statement from the "from" context into the
 /// "to" context.
 ///
 /// \returns The equivalent statement in the "to" context, or the import
 /// error.
-llvm::Expected Import_New(Stmt *FromS);
-// FIXME: Remove this version.
-Stmt *Import(Stmt *FromS);
+llvm::Expected Import(Stmt *FromS);
 
 /// Import the given nested-name-specifier from the "from"
 /// context into the "to" context.
 ///
 /// \returns The equivalent nested-name-specifier in the "to"
 /// context, or the import error.
-llvm::Expected
-Import_New(NestedNameSpecifier *FromNNS);
-// FIXME: Remove this version.
-NestedNameSpecifier *Import(NestedNameSpecifier *FromNNS);
+llvm::Expected Import(NestedNameSpecifier *FromNNS);
 
 /// Import the given nested-name-specifier-loc from the "from"
 /// context into the "to" context.
@@ -280,42 +262,32 @@
 /// \returns The equivalent nested-name-specifier-loc in the "to"
 /// context, or the import error.
 llvm::Expected
-Import_New(NestedNameSpecifierLoc FromNNS);
-// FIXME: Remove this version.
-NestedNameSpecifierLoc Import(NestedNameSpecifierLoc FromNNS);
+Import(NestedNameSpecifierLoc FromNNS);
 
 /// Import the given template name from the "from" context into the
 /// "to" context, o

[Lldb-commits] [PATCH] D62061: Add AST logging

[Gabor Marton via Phabricator via lldb-commits]

martong created this revision.
martong added a reviewer: shafik.
Herald added subscribers: lldb-commits, gamesh411, Szelethus, dkrupp, rnkovacs.
Herald added a project: LLDB.

Log the AST of the TU associated with LLDB's `expr` command, once a declaration
is completed


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D62061

Files:
  lldb/include/lldb/Utility/Logging.h
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/source/Utility/Logging.cpp


Index: lldb/source/Utility/Logging.cpp
===
--- lldb/source/Utility/Logging.cpp
+++ lldb/source/Utility/Logging.cpp
@@ -17,6 +17,7 @@
 
 static constexpr Log::Category g_categories[] = {
   {{"api"}, {"log API calls and return values"}, LIBLLDB_LOG_API},
+  {{"ast"}, {"log AST"}, LIBLLDB_LOG_AST},
   {{"break"}, {"log breakpoints"}, LIBLLDB_LOG_BREAKPOINTS},
   {{"commands"}, {"log command argument parsing"}, LIBLLDB_LOG_COMMANDS},
   {{"comm"}, {"log communication activities"}, LIBLLDB_LOG_COMMUNICATION},
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -951,6 +951,28 @@
   if (clang::TagDecl *to_tag = dyn_cast(to)) {
 if (clang::TagDecl *from_tag = dyn_cast(from)) {
   to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
+
+  Log *log_ast(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST));
+  if (log_ast) {
+std::string name_string;
+if (NamedDecl *from_named_decl = dyn_cast(from)) {
+  llvm::raw_string_ostream name_stream(name_string);
+  from_named_decl->printName(name_stream);
+  name_stream.flush();
+}
+log_ast->Printf(" [ClangASTImporter][TUDecl: %p] Imported "
+"(%sDecl*)%p, named %s (from "
+"(Decl*)%p)",
+static_cast(to->getTranslationUnitDecl()),
+from->getDeclKindName(), static_cast(to),
+name_string.c_str(), static_cast(from));
+
+// Log the AST of the TU.
+std::string ast_string;
+llvm::raw_string_ostream ast_stream(ast_string);
+to->getTranslationUnitDecl()->dump(ast_stream);
+log_ast->Printf("%s", ast_string.c_str());
+  }
 }
   }
 
Index: lldb/include/lldb/Utility/Logging.h
===
--- lldb/include/lldb/Utility/Logging.h
+++ lldb/include/lldb/Utility/Logging.h
@@ -42,6 +42,7 @@
 #define LIBLLDB_LOG_LANGUAGE (1u << 28)
 #define LIBLLDB_LOG_DATAFORMATTERS (1u << 29)
 #define LIBLLDB_LOG_DEMANGLE (1u << 30)
+#define LIBLLDB_LOG_AST (1u << 31)
 #define LIBLLDB_LOG_ALL (UINT32_MAX)
 #define LIBLLDB_LOG_DEFAULT
\
   (LIBLLDB_LOG_PROCESS | LIBLLDB_LOG_THREAD | LIBLLDB_LOG_DYNAMIC_LOADER | 
\


Index: lldb/source/Utility/Logging.cpp
===
--- lldb/source/Utility/Logging.cpp
+++ lldb/source/Utility/Logging.cpp
@@ -17,6 +17,7 @@
 
 static constexpr Log::Category g_categories[] = {
   {{"api"}, {"log API calls and return values"}, LIBLLDB_LOG_API},
+  {{"ast"}, {"log AST"}, LIBLLDB_LOG_AST},
   {{"break"}, {"log breakpoints"}, LIBLLDB_LOG_BREAKPOINTS},
   {{"commands"}, {"log command argument parsing"}, LIBLLDB_LOG_COMMANDS},
   {{"comm"}, {"log communication activities"}, LIBLLDB_LOG_COMMUNICATION},
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -951,6 +951,28 @@
   if (clang::TagDecl *to_tag = dyn_cast(to)) {
 if (clang::TagDecl *from_tag = dyn_cast(from)) {
   to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
+
+  Log *log_ast(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST));
+  if (log_ast) {
+std::string name_string;
+if (NamedDecl *from_named_decl = dyn_cast(from)) {
+  llvm::raw_string_ostream name_stream(name_string);
+  from_named_decl->printName(name_stream);
+  name_stream.flush();
+}
+log_ast->Printf(" [ClangASTImporter][TUDecl: %p] Imported "
+"(%sDecl*)%p, named %s (from "
+"(Decl*)%p)",
+static_cast(to->getTranslationUnitDecl()),
+from->getDeclKindName(), static_cast(to),
+name_string.c_str(), static_cast(from));
+
+// Log the AST of the TU.
+std::string ast_string;
+llvm::raw_string_ostream ast_stream(ast_string);
+to->getTranslationUnitDecl()->dump(ast_stream);
+log_ast->Printf("%s", ast_string.c_str());
+  }
 }
   }
 
Index: lldb/include/lldb/Utility/Logging.h

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 200031.
martong added a comment.

- Rebase to master
- Rebase to D62061 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/packages/Python/lldbsuite/test/lang/c/ast/Makefile
  lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
  lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
  lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -637,18 +637,6 @@
 
 m_ast_importer_sp->RequireCompleteType(copied_field_type);
   }
-
-  DeclContext *decl_context_non_const =
-  const_cast(decl_context);
-
-  if (copied_decl->getDeclContext() != decl_context) {
-if (copied_decl->getDeclContext()->containsDecl(copied_decl))
-  copied_decl->getDeclContext()->removeDecl(copied_decl);
-copied_decl->setDeclContext(decl_context_non_const);
-  }
-
-  if (!decl_context_non_const->containsDecl(copied_decl))
-decl_context_non_const->addDeclInternal(copied_decl);
 }
   }
 
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
@@ -5,11 +5,11 @@
 typedef struct {
 int a;
 int b;
-} FILE;
+} MYFILE;
 
 int main()
 {
-FILE *myFile = malloc(sizeof(FILE));
+MYFILE *myFile = malloc(sizeof(MYFILE));
 
 myFile->a = 5;
 myFile->b = 9;
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
@@ -0,0 +1,5 @@
+int main()
+{
+int a = 0; // Set breakpoint 0 here.
+return 0;
+}
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
@@ -0,0 +1,77 @@
+"""Test that importing modules in C works as expected."""
+
+from __future__ import print_function
+
+
+from distutils.version import StrictVersion
+import os
+import time
+import platform
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class CModulesTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIfFreeBSD
+@skipIfLinux
+@skipIfWindows
+@skipIfNetBSD
+@skipIf(macos_version=["<", "10.12"])
+def test_expr(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(
+self, "main.c", self.line, num_expected_locations=1, loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+# The stop reason of the thread should be breakpoint.
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+# The breakpoint should have a hit count of 1.
+self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,
+substrs=[' resolved, hit count = 1'])
+
+# Enable logging of the imported AST.
+log_file = os.path.join(self.getBuildDir(), "lldb-ast-log.txt")
+self.runCmd("log enable lldb ast -f '%s'" % log_file)
+
+self.expect(
+"expr -l objc++ -- @import Darwin; 3",
+VARIABLES_DISPLAYED_CORRECTLY,
+substrs=[
+"int",
+"3"])
+
+# This expr command imports __sFILE with definition
+# (FILE is a typedef to __sFILE.)
+self.expect(
+"expr *fopen(\"/dev/zero\", \"w\")",
+VARIABLES_DISPLAYED_CORRECTLY,
+substrs=[
+"FILE",
+"_close"]
+)
+
+# Check that the AST log contains exactly one definition of __sFILE.
+f = open(log_file)
+log_lines = f.readlines()
+f.close()
+os.remove(log_file)
+self.assertEqual(" ".join(log_lines).count("struct __sFILE definition"), 1)
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+# Find the line number to break inside main().
+self.line = line_number('main.c', '// Set breakpoint 0 here.')
Index: lldb/pack

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: lldb/source/Symbol/ClangASTImporter.cpp:922
+
+  Log *log_ast(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST));
+  if (log_ast) {

shafik wrote:
> I am going to say that the logging change is an excellent additions and 
> stands alone from this change. Although I realize the test depends on this 
> new feature. It makes sense to add the logging in a separate PR.
> 
> I also say this b/c I found a regression and it would be nice to get the 
> logging in while we resolve the regression.
Ok, I have created a new patch for logging (https://reviews.llvm.org/D62061).
I made this patch to be the child of that and rebased to that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 200033.
martong added a comment.

- se -> so


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/packages/Python/lldbsuite/test/lang/c/ast/Makefile
  lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
  lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
  lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -637,18 +637,6 @@
 
 m_ast_importer_sp->RequireCompleteType(copied_field_type);
   }
-
-  DeclContext *decl_context_non_const =
-  const_cast(decl_context);
-
-  if (copied_decl->getDeclContext() != decl_context) {
-if (copied_decl->getDeclContext()->containsDecl(copied_decl))
-  copied_decl->getDeclContext()->removeDecl(copied_decl);
-copied_decl->setDeclContext(decl_context_non_const);
-  }
-
-  if (!decl_context_non_const->containsDecl(copied_decl))
-decl_context_non_const->addDeclInternal(copied_decl);
 }
   }
 
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
@@ -5,11 +5,11 @@
 typedef struct {
 int a;
 int b;
-} FILE;
+} MYFILE;
 
 int main()
 {
-FILE *myFile = malloc(sizeof(FILE));
+MYFILE *myFile = malloc(sizeof(MYFILE));
 
 myFile->a = 5;
 myFile->b = 9;
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
@@ -0,0 +1,5 @@
+int main()
+{
+int a = 0; // Set breakpoint 0 here.
+return 0;
+}
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
@@ -0,0 +1,77 @@
+"""Test that importing modules in C works as expected."""
+
+from __future__ import print_function
+
+
+from distutils.version import StrictVersion
+import os
+import time
+import platform
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class CModulesTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIfFreeBSD
+@skipIfLinux
+@skipIfWindows
+@skipIfNetBSD
+@skipIf(macos_version=["<", "10.12"])
+def test_expr(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(
+self, "main.c", self.line, num_expected_locations=1, loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+# The stop reason of the thread should be breakpoint.
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+# The breakpoint should have a hit count of 1.
+self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,
+substrs=[' resolved, hit count = 1'])
+
+# Enable logging of the imported AST.
+log_file = os.path.join(self.getBuildDir(), "lldb-ast-log.txt")
+self.runCmd("log enable lldb ast -f '%s'" % log_file)
+
+self.expect(
+"expr -l objc++ -- @import Darwin; 3",
+VARIABLES_DISPLAYED_CORRECTLY,
+substrs=[
+"int",
+"3"])
+
+# This expr command imports __sFILE with definition
+# (FILE is a typedef to __sFILE.)
+self.expect(
+"expr *fopen(\"/dev/zero\", \"w\")",
+VARIABLES_DISPLAYED_CORRECTLY,
+substrs=[
+"FILE",
+"_close"]
+)
+
+# Check that the AST log contains exactly one definition of __sFILE.
+f = open(log_file)
+log_lines = f.readlines()
+f.close()
+os.remove(log_file)
+self.assertEqual(" ".join(log_lines).count("struct __sFILE definition"), 1)
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+# Find the line number to break inside main().
+self.line = line_number('main.c', '// Set breakpoint 0 here.')
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/Makefile
==

[Lldb-commits] [PATCH] D62061: Add AST logging

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: lldb/source/Symbol/ClangASTImporter.cpp:956
+  Log *log_ast(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST));
+  if (log_ast) {
+std::string name_string;

JDevlieghere wrote:
> Why not `if (Log* log_ast = 
> lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST))`
Sure, changed that.



Comment at: lldb/source/Symbol/ClangASTImporter.cpp:963
+}
+log_ast->Printf(" [ClangASTImporter][TUDecl: %p] Imported "
+"(%sDecl*)%p, named %s (from "

JDevlieghere wrote:
> You can use the `LLDB_LOG` macro which provides a much nicer interface.
Ok thanks for pointing it out. Changed to use it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62061/new/

https://reviews.llvm.org/D62061



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D62061: Add AST logging

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 200446.
martong marked 4 inline comments as done.
martong added a comment.

- Change to if(Log *log = ...)
- Use LLDB_LOG


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62061/new/

https://reviews.llvm.org/D62061

Files:
  lldb/include/lldb/Utility/Logging.h
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/source/Utility/Logging.cpp


Index: lldb/source/Utility/Logging.cpp
===
--- lldb/source/Utility/Logging.cpp
+++ lldb/source/Utility/Logging.cpp
@@ -17,6 +17,7 @@
 
 static constexpr Log::Category g_categories[] = {
   {{"api"}, {"log API calls and return values"}, LIBLLDB_LOG_API},
+  {{"ast"}, {"log AST"}, LIBLLDB_LOG_AST},
   {{"break"}, {"log breakpoints"}, LIBLLDB_LOG_BREAKPOINTS},
   {{"commands"}, {"log command argument parsing"}, LIBLLDB_LOG_COMMANDS},
   {{"comm"}, {"log communication activities"}, LIBLLDB_LOG_COMMUNICATION},
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -951,6 +951,28 @@
   if (clang::TagDecl *to_tag = dyn_cast(to)) {
 if (clang::TagDecl *from_tag = dyn_cast(from)) {
   to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
+
+  if (Log *log_ast =
+  lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST)) {
+std::string name_string;
+if (NamedDecl *from_named_decl = dyn_cast(from)) {
+  llvm::raw_string_ostream name_stream(name_string);
+  from_named_decl->printName(name_stream);
+  name_stream.flush();
+}
+LLDB_LOG(log_ast, " [ClangASTImporter][TUDecl: {0}] Imported "
+  "({1}Decl*){2}, named {3} (from "
+  "(Decl*){4})",
+ static_cast(to->getTranslationUnitDecl()),
+ from->getDeclKindName(), static_cast(to),
+ name_string.c_str(), static_cast(from));
+
+// Log the AST of the TU.
+std::string ast_string;
+llvm::raw_string_ostream ast_stream(ast_string);
+to->getTranslationUnitDecl()->dump(ast_stream);
+LLDB_LOG(log_ast, "{0}", ast_string.c_str());
+  }
 }
   }
 
Index: lldb/include/lldb/Utility/Logging.h
===
--- lldb/include/lldb/Utility/Logging.h
+++ lldb/include/lldb/Utility/Logging.h
@@ -42,6 +42,7 @@
 #define LIBLLDB_LOG_LANGUAGE (1u << 28)
 #define LIBLLDB_LOG_DATAFORMATTERS (1u << 29)
 #define LIBLLDB_LOG_DEMANGLE (1u << 30)
+#define LIBLLDB_LOG_AST (1u << 31)
 #define LIBLLDB_LOG_ALL (UINT32_MAX)
 #define LIBLLDB_LOG_DEFAULT
\
   (LIBLLDB_LOG_PROCESS | LIBLLDB_LOG_THREAD | LIBLLDB_LOG_DYNAMIC_LOADER | 
\


Index: lldb/source/Utility/Logging.cpp
===
--- lldb/source/Utility/Logging.cpp
+++ lldb/source/Utility/Logging.cpp
@@ -17,6 +17,7 @@
 
 static constexpr Log::Category g_categories[] = {
   {{"api"}, {"log API calls and return values"}, LIBLLDB_LOG_API},
+  {{"ast"}, {"log AST"}, LIBLLDB_LOG_AST},
   {{"break"}, {"log breakpoints"}, LIBLLDB_LOG_BREAKPOINTS},
   {{"commands"}, {"log command argument parsing"}, LIBLLDB_LOG_COMMANDS},
   {{"comm"}, {"log communication activities"}, LIBLLDB_LOG_COMMUNICATION},
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -951,6 +951,28 @@
   if (clang::TagDecl *to_tag = dyn_cast(to)) {
 if (clang::TagDecl *from_tag = dyn_cast(from)) {
   to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
+
+  if (Log *log_ast =
+  lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST)) {
+std::string name_string;
+if (NamedDecl *from_named_decl = dyn_cast(from)) {
+  llvm::raw_string_ostream name_stream(name_string);
+  from_named_decl->printName(name_stream);
+  name_stream.flush();
+}
+LLDB_LOG(log_ast, " [ClangASTImporter][TUDecl: {0}] Imported "
+  "({1}Decl*){2}, named {3} (from "
+  "(Decl*){4})",
+ static_cast(to->getTranslationUnitDecl()),
+ from->getDeclKindName(), static_cast(to),
+ name_string.c_str(), static_cast(from));
+
+// Log the AST of the TU.
+std::string ast_string;
+llvm::raw_string_ostream ast_stream(ast_string);
+to->getTranslationUnitDecl()->dump(ast_stream);
+LLDB_LOG(log_ast, "{0}", ast_string.c_str());
+  }
 }
   }
 
Index: lldb/include/lldb/Utility/Logging.h
=

[Lldb-commits] [PATCH] D62061: Add AST logging

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 200666.
martong added a comment.

- Remove superflous '.c_str()'


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62061/new/

https://reviews.llvm.org/D62061

Files:
  lldb/include/lldb/Utility/Logging.h
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/source/Utility/Logging.cpp


Index: lldb/source/Utility/Logging.cpp
===
--- lldb/source/Utility/Logging.cpp
+++ lldb/source/Utility/Logging.cpp
@@ -17,6 +17,7 @@
 
 static constexpr Log::Category g_categories[] = {
   {{"api"}, {"log API calls and return values"}, LIBLLDB_LOG_API},
+  {{"ast"}, {"log AST"}, LIBLLDB_LOG_AST},
   {{"break"}, {"log breakpoints"}, LIBLLDB_LOG_BREAKPOINTS},
   {{"commands"}, {"log command argument parsing"}, LIBLLDB_LOG_COMMANDS},
   {{"comm"}, {"log communication activities"}, LIBLLDB_LOG_COMMUNICATION},
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -951,6 +951,28 @@
   if (clang::TagDecl *to_tag = dyn_cast(to)) {
 if (clang::TagDecl *from_tag = dyn_cast(from)) {
   to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
+
+  if (Log *log_ast =
+  lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST)) {
+std::string name_string;
+if (NamedDecl *from_named_decl = dyn_cast(from)) {
+  llvm::raw_string_ostream name_stream(name_string);
+  from_named_decl->printName(name_stream);
+  name_stream.flush();
+}
+LLDB_LOG(log_ast, " [ClangASTImporter][TUDecl: {0}] Imported "
+  "({1}Decl*){2}, named {3} (from "
+  "(Decl*){4})",
+ static_cast(to->getTranslationUnitDecl()),
+ from->getDeclKindName(), static_cast(to), name_string,
+ static_cast(from));
+
+// Log the AST of the TU.
+std::string ast_string;
+llvm::raw_string_ostream ast_stream(ast_string);
+to->getTranslationUnitDecl()->dump(ast_stream);
+LLDB_LOG(log_ast, "{0}", ast_string);
+  }
 }
   }
 
Index: lldb/include/lldb/Utility/Logging.h
===
--- lldb/include/lldb/Utility/Logging.h
+++ lldb/include/lldb/Utility/Logging.h
@@ -42,6 +42,7 @@
 #define LIBLLDB_LOG_LANGUAGE (1u << 28)
 #define LIBLLDB_LOG_DATAFORMATTERS (1u << 29)
 #define LIBLLDB_LOG_DEMANGLE (1u << 30)
+#define LIBLLDB_LOG_AST (1u << 31)
 #define LIBLLDB_LOG_ALL (UINT32_MAX)
 #define LIBLLDB_LOG_DEFAULT
\
   (LIBLLDB_LOG_PROCESS | LIBLLDB_LOG_THREAD | LIBLLDB_LOG_DYNAMIC_LOADER | 
\


Index: lldb/source/Utility/Logging.cpp
===
--- lldb/source/Utility/Logging.cpp
+++ lldb/source/Utility/Logging.cpp
@@ -17,6 +17,7 @@
 
 static constexpr Log::Category g_categories[] = {
   {{"api"}, {"log API calls and return values"}, LIBLLDB_LOG_API},
+  {{"ast"}, {"log AST"}, LIBLLDB_LOG_AST},
   {{"break"}, {"log breakpoints"}, LIBLLDB_LOG_BREAKPOINTS},
   {{"commands"}, {"log command argument parsing"}, LIBLLDB_LOG_COMMANDS},
   {{"comm"}, {"log communication activities"}, LIBLLDB_LOG_COMMUNICATION},
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -951,6 +951,28 @@
   if (clang::TagDecl *to_tag = dyn_cast(to)) {
 if (clang::TagDecl *from_tag = dyn_cast(from)) {
   to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
+
+  if (Log *log_ast =
+  lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST)) {
+std::string name_string;
+if (NamedDecl *from_named_decl = dyn_cast(from)) {
+  llvm::raw_string_ostream name_stream(name_string);
+  from_named_decl->printName(name_stream);
+  name_stream.flush();
+}
+LLDB_LOG(log_ast, " [ClangASTImporter][TUDecl: {0}] Imported "
+  "({1}Decl*){2}, named {3} (from "
+  "(Decl*){4})",
+ static_cast(to->getTranslationUnitDecl()),
+ from->getDeclKindName(), static_cast(to), name_string,
+ static_cast(from));
+
+// Log the AST of the TU.
+std::string ast_string;
+llvm::raw_string_ostream ast_stream(ast_string);
+to->getTranslationUnitDecl()->dump(ast_stream);
+LLDB_LOG(log_ast, "{0}", ast_string);
+  }
 }
   }
 
Index: lldb/include/lldb/Utility/Logging.h
===
--- lldb/include/lldb/Utility/Logging.h
+++ lldb/include/lldb/Utility/Logging.

[Lldb-commits] [PATCH] D62061: Add AST logging

[Gabor Marton via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB361362: Add AST logging (authored by martong, committed 
by ).
Herald added a subscriber: teemperor.

Changed prior to commit:
  https://reviews.llvm.org/D62061?vs=200666&id=200670#toc

Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62061/new/

https://reviews.llvm.org/D62061

Files:
  include/lldb/Utility/Logging.h
  source/Symbol/ClangASTImporter.cpp
  source/Utility/Logging.cpp


Index: source/Symbol/ClangASTImporter.cpp
===
--- source/Symbol/ClangASTImporter.cpp
+++ source/Symbol/ClangASTImporter.cpp
@@ -951,6 +951,28 @@
   if (clang::TagDecl *to_tag = dyn_cast(to)) {
 if (clang::TagDecl *from_tag = dyn_cast(from)) {
   to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
+
+  if (Log *log_ast =
+  lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST)) {
+std::string name_string;
+if (NamedDecl *from_named_decl = dyn_cast(from)) {
+  llvm::raw_string_ostream name_stream(name_string);
+  from_named_decl->printName(name_stream);
+  name_stream.flush();
+}
+LLDB_LOG(log_ast, " [ClangASTImporter][TUDecl: {0}] Imported "
+  "({1}Decl*){2}, named {3} (from "
+  "(Decl*){4})",
+ static_cast(to->getTranslationUnitDecl()),
+ from->getDeclKindName(), static_cast(to), name_string,
+ static_cast(from));
+
+// Log the AST of the TU.
+std::string ast_string;
+llvm::raw_string_ostream ast_stream(ast_string);
+to->getTranslationUnitDecl()->dump(ast_stream);
+LLDB_LOG(log_ast, "{0}", ast_string);
+  }
 }
   }
 
Index: source/Utility/Logging.cpp
===
--- source/Utility/Logging.cpp
+++ source/Utility/Logging.cpp
@@ -17,6 +17,7 @@
 
 static constexpr Log::Category g_categories[] = {
   {{"api"}, {"log API calls and return values"}, LIBLLDB_LOG_API},
+  {{"ast"}, {"log AST"}, LIBLLDB_LOG_AST},
   {{"break"}, {"log breakpoints"}, LIBLLDB_LOG_BREAKPOINTS},
   {{"commands"}, {"log command argument parsing"}, LIBLLDB_LOG_COMMANDS},
   {{"comm"}, {"log communication activities"}, LIBLLDB_LOG_COMMUNICATION},
Index: include/lldb/Utility/Logging.h
===
--- include/lldb/Utility/Logging.h
+++ include/lldb/Utility/Logging.h
@@ -42,6 +42,7 @@
 #define LIBLLDB_LOG_LANGUAGE (1u << 28)
 #define LIBLLDB_LOG_DATAFORMATTERS (1u << 29)
 #define LIBLLDB_LOG_DEMANGLE (1u << 30)
+#define LIBLLDB_LOG_AST (1u << 31)
 #define LIBLLDB_LOG_ALL (UINT32_MAX)
 #define LIBLLDB_LOG_DEFAULT
\
   (LIBLLDB_LOG_PROCESS | LIBLLDB_LOG_THREAD | LIBLLDB_LOG_DYNAMIC_LOADER | 
\


Index: source/Symbol/ClangASTImporter.cpp
===
--- source/Symbol/ClangASTImporter.cpp
+++ source/Symbol/ClangASTImporter.cpp
@@ -951,6 +951,28 @@
   if (clang::TagDecl *to_tag = dyn_cast(to)) {
 if (clang::TagDecl *from_tag = dyn_cast(from)) {
   to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
+
+  if (Log *log_ast =
+  lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST)) {
+std::string name_string;
+if (NamedDecl *from_named_decl = dyn_cast(from)) {
+  llvm::raw_string_ostream name_stream(name_string);
+  from_named_decl->printName(name_stream);
+  name_stream.flush();
+}
+LLDB_LOG(log_ast, " [ClangASTImporter][TUDecl: {0}] Imported "
+  "({1}Decl*){2}, named {3} (from "
+  "(Decl*){4})",
+ static_cast(to->getTranslationUnitDecl()),
+ from->getDeclKindName(), static_cast(to), name_string,
+ static_cast(from));
+
+// Log the AST of the TU.
+std::string ast_string;
+llvm::raw_string_ostream ast_stream(ast_string);
+to->getTranslationUnitDecl()->dump(ast_stream);
+LLDB_LOG(log_ast, "{0}", ast_string);
+  }
 }
   }
 
Index: source/Utility/Logging.cpp
===
--- source/Utility/Logging.cpp
+++ source/Utility/Logging.cpp
@@ -17,6 +17,7 @@
 
 static constexpr Log::Category g_categories[] = {
   {{"api"}, {"log API calls and return values"}, LIBLLDB_LOG_API},
+  {{"ast"}, {"log AST"}, LIBLLDB_LOG_AST},
   {{"break"}, {"log breakpoints"}, LIBLLDB_LOG_BREAKPOINTS},
   {{"commands"}, {"log command argument parsing"}, LIBLLDB_LOG_COMMANDS},
   {{"comm"}, {"log communication activities"}, LIBLLDB_LOG_COMMUNICATION},
Index: include/lldb/Utility/Logging.h
===
--- inclu

[Lldb-commits] [PATCH] D62352: Call to HandleNameConflict in VisitRecordDecl mistakeningly using Name instead of SearchName

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

Looks good, thank you!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62352/new/

https://reviews.llvm.org/D62352



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 205093.
martong added a comment.

- Fix regression of TestFormatters.py


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/packages/Python/lldbsuite/test/lang/c/ast/Makefile
  lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
  lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
  lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -609,6 +609,8 @@
   if (!original_decl_context)
 return;
 
+  // Indicates whether we skipped any Decls of the original DeclContext.
+  bool SkippedDecls = false;
   for (TagDecl::decl_iterator iter = original_decl_context->decls_begin();
iter != original_decl_context->decls_end(); ++iter) {
 Decl *decl = *iter;
@@ -637,21 +639,22 @@
 
 m_ast_importer_sp->RequireCompleteType(copied_field_type);
   }
-
-  DeclContext *decl_context_non_const =
-  const_cast(decl_context);
-
-  if (copied_decl->getDeclContext() != decl_context) {
-if (copied_decl->getDeclContext()->containsDecl(copied_decl))
-  copied_decl->getDeclContext()->removeDecl(copied_decl);
-copied_decl->setDeclContext(decl_context_non_const);
-  }
-
-  if (!decl_context_non_const->containsDecl(copied_decl))
-decl_context_non_const->addDeclInternal(copied_decl);
+} else {
+  SkippedDecls = true;
 }
   }
 
+  // CopyDecl may build a lookup table which may set up ExternalLexicalStorage
+  // to false.  However, since we skipped some of the external Decls we must
+  // set it back!
+  if (SkippedDecls) {
+decl_context->setHasExternalLexicalStorage(true);
+// This sets HasLazyExternalLexicalLookups to true.  By setting this bit we
+// ensure that the lookup table is rebuilt, which means the external source
+// is consulted again when a clang::DeclContext::lookup is called.
+const_cast(decl_context)->setMustBuildLookupTable();
+  }
+
   return;
 }
 
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
@@ -5,11 +5,11 @@
 typedef struct {
 int a;
 int b;
-} FILE;
+} MYFILE;
 
 int main()
 {
-FILE *myFile = malloc(sizeof(FILE));
+MYFILE *myFile = malloc(sizeof(MYFILE));
 
 myFile->a = 5;
 myFile->b = 9;
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
@@ -0,0 +1,5 @@
+int main()
+{
+int a = 0; // Set breakpoint 0 here.
+return 0;
+}
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
@@ -0,0 +1,77 @@
+"""Test that importing modules in C works as expected."""
+
+from __future__ import print_function
+
+
+from distutils.version import StrictVersion
+import os
+import time
+import platform
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class CModulesTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIfFreeBSD
+@skipIfLinux
+@skipIfWindows
+@skipIfNetBSD
+@skipIf(macos_version=["<", "10.12"])
+def test_expr(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(
+self, "main.c", self.line, num_expected_locations=1, loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+# The stop reason of the thread should be breakpoint.
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+# The breakpoint should have a hit count of 1.
+self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,
+substrs=[' resolved, hit count = 1'])
+
+# Enable logging of the imported AST.
+log_file = os.path.join(self.getBuildDir(), "lldb-ast-log.txt")
+self.runCmd("log enable lldb ast -f '%s'" % log_file)
+
+self.expect(
+"expr -l objc++ -- @import Darwin; 3",
+VARIABLE

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

About the regression of TestFormatters.py: I realized that the problem is about 
the wrong implementation of the ExternalASTSource interface.
In the implementation of FindExternalLexicalDecls of this interface, we simply 
ignored those cases when the given predicate (passed as a param) is false. When 
that happens, that means we still have some more external decls which should be 
dug up by the upcoming calls of DeclContext::lookup. The fix is about to 
indicate this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

@shafik @jingham This is a polite Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 209832.
martong added a comment.

- Rebase to master


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/packages/Python/lldbsuite/test/lang/c/ast/Makefile
  lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
  lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
  lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -611,6 +611,8 @@
   if (!original_decl_context)
 return;
 
+  // Indicates whether we skipped any Decls of the original DeclContext.
+  bool SkippedDecls = false;
   for (TagDecl::decl_iterator iter = original_decl_context->decls_begin();
iter != original_decl_context->decls_end(); ++iter) {
 Decl *decl = *iter;
@@ -639,21 +641,22 @@
 
 m_ast_importer_sp->RequireCompleteType(copied_field_type);
   }
-
-  DeclContext *decl_context_non_const =
-  const_cast(decl_context);
-
-  if (copied_decl->getDeclContext() != decl_context) {
-if (copied_decl->getDeclContext()->containsDecl(copied_decl))
-  copied_decl->getDeclContext()->removeDecl(copied_decl);
-copied_decl->setDeclContext(decl_context_non_const);
-  }
-
-  if (!decl_context_non_const->containsDecl(copied_decl))
-decl_context_non_const->addDeclInternal(copied_decl);
+} else {
+  SkippedDecls = true;
 }
   }
 
+  // CopyDecl may build a lookup table which may set up ExternalLexicalStorage
+  // to false.  However, since we skipped some of the external Decls we must
+  // set it back!
+  if (SkippedDecls) {
+decl_context->setHasExternalLexicalStorage(true);
+// This sets HasLazyExternalLexicalLookups to true.  By setting this bit we
+// ensure that the lookup table is rebuilt, which means the external source
+// is consulted again when a clang::DeclContext::lookup is called.
+const_cast(decl_context)->setMustBuildLookupTable();
+  }
+
   return;
 }
 
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
@@ -5,11 +5,11 @@
 typedef struct {
 int a;
 int b;
-} FILE;
+} MYFILE;
 
 int main()
 {
-FILE *myFile = malloc(sizeof(FILE));
+MYFILE *myFile = malloc(sizeof(MYFILE));
 
 myFile->a = 5;
 myFile->b = 9;
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/main.c
@@ -0,0 +1,5 @@
+int main()
+{
+int a = 0; // Set breakpoint 0 here.
+return 0;
+}
Index: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
@@ -0,0 +1,77 @@
+"""Test that importing modules in C works as expected."""
+
+from __future__ import print_function
+
+
+from distutils.version import StrictVersion
+import os
+import time
+import platform
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class CModulesTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIfFreeBSD
+@skipIfLinux
+@skipIfWindows
+@skipIfNetBSD
+@skipIf(macos_version=["<", "10.12"])
+def test_expr(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(
+self, "main.c", self.line, num_expected_locations=1, loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+# The stop reason of the thread should be breakpoint.
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+# The breakpoint should have a hit count of 1.
+self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,
+substrs=[' resolved, hit count = 1'])
+
+# Enable logging of the imported AST.
+log_file = os.path.join(self.getBuildDir(), "lldb-ast-log.txt")
+self.runCmd("log enable lldb ast -f '%s'" % log_file)
+
+self.expect(
+"expr -l objc++ -- @import Darwin; 3",
+VARIABLES_DISPLAYED_CORRECT

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

In D61333#1583173 , @teemperor wrote:

> @martong Sorry for the delay, feel free to ping me in the future about these 
> patches. I'll review them ASAP now that I'm back in office, so these delay's 
> hopefully won't happen again.
>
> I tried applying this patch and it seems it needs to be rebased. I would do 
> it myself, but I'm not entirely sure how to rebase the changes to 
> `ASTNodeImporter::ImportDefinition`. It seems we got rid of 
> `To->completeDefinition();`, so not sure if the code that this patch adds is 
> still in the right place.


Raphael,

Thank you for looking into this. I've rebased to master and updated the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py:57
+# This expr command imports __sFILE with definition
+# (FILE is a typedef to __sFILE.)
+self.expect(

teemperor wrote:
> I think this test has a good chance to fail once someone 
> renamed/removes/changes this internal struct (especially if it's currently  
> abstracted with a typedef). Would it be possible to just make a minimal 
> module with open and FILE/__sFILE instead? If it's too much trouble, then I'm 
> also fine with merging this as-is (as this regression is easy to fix).
I'd rather keep this as-is, because I don't have enough confidence and 
experience with c modules (and with macOS).



Comment at: lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py:67
+# Check that the AST log contains exactly one definition of __sFILE.
+f = open(log_file)
+log_lines = f.readlines()

teemperor wrote:
> It seems that this is the only different in the test compared to 
> TestCModules.py. Would it be possible to just add this logging/checking to 
> TestCModules.py as it's anyway testing something very similar?
Ok, I have removed the TestAST.py and added the extra logging and the check 
into TestCModules.py.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong updated this revision to Diff 210281.
martong marked 5 inline comments as done.
martong added a comment.

- Applied clang-format on lldb parts (this changed two lines)
- Added a comment for predicate
- Merged the test into TestCModules.py


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py
  lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
@@ -611,10 +611,15 @@
   if (!original_decl_context)
 return;
 
+  // Indicates whether we skipped any Decls of the original DeclContext.
+  bool SkippedDecls = false;
   for (TagDecl::decl_iterator iter = original_decl_context->decls_begin();
iter != original_decl_context->decls_end(); ++iter) {
 Decl *decl = *iter;
 
+// The predicate function returns true if the passed declaration kind is
+// the one we are looking for.
+// See clang::ExternalASTSource::FindExternalLexicalDecls()
 if (predicate(decl->getKind())) {
   if (log) {
 ASTDumper ast_dumper(decl);
@@ -639,21 +644,22 @@
 
 m_ast_importer_sp->RequireCompleteType(copied_field_type);
   }
-
-  DeclContext *decl_context_non_const =
-  const_cast(decl_context);
-
-  if (copied_decl->getDeclContext() != decl_context) {
-if (copied_decl->getDeclContext()->containsDecl(copied_decl))
-  copied_decl->getDeclContext()->removeDecl(copied_decl);
-copied_decl->setDeclContext(decl_context_non_const);
-  }
-
-  if (!decl_context_non_const->containsDecl(copied_decl))
-decl_context_non_const->addDeclInternal(copied_decl);
+} else {
+  SkippedDecls = true;
 }
   }
 
+  // CopyDecl may build a lookup table which may set up ExternalLexicalStorage
+  // to false.  However, since we skipped some of the external Decls we must
+  // set it back!
+  if (SkippedDecls) {
+decl_context->setHasExternalLexicalStorage(true);
+// This sets HasLazyExternalLexicalLookups to true.  By setting this bit we
+// ensure that the lookup table is rebuilt, which means the external source
+// is consulted again when a clang::DeclContext::lookup is called.
+const_cast(decl_context)->setMustBuildLookupTable();
+  }
+
   return;
 }
 
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
+++ lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
@@ -5,11 +5,11 @@
 typedef struct {
 int a;
 int b;
-} FILE;
+} MYFILE;
 
 int main()
 {
-FILE *myFile = malloc(sizeof(FILE));
+MYFILE *myFile = malloc(sizeof(MYFILE));
 
 myFile->a = 5;
 myFile->b = 9;
Index: lldb/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py
===
--- lldb/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py
+++ lldb/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py
@@ -47,6 +47,10 @@
 self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,
 substrs=[' resolved, hit count = 1'])
 
+# Enable logging of the imported AST.
+log_file = os.path.join(self.getBuildDir(), "lldb-ast-log.txt")
+self.runCmd("log enable lldb ast -f '%s'" % log_file)
+
 self.expect(
 "expr -l objc++ -- @import Darwin; 3",
 VARIABLES_DISPLAYED_CORRECTLY,
@@ -54,6 +58,8 @@
 "int",
 "3"])
 
+# This expr command imports __sFILE with definition
+# (FILE is a typedef to __sFILE.)
 self.expect(
 "expr *fopen(\"/dev/zero\", \"w\")",
 VARIABLES_DISPLAYED_CORRECTLY,
@@ -61,6 +67,14 @@
 "FILE",
 "_close"])
 
+# Check that the AST log contains exactly one definition of __sFILE.
+f = open(log_file)
+log_lines = f.readlines()
+f.close()
+os.remove(log_file)
+self.assertEqual(" ".join(log_lines).count("struct __sFILE definition"),
+ 1)
+
 self.expect("expr *myFile", VARIABLES_DISPLAYED_CORRECTLY,
 substrs=["a", "5", "b", "9"])
 
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5122,6 +5122,51 @@
   EXPECT_EQ(ToLSize, F

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

Thank you guys for the review!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL366325: [ASTImporter] Fix LLDB lookup in transparent ctx and 
with ext src (authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61333?vs=210281&id=210318#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp
  lldb/trunk/packages/Python/lldbsuite/test/lang/c/modules/TestCModules.py
  lldb/trunk/packages/Python/lldbsuite/test/lang/c/modules/main.c
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1707,6 +1707,17 @@
 
 Error ASTNodeImporter::ImportDefinition(
 RecordDecl *From, RecordDecl *To, ImportDefinitionKind Kind) {
+  auto DefinitionCompleter = [To]() {
+// There are cases in LLDB when we first import a class without its
+// members. The class will have DefinitionData, but no members. Then,
+// importDefinition is called from LLDB, which tries to get the members, so
+// when we get here, the class already has the DefinitionData set, so we
+// must unset the CompleteDefinition here to be able to complete again the
+// definition.
+To->setCompleteDefinition(false);
+To->completeDefinition();
+  };
+
   if (To->getDefinition() || To->isBeingDefined()) {
 if (Kind == IDK_Everything ||
 // In case of lambdas, the class already has a definition ptr set, but
@@ -1717,7 +1728,7 @@
   Error Result = ImportDeclContext(From, /*ForceImport=*/true);
   // Finish the definition of the lambda, set isBeingDefined to false.
   if (To->isLambda())
-To->completeDefinition();
+DefinitionCompleter();
   return Result;
 }
 
@@ -1728,8 +1739,8 @@
   // Complete the definition even if error is returned.
   // The RecordDecl may be already part of the AST so it is better to
   // have it in complete state even if something is wrong with it.
-  auto DefinitionCompleter =
-  llvm::make_scope_exit([To]() { To->completeDefinition(); });
+  auto DefinitionCompleterScopeExit =
+  llvm::make_scope_exit(DefinitionCompleter);
 
   if (Error Err = setTypedefNameForAnonDecl(From, To, Importer))
 return Err;
@@ -7757,10 +7768,20 @@
 SharedState->getLookupTable()->lookup(ReDC, Name);
 return FoundDeclsTy(LookupResult.begin(), LookupResult.end());
   } else {
-// FIXME Can we remove this kind of lookup?
-// Or lldb really needs this C/C++ lookup?
-FoundDeclsTy Result;
-ReDC->localUncachedLookup(Name, Result);
+DeclContext::lookup_result NoloadLookupResult = ReDC->noload_lookup(Name);
+FoundDeclsTy Result(NoloadLookupResult.begin(), NoloadLookupResult.end());
+// We must search by the slow case of localUncachedLookup because that is
+// working even if there is no LookupPtr for the DC. We could use
+// DC::buildLookup() to create the LookupPtr, but that would load external
+// decls again, we must avoid that case.
+// Also, even if we had the LookupPtr, we must find Decls which are not
+// in the LookupPtr, so we need the slow case.
+// These cases are handled in ASTImporterLookupTable, but we cannot use
+// that with LLDB since that traverses through the AST which initiates the
+// load of external decls again via DC::decls().  And again, we must avoid
+// loading external decls during the import.
+if (Result.empty())
+  ReDC->localUncachedLookup(Name, Result);
 return Result;
   }
 }
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -5122,6 +5122,51 @@
   EXPECT_EQ(ToLSize, FromLSize);
 }
 
+struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
+  LLDBLookupTest() {
+Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+ ASTContext &FromContext, FileManager &FromFileManager,
+ bool MinimalImport,
+ const std::shared_ptr &SharedState) {
+  return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport,
+ // We use the regular lookup.
+ /*SharedState=*/nullptr);
+};
+  }
+};
+
+TEST_P(LLDBLookupTest, ImporterShouldFindInTransparentContext) {
+  TranslationUnitDecl *ToTU = getToTuDecl(
+  R"(
+  extern "C" {
+class X{};
+  };
+  )",
+  Lang_CXX);
+  auto *ToX = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(ha

[Lldb-commits] [PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

Jenkins looks okay: 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31157/ .The build is 
red but the the previous run has the very same failing test case:
expression_command/weak_symbols/TestWeakSymbols.py


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61333/new/

https://reviews.llvm.org/D61333



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D92103: [ASTImporter] Import the default argument of TemplateTypeParmDecl

[Gabor Marton via Phabricator via lldb-commits]

martong added a subscriber: balazske.
martong added a comment.

Hey Raphael, thanks for looking into the CTU crash!

I also had a look and I could reproduce that on Linux Ubuntu 18.04 with GCC 7. 
I think the discrepancy stems from GCC's libstdc++ and MacOS's libc++ 
implementation differences of `basic_streambuf`. This is the CXXRecordDecl 
which does not need an injected class name type (so there is the assert). 
However, the situation is more complex because there is a 5 long redecl chain 
(streambuf is forward declared in many places).

Anyway, I don't think that this patch itself caused the crash, it just revealed 
some other issues that are related to the possibly flawed import logic of 
injected class (name) types. I am adding another college @balazske as a 
subscriber because he started to investigate the crash deeper.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92103/new/

https://reviews.llvm.org/D92103

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

Looks okay to me (other than the redundant import code that I have a comment 
about).

> Also this seems to be testable via a Clang unit test, so I think this patch 
> should have one.

Yeah, would be nice to have a Clang unit test. I believe we have the 
infrastructure for that in place. E.g. in `LLDBLookupTest` we have an lldb 
specific setup, that could be a good starting point.




Comment at: clang/lib/AST/ASTImporter.cpp:8173
+  // If a RecordDecl has base classes they won't be imported and we will
+  // be missing anything that we inherit from those bases.
+  if (FromRecord->hasExternalLexicalStorage() &&

teemperor wrote:
> I'm not sure how it can be that ASTImporter::CompleteDecl starts but never 
> finishes the decl as it does both things at once unconditionally?
> ```
> lang=c++
>   TD->startDefinition();
>   TD->setCompleteDefinition(true);
> ```
> 
> FWIW, this whole comment could just be `Ask the external source if this is 
> actually a complete record that just hasn't been completed yet` or something 
> like this. I think if we reach this point then it makes a complete sense to 
> ask the external source for more info. The explanation about the base classes 
> seems to be just a smaller detail of the whole picture here.
> Ask the external source if this is actually a complete record that just 
> hasn't been completed yet

FWIW this seems to be a recurring pattern, so maybe it would be worth to do 
this not just with `RecordDecl`s but with other kind of decls. E.g. an 
`EnumDecl` could have an external source and not  completed, couldn't it?



Comment at: clang/lib/AST/ASTImporter.cpp:8179
+
+  if (FromRecord->isCompleteDefinition())
+if (Error Err = ASTNodeImporter(*this).ImportDefinition(

We could merge this `if` with the `else if` at line 8164, because their body is 
exactly the same.
But to do that, we should move the external storage query and type completion 
above the enclosing `if` (above line 8162 and just after line 8161).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78000/new/

https://reviews.llvm.org/D78000



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

Thanks for the test Shafik, that is pretty self explanatory!




Comment at: clang/lib/AST/ASTImporter.cpp:8161
   if (auto *ToRecord = dyn_cast(ToDC)) {
 auto *FromRecord = cast(FromDC);
 if (ToRecord->isCompleteDefinition()) {

What if we did this a bit differently? We could simply complete the From type 
if it is not completed, before getting into `ImportDefinition`.
```
if (ToRecord->isCompleteDefinition())
  return ToDC;
auto *FromRecord = cast(FromDC);
if (FromRecord->hasExternalLexicalStorage() &&
  !FromRecord->isCompleteDefinition())
FromRecord->getASTContext().getExternalSource()->CompleteType(
FromRecord);
```

This way we could get rid of the redundant calls of `ImportDefinition`. As we 
have now a call for the case when we don't have external storage and for the 
case when we have.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78000/new/

https://reviews.llvm.org/D78000



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.

LGTM! Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78000/new/

https://reviews.llvm.org/D78000



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

Thanks for the patch! It look almost good to me, but I have a comment about the 
error handling.




Comment at: clang/lib/AST/ASTImporter.cpp:1707
+if (Err)
+  return Err;
+}

Rather than just simply returning with the Error, I think we should append this 
error to `ChildErrors` and go on with the next field, as this is what the 
original code did.
I am thinking about something like this:
```
  if (Err && AccumulateChildErrors)
ChildErrors =  joinErrors(std::move(ChildErrors), Err);
  else
consumeError(Err);
```

And perhaps line 1713 could be just a simple `else {`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71378/new/

https://reviews.llvm.org/D71378



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

Just one more thing, maybe that is too overkill, but I think on a long term we 
could benefit from a unittest for this case. You could create a test similar to 
`LLDBLookupTest` in ASTImporterTest.cpp. In that Fixture we use Minimal import 
and the regular lookup (that is the exact case in LLDB).
In the test itself we could call ImportDefinition on a struct that has fields 
with record types. And then we could assert that all field's type's have 
complete types.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71378/new/

https://reviews.llvm.org/D71378



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71562: [lldb] Remove modern-type-lookup

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

> I'm curious what you think should happen to the clang-import-test. We could 
> either rewrite the tests as unit tests in the ASTImporterTest you guys are 
> already using or we move the necessary parts of the ExternalASTMerger into 
> the clang-import-test

Raphael, I think there is no point to keep the clang-import-test and the 
ExternalASTMerger in Clang if we remove the only user of these things.
So, in my opinion the best would be on a long-term if we could cover these 
tests with unit tests in ASTImporterTest.
On the other hand, I understand that rewriting these tests could be quite a 
work, so perhaps we should gradually add the new unit tests and once we are 
ready then we could remove entirely the clang-import-test and the 
ExternalASTMerger.
Also, some of the tests are already covered by the existing unit tests, e.g. 
`switch-stmt` with `ImportSwitch`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71562/new/

https://reviews.llvm.org/D71562



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits