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

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/t

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

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://r

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

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

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

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 LAS

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

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. Rap

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

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

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

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

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

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_a

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

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

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

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

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

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 subseq

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

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

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

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 asser

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

martong added a comment. In D59485#1444673 , @teemperor wrote: > In D59485#1439628 , @martong wrote: > > > In D59485#1439570 , @martong wrote: > > > > > In D59485#1439390

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

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

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

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 e

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

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/mailma

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

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

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

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; +};

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

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

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

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

[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

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

[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

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 = To

[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

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 te

[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

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) temporar

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

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.getTranslationUnitDe

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

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 discov

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

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-comm

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

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 Git

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

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árto

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

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

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

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); }

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

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/AST

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

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/n

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

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

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

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

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

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

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

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: r

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

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 wro

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

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 wro

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

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 import

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

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.ll

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

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); ---

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

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_

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

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) {

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

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 localUncac

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

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")`

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

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://

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

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

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

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 :

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

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

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

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/ http

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

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

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

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

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

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 typ

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

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

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

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/AS

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

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 *

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

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.l

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

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 *

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

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://revie

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

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/ASTIm

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

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:

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

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

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

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/

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

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 sa

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

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

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

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* lo

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

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

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

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/sour

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

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: rLL

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

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 maili

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

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/un

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

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 (passe

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

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@list

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

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/ASTImpo

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

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. >

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

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(

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

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 SIN

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

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.or

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

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 comm

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

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 htt

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

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 `

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

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 fo

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

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()) { Wha

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

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

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

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 retu

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

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

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

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 th