[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
bnbarham created this revision. bnbarham added reviewers: vsapsai, dexonsmith. Herald added a subscriber: kristof.beyls. bnbarham requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Reading modules first reads each control block in the chain and then all AST blocks. The first phase is intended to find recoverable errors, eg. an out of date or missing module. If any error occurs during this phase, it is safe to remove all modules in the chain as no references to them will exist. While reading the AST blocks, however, various fields in ASTReader are updated with references to the module. Removing modules at this point can cause dangling pointers which can be accessed later. These would be otherwise harmless, eg. a binary search over `GlobalSLocEntryMap` may access a failed module that could error, but shouldn't crash. Do not remove modules in this phase, regardless of failures. Since this is the case, it also doesn't make sense to return OutOfDate during this phase, so remove the two cases where this happens. When they were originally added these checks would return OutOfDate when the serialized and current path didn't match up. This is not the same as the existing check, which is now different since only the name as written is serialized. This means the OutOfDate condition is really only checking for an umbrella with a different *name* (assuming it exists in the base directory). It is also skipped completely for frameworks since these don't include `Headers/` in the name, causing no file entry to be found. Given all this, it seems safe to ignore this case entirely for now. This makes the handling of an umbrella header/directory the same as regular headers, which also don't check for differences in the path caused by VFS. Resolves rdar://79329355 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D107690 Files: clang/lib/Serialization/ASTReader.cpp clang/test/VFS/module-header-mismatches.m clang/test/VFS/umbrella-mismatch.m Index: clang/test/VFS/umbrella-mismatch.m === --- clang/test/VFS/umbrella-mismatch.m +++ /dev/null @@ -1,7 +0,0 @@ -// RUN: rm -rf %t -// RUN: sed -e "s;INPUT_DIR;%/S/Inputs;g" -e "s;OUT_DIR;%/S/Inputs;g" %S/Inputs/vfsoverlay.yaml > %t.yaml - -// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify -// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify -// expected-no-diagnostics -@import UsesFoo; Index: clang/test/VFS/module-header-mismatches.m === --- /dev/null +++ clang/test/VFS/module-header-mismatches.m @@ -0,0 +1,86 @@ +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: sed -e "s;TEST_DIR;%/t;g" %t/sed-overlay.yaml > %t/overlay.yaml + +// These tests first build with an overlay such that the header is resolved +// to %t/other/Mismatch.h. They then build again with the header resolved +// to the one in their directory. +// +// This should cause a rebuild if the contents is different (and thus multiple +// PCMs), but this currently isn't the case. We should at least not error, +// since this does happen in real projects (with a different copy of the same +// file). + +// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -ivfsoverlay %t/overlay.yaml -F %t/header-frameworks -fsyntax-only -verify %t/use.m +// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/header-frameworks -fsyntax-only -verify %t/use.m +// RUN: find %t/hf-mcp -name "Mismatch-*.pcm" | count 1 + +// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/df-mcp -ivfsoverlay %t/overlay.yaml -F %t/dir-frameworks -fsyntax-only -verify %t/use.m +// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/dir-frameworks -fsyntax-only -verify %t/use.m +// RUN: find %t/df-mcp -name "Mismatch-*.pcm" | count 1 + +// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -ivfsoverlay %t/overlay.yaml -F %t/norm-frameworks -fsyntax-only -verify %t/use.m +// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -F %t/norm-frameworks -fsyntax-only -verify %t/use.m +// RUN: find %t/nf-mcp -name "Mismatch-*.pcm" | count 1 + +// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -ivfsoverlay %t/overlay.yaml -I %t/mod -fsyntax-only -verify %t/use.m +// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -I %t/mod -fsyntax-only -verify %t/use.m +// RUN: find %t/m-mcp -name "Mismatch-*.pcm" | count 1 + +//--- use.m +// expected-no-d
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
bnbarham added a comment. I have another change to update the post-control-block functions to llvm::Error instead as well to hopefully make the distinction more clear as well. Let me know if you think it belongs in this PR, otherwise I'll open another once this one is in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107690/new/ https://reviews.llvm.org/D107690 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
vsapsai added a comment. Made the first review pass and `return Failure` makes sense to me as recovery isn't the best idea at this point. Still want to check more thoroughly if the removed code for `SUBMODULE_UMBRELLA_HEADER` and `SUBMODULE_UMBRELLA_DIR` has any load-bearing side-effects. Have no opinion on updating post-control-block functions to llvm::Error. Comment at: clang/test/VFS/module-header-mismatches.m:2 +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: sed -e "s;TEST_DIR;%/t;g" %t/sed-overlay.yaml > %t/overlay.yaml Didn't know about `split-file`, that's nice. Comment at: clang/test/VFS/umbrella-mismatch.m:4 - -// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify -// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify Are you deleting this test case because it is subsumed by module-header-mismatches.m? Also if it makes sense to delete this test, need to do more cleanup because UsesFoo.framework and Foo.framework seem to be used only from this test. Though I'm not 100% sure. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107690/new/ https://reviews.llvm.org/D107690 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
bnbarham added inline comments. Comment at: clang/test/VFS/umbrella-mismatch.m:4 - -// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify -// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify vsapsai wrote: > Are you deleting this test case because it is subsumed by > module-header-mismatches.m? > > Also if it makes sense to delete this test, need to do more cleanup because > UsesFoo.framework and Foo.framework seem to be used only from this test. > Though I'm not 100% sure. Yep, the check in this one is the same as the `header-frameworks`. `Foo.framework` is used by `subframework-symlink.m`. But `UsesFoo.framework` isn't used by anything else, thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107690/new/ https://reviews.llvm.org/D107690 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
bnbarham updated this revision to Diff 365619.
bnbarham added a comment.
Removed the now unused UsesFoo.framework in the tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107690/new/
https://reviews.llvm.org/D107690
Files:
clang/lib/Serialization/ASTReader.cpp
clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h
clang/test/VFS/Inputs/UsesFoo.framework/Modules/module.modulemap
clang/test/VFS/module-header-mismatches.m
clang/test/VFS/umbrella-mismatch.m
Index: clang/test/VFS/umbrella-mismatch.m
===
--- clang/test/VFS/umbrella-mismatch.m
+++ /dev/null
@@ -1,7 +0,0 @@
-// RUN: rm -rf %t
-// RUN: sed -e "s;INPUT_DIR;%/S/Inputs;g" -e "s;OUT_DIR;%/S/Inputs;g" %S/Inputs/vfsoverlay.yaml > %t.yaml
-
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// expected-no-diagnostics
-@import UsesFoo;
Index: clang/test/VFS/module-header-mismatches.m
===
--- /dev/null
+++ clang/test/VFS/module-header-mismatches.m
@@ -0,0 +1,86 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s;TEST_DIR;%/t;g" %t/sed-overlay.yaml > %t/overlay.yaml
+
+// These tests first build with an overlay such that the header is resolved
+// to %t/other/Mismatch.h. They then build again with the header resolved
+// to the one in their directory.
+//
+// This should cause a rebuild if the contents is different (and thus multiple
+// PCMs), but this currently isn't the case. We should at least not error,
+// since this does happen in real projects (with a different copy of the same
+// file).
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -ivfsoverlay %t/overlay.yaml -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/hf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/df-mcp -ivfsoverlay %t/overlay.yaml -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/df-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -ivfsoverlay %t/overlay.yaml -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/nf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -ivfsoverlay %t/overlay.yaml -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: find %t/m-mcp -name "Mismatch-*.pcm" | count 1
+
+//--- use.m
+// expected-no-diagnostics
+@import Mismatch;
+
+//--- header-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ umbrella header "Mismatch.h"
+}
+//--- header-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- dir-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ umbrella "someheaders"
+}
+//--- dir-frameworks/Mismatch.framework/someheaders/Mismatch.h
+
+//--- norm-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ header "Mismatch.h"
+}
+//--- norm-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- mod/module.modulemap
+module Mismatch {
+ umbrella header "Mismatch.h"
+}
+//--- mod/Mismatch.h
+
+//--- other/Mismatch.h
+
+//--- sed-overlay.yaml
+{
+ 'version': 0,
+ 'roots': [
+{ 'name': 'TEST_DIR', 'type': 'directory',
+ 'contents': [
+{ 'name': 'header-frameworks/Mismatch.framework/Headers/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+},
+{ 'name': 'dir-frameworks/Mismatch.framework/someheaders',
+ 'type': 'directory',
+ 'external-contents': 'TEST_DIR/others'
+},
+{ 'name': 'norm-frameworks/Mismatch.framework/Headers/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+},
+{ 'name': 'mod/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+}
+
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
vsapsai added a comment. Why are you not just changing `return OutOfDate` to `return Failure` for SUBMODULE_UMBRELLA_HEADER and SUBMODULE_UMBRELLA_DIR? I have no strong opinion on this but it feels more in line with the rest of the changes. Also I've tried to do that and nothing seems to be broken. And for the rest of the code for these records I think it is worth keeping `ModMap.setUmbrellaHeader` and `ModMap.setUmbrellaDir` with preceding preparation and checks. The code in ModuleMap seems to be complicated enough and stateful, so removing those setters can break some stuff. It is possible everything will be fine but I don't want to chance that unless required. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107690/new/ https://reviews.llvm.org/D107690 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
bnbarham updated this revision to Diff 366152.
bnbarham marked an inline comment as done.
bnbarham edited the summary of this revision.
bnbarham added a comment.
Changed to keep setting the umbrella header/directory with a FIXME that it
currently doesn't work for framework modules.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107690/new/
https://reviews.llvm.org/D107690
Files:
clang/lib/Serialization/ASTReader.cpp
clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h
clang/test/VFS/Inputs/UsesFoo.framework/Modules/module.modulemap
clang/test/VFS/module-header-mismatches.m
clang/test/VFS/umbrella-mismatch.m
Index: clang/test/VFS/umbrella-mismatch.m
===
--- clang/test/VFS/umbrella-mismatch.m
+++ /dev/null
@@ -1,7 +0,0 @@
-// RUN: rm -rf %t
-// RUN: sed -e "s;INPUT_DIR;%/S/Inputs;g" -e "s;OUT_DIR;%/S/Inputs;g" %S/Inputs/vfsoverlay.yaml > %t.yaml
-
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// expected-no-diagnostics
-@import UsesFoo;
Index: clang/test/VFS/module-header-mismatches.m
===
--- /dev/null
+++ clang/test/VFS/module-header-mismatches.m
@@ -0,0 +1,86 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s;TEST_DIR;%/t;g" %t/sed-overlay.yaml > %t/overlay.yaml
+
+// These tests first build with an overlay such that the header is resolved
+// to %t/other/Mismatch.h. They then build again with the header resolved
+// to the one in their directory.
+//
+// This should cause a rebuild if the contents is different (and thus multiple
+// PCMs), but this currently isn't the case. We should at least not error,
+// since this does happen in real projects (with a different copy of the same
+// file).
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -ivfsoverlay %t/overlay.yaml -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/hf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/df-mcp -ivfsoverlay %t/overlay.yaml -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/df-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -ivfsoverlay %t/overlay.yaml -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/nf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -ivfsoverlay %t/overlay.yaml -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: find %t/m-mcp -name "Mismatch-*.pcm" | count 1
+
+//--- use.m
+// expected-no-diagnostics
+@import Mismatch;
+
+//--- header-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ umbrella header "Mismatch.h"
+}
+//--- header-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- dir-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ umbrella "someheaders"
+}
+//--- dir-frameworks/Mismatch.framework/someheaders/Mismatch.h
+
+//--- norm-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ header "Mismatch.h"
+}
+//--- norm-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- mod/module.modulemap
+module Mismatch {
+ umbrella header "Mismatch.h"
+}
+//--- mod/Mismatch.h
+
+//--- other/Mismatch.h
+
+//--- sed-overlay.yaml
+{
+ 'version': 0,
+ 'roots': [
+{ 'name': 'TEST_DIR', 'type': 'directory',
+ 'contents': [
+{ 'name': 'header-frameworks/Mismatch.framework/Headers/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+},
+{ 'name': 'dir-frameworks/Mismatch.framework/someheaders',
+ 'type': 'directory',
+ 'external-contents': 'TEST_DIR/others'
+},
+{ 'name': 'norm-frameworks/Mismatch.framework/Headers/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mism
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
vsapsai accepted this revision. vsapsai added a comment. This revision is now accepted and ready to land. Looks good to me. The only nitpicky thing is I don't remember if the code style requires braces around multi-line case blocks or not. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107690/new/ https://reviews.llvm.org/D107690 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
bnbarham updated this revision to Diff 366154.
bnbarham added a comment.
Forgot to add the braces back in for the case statements
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107690/new/
https://reviews.llvm.org/D107690
Files:
clang/lib/Serialization/ASTReader.cpp
clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h
clang/test/VFS/Inputs/UsesFoo.framework/Modules/module.modulemap
clang/test/VFS/module-header-mismatches.m
clang/test/VFS/umbrella-mismatch.m
Index: clang/test/VFS/umbrella-mismatch.m
===
--- clang/test/VFS/umbrella-mismatch.m
+++ /dev/null
@@ -1,7 +0,0 @@
-// RUN: rm -rf %t
-// RUN: sed -e "s;INPUT_DIR;%/S/Inputs;g" -e "s;OUT_DIR;%/S/Inputs;g" %S/Inputs/vfsoverlay.yaml > %t.yaml
-
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// expected-no-diagnostics
-@import UsesFoo;
Index: clang/test/VFS/module-header-mismatches.m
===
--- /dev/null
+++ clang/test/VFS/module-header-mismatches.m
@@ -0,0 +1,86 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s;TEST_DIR;%/t;g" %t/sed-overlay.yaml > %t/overlay.yaml
+
+// These tests first build with an overlay such that the header is resolved
+// to %t/other/Mismatch.h. They then build again with the header resolved
+// to the one in their directory.
+//
+// This should cause a rebuild if the contents is different (and thus multiple
+// PCMs), but this currently isn't the case. We should at least not error,
+// since this does happen in real projects (with a different copy of the same
+// file).
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -ivfsoverlay %t/overlay.yaml -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/hf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/df-mcp -ivfsoverlay %t/overlay.yaml -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/df-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -ivfsoverlay %t/overlay.yaml -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/nf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -ivfsoverlay %t/overlay.yaml -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: find %t/m-mcp -name "Mismatch-*.pcm" | count 1
+
+//--- use.m
+// expected-no-diagnostics
+@import Mismatch;
+
+//--- header-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ umbrella header "Mismatch.h"
+}
+//--- header-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- dir-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ umbrella "someheaders"
+}
+//--- dir-frameworks/Mismatch.framework/someheaders/Mismatch.h
+
+//--- norm-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ header "Mismatch.h"
+}
+//--- norm-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- mod/module.modulemap
+module Mismatch {
+ umbrella header "Mismatch.h"
+}
+//--- mod/Mismatch.h
+
+//--- other/Mismatch.h
+
+//--- sed-overlay.yaml
+{
+ 'version': 0,
+ 'roots': [
+{ 'name': 'TEST_DIR', 'type': 'directory',
+ 'contents': [
+{ 'name': 'header-frameworks/Mismatch.framework/Headers/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+},
+{ 'name': 'dir-frameworks/Mismatch.framework/someheaders',
+ 'type': 'directory',
+ 'external-contents': 'TEST_DIR/others'
+},
+{ 'name': 'norm-frameworks/Mismatch.framework/Headers/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+},
+{ 'name': 'mod/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+}
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
bnbarham updated this revision to Diff 366378.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107690/new/
https://reviews.llvm.org/D107690
Files:
clang/lib/Serialization/ASTReader.cpp
clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h
clang/test/VFS/Inputs/UsesFoo.framework/Modules/module.modulemap
clang/test/VFS/module-header-mismatches.m
clang/test/VFS/umbrella-mismatch.m
Index: clang/test/VFS/umbrella-mismatch.m
===
--- clang/test/VFS/umbrella-mismatch.m
+++ /dev/null
@@ -1,7 +0,0 @@
-// RUN: rm -rf %t
-// RUN: sed -e "s;INPUT_DIR;%/S/Inputs;g" -e "s;OUT_DIR;%/S/Inputs;g" %S/Inputs/vfsoverlay.yaml > %t.yaml
-
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// expected-no-diagnostics
-@import UsesFoo;
Index: clang/test/VFS/module-header-mismatches.m
===
--- /dev/null
+++ clang/test/VFS/module-header-mismatches.m
@@ -0,0 +1,86 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s;TEST_DIR;%/t;g" %t/sed-overlay.yaml > %t/overlay.yaml
+
+// These tests first build with an overlay such that the header is resolved
+// to %t/other/Mismatch.h. They then build again with the header resolved
+// to the one in their directory.
+//
+// This should cause a rebuild if the contents is different (and thus multiple
+// PCMs), but this currently isn't the case. We should at least not error,
+// since this does happen in real projects (with a different copy of the same
+// file).
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -ivfsoverlay %t/overlay.yaml -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/hf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/df-mcp -ivfsoverlay %t/overlay.yaml -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/df-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -ivfsoverlay %t/overlay.yaml -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/nf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -ivfsoverlay %t/overlay.yaml -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: find %t/m-mcp -name "Mismatch-*.pcm" | count 1
+
+//--- use.m
+// expected-no-diagnostics
+@import Mismatch;
+
+//--- header-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ umbrella header "Mismatch.h"
+}
+//--- header-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- dir-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ umbrella "someheaders"
+}
+//--- dir-frameworks/Mismatch.framework/someheaders/Mismatch.h
+
+//--- norm-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ header "Mismatch.h"
+}
+//--- norm-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- mod/module.modulemap
+module Mismatch {
+ umbrella header "Mismatch.h"
+}
+//--- mod/Mismatch.h
+
+//--- other/Mismatch.h
+
+//--- sed-overlay.yaml
+{
+ 'version': 0,
+ 'roots': [
+{ 'name': 'TEST_DIR', 'type': 'directory',
+ 'contents': [
+{ 'name': 'header-frameworks/Mismatch.framework/Headers/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+},
+{ 'name': 'dir-frameworks/Mismatch.framework/someheaders',
+ 'type': 'directory',
+ 'external-contents': 'TEST_DIR/others'
+},
+{ 'name': 'norm-frameworks/Mismatch.framework/Headers/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+},
+{ 'name': 'mod/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+}
+ ]
+}
+ ]
+}
+
Index: clang/test/VFS/Inputs/UsesFoo.framework/Modules/mod
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
This revision was automatically updated to reflect the committed changes.
Closed by commit rG32208555af26: [Modules] Do not remove failed modules after
the control block phase (authored by bnbarham, committed by vsapsai).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107690/new/
https://reviews.llvm.org/D107690
Files:
clang/lib/Serialization/ASTReader.cpp
clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h
clang/test/VFS/Inputs/UsesFoo.framework/Modules/module.modulemap
clang/test/VFS/module-header-mismatches.m
clang/test/VFS/umbrella-mismatch.m
Index: clang/test/VFS/umbrella-mismatch.m
===
--- clang/test/VFS/umbrella-mismatch.m
+++ /dev/null
@@ -1,7 +0,0 @@
-// RUN: rm -rf %t
-// RUN: sed -e "s;INPUT_DIR;%/S/Inputs;g" -e "s;OUT_DIR;%/S/Inputs;g" %S/Inputs/vfsoverlay.yaml > %t.yaml
-
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// expected-no-diagnostics
-@import UsesFoo;
Index: clang/test/VFS/module-header-mismatches.m
===
--- /dev/null
+++ clang/test/VFS/module-header-mismatches.m
@@ -0,0 +1,86 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s;TEST_DIR;%/t;g" %t/sed-overlay.yaml > %t/overlay.yaml
+
+// These tests first build with an overlay such that the header is resolved
+// to %t/other/Mismatch.h. They then build again with the header resolved
+// to the one in their directory.
+//
+// This should cause a rebuild if the contents is different (and thus multiple
+// PCMs), but this currently isn't the case. We should at least not error,
+// since this does happen in real projects (with a different copy of the same
+// file).
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -ivfsoverlay %t/overlay.yaml -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/hf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/df-mcp -ivfsoverlay %t/overlay.yaml -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/df-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -ivfsoverlay %t/overlay.yaml -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/nf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -ivfsoverlay %t/overlay.yaml -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: find %t/m-mcp -name "Mismatch-*.pcm" | count 1
+
+//--- use.m
+// expected-no-diagnostics
+@import Mismatch;
+
+//--- header-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ umbrella header "Mismatch.h"
+}
+//--- header-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- dir-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ umbrella "someheaders"
+}
+//--- dir-frameworks/Mismatch.framework/someheaders/Mismatch.h
+
+//--- norm-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ header "Mismatch.h"
+}
+//--- norm-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- mod/module.modulemap
+module Mismatch {
+ umbrella header "Mismatch.h"
+}
+//--- mod/Mismatch.h
+
+//--- other/Mismatch.h
+
+//--- sed-overlay.yaml
+{
+ 'version': 0,
+ 'roots': [
+{ 'name': 'TEST_DIR', 'type': 'directory',
+ 'contents': [
+{ 'name': 'header-frameworks/Mismatch.framework/Headers/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+},
+{ 'name': 'dir-frameworks/Mismatch.framework/someheaders',
+ 'type': 'directory',
+ 'external-contents': 'TEST_DIR/others'
+},
+{ 'name': 'norm-frameworks/Mismatch.framework/Headers/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+},
+{ 'name': 'mod/Mismatch.h',
+
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
yaron.keren added inline comments.
Comment at: clang/lib/Serialization/ASTReader.cpp:4268
// Read the AST block.
if (ASTReadResult Result = ReadASTBlock(F, ClientLoadCapabilities))
+ return Failure;
Result is unused now.
Comment at: clang/lib/Serialization/ASTReader.cpp:4279
while (!SkipCursorToBlock(F.Stream, EXTENSION_BLOCK_ID)) {
if (ASTReadResult Result = ReadExtensionBlock(F))
+return Failure;
Same here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107690/new/
https://reviews.llvm.org/D107690
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
vsapsai added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:4268 // Read the AST block. if (ASTReadResult Result = ReadASTBlock(F, ClientLoadCapabilities)) + return Failure; yaron.keren wrote: > Result is unused now. Thanks for pointing it out. Are there any bots failing because of that now? Asking if should have a small urgent fix or if can wait for https://reviews.llvm.org/D108268 to land. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107690/new/ https://reviews.llvm.org/D107690 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase
yaron.keren added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:4268 // Read the AST block. if (ASTReadResult Result = ReadASTBlock(F, ClientLoadCapabilities)) + return Failure; vsapsai wrote: > yaron.keren wrote: > > Result is unused now. > Thanks for pointing it out. Are there any bots failing because of that now? > Asking if should have a small urgent fix or if can wait for > https://reviews.llvm.org/D108268 to land. Just a compiler warning, could wait. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107690/new/ https://reviews.llvm.org/D107690 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
