[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.
This revision was automatically updated to reflect the committed changes. Closed by commit rG7ac737e56bee: [HeaderSearch] Fix processing #import-ed headers multiple times with modules… (authored by vsapsai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80263/new/ https://reviews.llvm.org/D80263 Files: clang/lib/Lex/HeaderSearch.cpp clang/test/Modules/Inputs/import-once/ImportOnce.framework/Headers/ImportOnce.h clang/test/Modules/Inputs/import-once/ImportOnce.framework/Modules/module.modulemap clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Headers/IndirectImporter.h clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Modules/module.modulemap clang/test/Modules/Inputs/import-once/Unrelated.framework/Headers/Unrelated.h clang/test/Modules/Inputs/import-once/Unrelated.framework/Modules/module.modulemap clang/test/Modules/import-once.m Index: clang/test/Modules/import-once.m === --- /dev/null +++ clang/test/Modules/import-once.m @@ -0,0 +1,15 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodules -fmodule-name=ImportOnce -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/import-once %s + +// Test #import-ed headers are processed only once, even without header guards. +// Dependency graph is +// +// Unrelated ImportOnce +// ^ ^^ +//\/ | +// IndirectImporter| +// ^ | +// \| +// import-once.m +#import +#import Index: clang/test/Modules/Inputs/import-once/Unrelated.framework/Modules/module.modulemap === --- /dev/null +++ clang/test/Modules/Inputs/import-once/Unrelated.framework/Modules/module.modulemap @@ -0,0 +1,4 @@ +framework module Unrelated { +umbrella header "Unrelated.h" +export * +} Index: clang/test/Modules/Inputs/import-once/Unrelated.framework/Headers/Unrelated.h === --- /dev/null +++ clang/test/Modules/Inputs/import-once/Unrelated.framework/Headers/Unrelated.h @@ -0,0 +1 @@ +void foo(void); Index: clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Modules/module.modulemap === --- /dev/null +++ clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Modules/module.modulemap @@ -0,0 +1,4 @@ +framework module IndirectImporter { +umbrella header "IndirectImporter.h" +export * +} Index: clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Headers/IndirectImporter.h === --- /dev/null +++ clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Headers/IndirectImporter.h @@ -0,0 +1,2 @@ +#import +#import Index: clang/test/Modules/Inputs/import-once/ImportOnce.framework/Modules/module.modulemap === --- /dev/null +++ clang/test/Modules/Inputs/import-once/ImportOnce.framework/Modules/module.modulemap @@ -0,0 +1,4 @@ +framework module ImportOnce { + umbrella header "ImportOnce.h" + export * +} Index: clang/test/Modules/Inputs/import-once/ImportOnce.framework/Headers/ImportOnce.h === --- /dev/null +++ clang/test/Modules/Inputs/import-once/ImportOnce.framework/Headers/ImportOnce.h @@ -0,0 +1,5 @@ +// No header guards on purpose. + +enum SomeSimpleEnum { +SomeSimpleEnumCase, +}; Index: clang/lib/Lex/HeaderSearch.cpp === --- clang/lib/Lex/HeaderSearch.cpp +++ clang/lib/Lex/HeaderSearch.cpp @@ -1167,12 +1167,12 @@ HeaderFileInfo *HFI = [FE->getUID()]; // FIXME: Use a generation count to check whether this is really up to date. if (ExternalSource && !HFI->Resolved) { -HFI->Resolved = true; auto ExternalHFI = ExternalSource->GetHeaderFileInfo(FE); - -HFI = [FE->getUID()]; -if (ExternalHFI.External) - mergeHeaderFileInfo(*HFI, ExternalHFI); +if (ExternalHFI.IsValid) { + HFI->Resolved = true; + if (ExternalHFI.External) +mergeHeaderFileInfo(*HFI, ExternalHFI); +} } HFI->IsValid = true; @@ -1199,12 +1199,12 @@ if (!WantExternal && (!HFI->IsValid || HFI->External)) return nullptr; if (!HFI->Resolved) { - HFI->Resolved = true; auto ExternalHFI = ExternalSource->GetHeaderFileInfo(FE); - - HFI = [FE->getUID()]; - if (ExternalHFI.External) -mergeHeaderFileInfo(*HFI, ExternalHFI); + if (ExternalHFI.IsValid) { +HFI->Resolved = true; +if (ExternalHFI.External) + mergeHeaderFileInfo(*HFI, ExternalHFI); + } } } else if (FE->getUID() >= FileInfo.size()) { return
[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.
vsapsai added a comment. Thanks for the review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80263/new/ https://reviews.llvm.org/D80263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.
bruno accepted this revision. bruno added a comment. This revision is now accepted and ready to land. Nice catch! LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80263/new/ https://reviews.llvm.org/D80263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.
vsapsai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80263/new/ https://reviews.llvm.org/D80263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.
vsapsai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80263/new/ https://reviews.llvm.org/D80263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.
vsapsai updated this revision to Diff 282035. vsapsai added a comment. Rebased the change to make sure it still works. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80263/new/ https://reviews.llvm.org/D80263 Files: clang/lib/Lex/HeaderSearch.cpp clang/test/Modules/Inputs/import-once/ImportOnce.framework/Headers/ImportOnce.h clang/test/Modules/Inputs/import-once/ImportOnce.framework/Modules/module.modulemap clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Headers/IndirectImporter.h clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Modules/module.modulemap clang/test/Modules/Inputs/import-once/Unrelated.framework/Headers/Unrelated.h clang/test/Modules/Inputs/import-once/Unrelated.framework/Modules/module.modulemap clang/test/Modules/import-once.m Index: clang/test/Modules/import-once.m === --- /dev/null +++ clang/test/Modules/import-once.m @@ -0,0 +1,15 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodules -fmodule-name=ImportOnce -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/import-once %s + +// Test #import-ed headers are processed only once, even without header guards. +// Dependency graph is +// +// Unrelated ImportOnce +// ^ ^^ +//\/ | +// IndirectImporter| +// ^ | +// \| +// import-once.m +#import +#import Index: clang/test/Modules/Inputs/import-once/Unrelated.framework/Modules/module.modulemap === --- /dev/null +++ clang/test/Modules/Inputs/import-once/Unrelated.framework/Modules/module.modulemap @@ -0,0 +1,4 @@ +framework module Unrelated { +umbrella header "Unrelated.h" +export * +} Index: clang/test/Modules/Inputs/import-once/Unrelated.framework/Headers/Unrelated.h === --- /dev/null +++ clang/test/Modules/Inputs/import-once/Unrelated.framework/Headers/Unrelated.h @@ -0,0 +1 @@ +void foo(void); Index: clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Modules/module.modulemap === --- /dev/null +++ clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Modules/module.modulemap @@ -0,0 +1,4 @@ +framework module IndirectImporter { +umbrella header "IndirectImporter.h" +export * +} Index: clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Headers/IndirectImporter.h === --- /dev/null +++ clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Headers/IndirectImporter.h @@ -0,0 +1,2 @@ +#import +#import Index: clang/test/Modules/Inputs/import-once/ImportOnce.framework/Modules/module.modulemap === --- /dev/null +++ clang/test/Modules/Inputs/import-once/ImportOnce.framework/Modules/module.modulemap @@ -0,0 +1,4 @@ +framework module ImportOnce { + umbrella header "ImportOnce.h" + export * +} Index: clang/test/Modules/Inputs/import-once/ImportOnce.framework/Headers/ImportOnce.h === --- /dev/null +++ clang/test/Modules/Inputs/import-once/ImportOnce.framework/Headers/ImportOnce.h @@ -0,0 +1,5 @@ +// No header guards on purpose. + +enum SomeSimpleEnum { +SomeSimpleEnumCase, +}; Index: clang/lib/Lex/HeaderSearch.cpp === --- clang/lib/Lex/HeaderSearch.cpp +++ clang/lib/Lex/HeaderSearch.cpp @@ -1167,12 +1167,12 @@ HeaderFileInfo *HFI = [FE->getUID()]; // FIXME: Use a generation count to check whether this is really up to date. if (ExternalSource && !HFI->Resolved) { -HFI->Resolved = true; auto ExternalHFI = ExternalSource->GetHeaderFileInfo(FE); - -HFI = [FE->getUID()]; -if (ExternalHFI.External) - mergeHeaderFileInfo(*HFI, ExternalHFI); +if (ExternalHFI.IsValid) { + HFI->Resolved = true; + if (ExternalHFI.External) +mergeHeaderFileInfo(*HFI, ExternalHFI); +} } HFI->IsValid = true; @@ -1199,12 +1199,12 @@ if (!WantExternal && (!HFI->IsValid || HFI->External)) return nullptr; if (!HFI->Resolved) { - HFI->Resolved = true; auto ExternalHFI = ExternalSource->GetHeaderFileInfo(FE); - - HFI = [FE->getUID()]; - if (ExternalHFI.External) -mergeHeaderFileInfo(*HFI, ExternalHFI); + if (ExternalHFI.IsValid) { +HFI->Resolved = true; +if (ExternalHFI.External) + mergeHeaderFileInfo(*HFI, ExternalHFI); + } } } else if (FE->getUID() >= FileInfo.size()) { return nullptr; ___ cfe-commits mailing list
[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.
vsapsai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80263/new/ https://reviews.llvm.org/D80263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.
vsapsai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80263/new/ https://reviews.llvm.org/D80263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.
vsapsai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80263/new/ https://reviews.llvm.org/D80263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.
vsapsai added a comment. Ping. Also suggested clang-format linter changes make the test useless as the order of imports is important. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80263/new/ https://reviews.llvm.org/D80263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.
vsapsai created this revision. vsapsai added reviewers: rsmith, bruno, Bigcheese. Herald added subscribers: ributzka, dexonsmith, jkorous. Herald added a project: clang. HeaderSearch was marking requested HeaderFileInfo as Resolved only based on the presence of ExternalSource. As the result, using any module was enough to set ExternalSource and headers unknown to this module would have HeaderFileInfo with empty fields, including `isImport = 0`, `NumIncludes = 0`. Such HeaderFileInfo was preserved without changes regardless of how the header was used in other modules and caused incorrect result in `HeaderSearch::ShouldEnterIncludeFile`. Fix by marking HeaderFileInfo as Resolved only if ExternalSource knows about this header. rdar://problem/62126911 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D80263 Files: clang/lib/Lex/HeaderSearch.cpp clang/test/Modules/Inputs/import-once/ImportOnce.framework/Headers/ImportOnce.h clang/test/Modules/Inputs/import-once/ImportOnce.framework/Modules/module.modulemap clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Headers/IndirectImporter.h clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Modules/module.modulemap clang/test/Modules/Inputs/import-once/Unrelated.framework/Headers/Unrelated.h clang/test/Modules/Inputs/import-once/Unrelated.framework/Modules/module.modulemap clang/test/Modules/import-once.m Index: clang/test/Modules/import-once.m === --- /dev/null +++ clang/test/Modules/import-once.m @@ -0,0 +1,15 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodules -fmodule-name=ImportOnce -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/import-once %s + +// Test #import-ed headers are processed only once, even without header guards. +// Dependency graph is +// +// Unrelated ImportOnce +// ^ ^^ +//\/ | +// IndirectImporter| +// ^ | +// \| +// import-once.m +#import +#import Index: clang/test/Modules/Inputs/import-once/Unrelated.framework/Modules/module.modulemap === --- /dev/null +++ clang/test/Modules/Inputs/import-once/Unrelated.framework/Modules/module.modulemap @@ -0,0 +1,4 @@ +framework module Unrelated { +umbrella header "Unrelated.h" +export * +} Index: clang/test/Modules/Inputs/import-once/Unrelated.framework/Headers/Unrelated.h === --- /dev/null +++ clang/test/Modules/Inputs/import-once/Unrelated.framework/Headers/Unrelated.h @@ -0,0 +1 @@ +void foo(void); Index: clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Modules/module.modulemap === --- /dev/null +++ clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Modules/module.modulemap @@ -0,0 +1,4 @@ +framework module IndirectImporter { +umbrella header "IndirectImporter.h" +export * +} Index: clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Headers/IndirectImporter.h === --- /dev/null +++ clang/test/Modules/Inputs/import-once/IndirectImporter.framework/Headers/IndirectImporter.h @@ -0,0 +1,2 @@ +#import +#import Index: clang/test/Modules/Inputs/import-once/ImportOnce.framework/Modules/module.modulemap === --- /dev/null +++ clang/test/Modules/Inputs/import-once/ImportOnce.framework/Modules/module.modulemap @@ -0,0 +1,4 @@ +framework module ImportOnce { + umbrella header "ImportOnce.h" + export * +} Index: clang/test/Modules/Inputs/import-once/ImportOnce.framework/Headers/ImportOnce.h === --- /dev/null +++ clang/test/Modules/Inputs/import-once/ImportOnce.framework/Headers/ImportOnce.h @@ -0,0 +1,5 @@ +// No header guards on purpose. + +enum SomeSimpleEnum { +SomeSimpleEnumCase, +}; Index: clang/lib/Lex/HeaderSearch.cpp === --- clang/lib/Lex/HeaderSearch.cpp +++ clang/lib/Lex/HeaderSearch.cpp @@ -1167,12 +1167,12 @@ HeaderFileInfo *HFI = [FE->getUID()]; // FIXME: Use a generation count to check whether this is really up to date. if (ExternalSource && !HFI->Resolved) { -HFI->Resolved = true; auto ExternalHFI = ExternalSource->GetHeaderFileInfo(FE); - -HFI = [FE->getUID()]; -if (ExternalHFI.External) - mergeHeaderFileInfo(*HFI, ExternalHFI); +if (ExternalHFI.IsValid) { + HFI->Resolved = true; + if (ExternalHFI.External) +mergeHeaderFileInfo(*HFI, ExternalHFI); +} } HFI->IsValid = true; @@ -1199,12 +1199,12 @@ if (!WantExternal && (!HFI->IsValid || HFI->External))