r300380 - Modules: Do not serialize #pragma pack state
Duncan P. N. Exon Smith via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 14 17:07:58 PDT 2017
Author: dexonsmith
Date: Fri Apr 14 19:07:57 2017
New Revision: 300380
URL: http://llvm.org/viewvc/llvm-project?rev=300380&view=rev
Log:
Modules: Do not serialize #pragma pack state
The modules side of r299226, which serializes #pragma pack state,
doesn't work well.
The main purpose was to make -include and -include-pch match semantics
(the PCH side). We also started serializing #pragma pack in PCMs, in
the hopes of making modules and non-modules builds more consistent. But
consider:
$ cat a.h
$ cat b.h
#pragma pack(push, 2)
$ cat module.modulemap
module M {
module a { header "a.h" }
module b { header "b.h" }
}
$ cat t.cpp
#include "a.h"
#pragma pack(show)
As of r299226, the #pragma pack(show) gives "2", even though we've only
included "a.h".
- With -fmodules-local-submodule-visibility, this is clearly wrong. We
should get the default state (8 on x86_64).
- Without -fmodules-local-submodule-visibility, this kind of matches how
other things work (as if include-the-whole-module), but it's still
really terrible, and it doesn't actually make modules and non-modules
builds more consistent.
This commit disables the serialization for modules, essentially a
partial revert of r299226.
Going forward:
1. Having this #pragma pack stuff escape is terrible design (or, more
often, a horrible bug). We should prioritize adding warnings (maybe
-Werror by default?).
2. If we eventually reintroduce this for modules, it should only apply
to -fmodules-local-submodule-visibility, and it should be tracked on
a per-submodule basis.
Added:
cfe/trunk/test/Modules/pragma-pack.cpp
Removed:
cfe/trunk/test/Modules/Inputs/pragma_pack_push.h
cfe/trunk/test/Modules/Inputs/pragma_pack_reset_push.h
cfe/trunk/test/Modules/pragma-pack.c
Modified:
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/Modules/Inputs/module.map
Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=300380&r1=300379&r2=300380&view=diff
==============================================================================
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Fri Apr 14 19:07:57 2017
@@ -4186,6 +4186,11 @@ void ASTWriter::WriteMSPointersToMembers
/// \brief Write the state of 'pragma pack' at the end of the module.
void ASTWriter::WritePackPragmaOptions(Sema &SemaRef) {
+ // Don't serialize pragma pack state for modules, since it should only take
+ // effect on a per-submodule basis.
+ if (WritingModule)
+ return;
+
RecordData Record;
Record.push_back(SemaRef.PackStack.CurrentValue);
AddSourceLocation(SemaRef.PackStack.CurrentPragmaLocation, Record);
Modified: cfe/trunk/test/Modules/Inputs/module.map
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/module.map?rev=300380&r1=300379&r2=300380&view=diff
==============================================================================
--- cfe/trunk/test/Modules/Inputs/module.map (original)
+++ cfe/trunk/test/Modules/Inputs/module.map Fri Apr 14 19:07:57 2017
@@ -265,20 +265,9 @@ module diag_pragma {
header "diag_pragma.h"
}
-module pragma_pack_set {
- header "pragma_pack_set.h"
-}
-
-module pragma_pack_push {
- header "pragma_pack_push.h"
-}
-
-module pragma_pack_empty {
- header "empty.h"
-}
-
-module pragma_pack_reset_push {
- header "pragma_pack_reset_push.h"
+module pragma_pack {
+ module set { header "pragma_pack_set.h" }
+ module empty { header "empty.h" }
}
module dummy {
Removed: cfe/trunk/test/Modules/Inputs/pragma_pack_push.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/pragma_pack_push.h?rev=300379&view=auto
==============================================================================
--- cfe/trunk/test/Modules/Inputs/pragma_pack_push.h (original)
+++ cfe/trunk/test/Modules/Inputs/pragma_pack_push.h (removed)
@@ -1,6 +0,0 @@
-
-#pragma pack (push, 4)
-#pragma pack (push, 2)
-#pragma pack (push, 1)
-#pragma pack (pop)
-
Removed: cfe/trunk/test/Modules/Inputs/pragma_pack_reset_push.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/pragma_pack_reset_push.h?rev=300379&view=auto
==============================================================================
--- cfe/trunk/test/Modules/Inputs/pragma_pack_reset_push.h (original)
+++ cfe/trunk/test/Modules/Inputs/pragma_pack_reset_push.h (removed)
@@ -1,4 +0,0 @@
-
-#pragma pack ()
-#pragma pack (push, 4)
-
Removed: cfe/trunk/test/Modules/pragma-pack.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/pragma-pack.c?rev=300379&view=auto
==============================================================================
--- cfe/trunk/test/Modules/pragma-pack.c (original)
+++ cfe/trunk/test/Modules/pragma-pack.c (removed)
@@ -1,35 +0,0 @@
-// RUN: rm -rf %t
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fmodules -fimplicit-module-maps -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=pragma_pack_set %S/Inputs/module.map
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fmodules -fimplicit-module-maps -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=pragma_pack_push %S/Inputs/module.map
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fmodules -fimplicit-module-maps -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=pragma_pack_empty %S/Inputs/module.map
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fmodules -fimplicit-module-maps -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=pragma_pack_reset_push %S/Inputs/module.map
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fmodules -fimplicit-module-maps -x objective-c -verify -fmodules-cache-path=%t -I %S/Inputs %s
-// FIXME: When we have a syntax for modules in C, use that.
-
- at import pragma_pack_set;
-
-#pragma pack (show) // expected-warning {{value of #pragma pack(show) == 1}}
-#pragma pack (pop) // expected-warning {{#pragma pack(pop, ...) failed: stack empty}}
-
- at import pragma_pack_push;
-
-#pragma pack (show) // expected-warning {{value of #pragma pack(show) == 2}}
-#pragma pack (pop)
-#pragma pack (show) // expected-warning {{value of #pragma pack(show) == 4}}
-#pragma pack (pop)
-#pragma pack (show) // expected-warning {{value of #pragma pack(show) == 1}}
-#pragma pack (pop) // expected-warning {{#pragma pack(pop, ...) failed: stack empty}}
-
-#pragma pack (16)
-
- at import pragma_pack_empty;
-
-#pragma pack (show) // expected-warning {{value of #pragma pack(show) == 16}}
-#pragma pack (pop) // expected-warning {{#pragma pack(pop, ...) failed: stack empty}}
-
- at import pragma_pack_reset_push;
-
-#pragma pack (show) // expected-warning {{value of #pragma pack(show) == 4}}
-#pragma pack (pop)
-#pragma pack (show) // expected-warning {{value of #pragma pack(show) == 8}}
-
Added: cfe/trunk/test/Modules/pragma-pack.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/pragma-pack.cpp?rev=300380&view=auto
==============================================================================
--- cfe/trunk/test/Modules/pragma-pack.cpp (added)
+++ cfe/trunk/test/Modules/pragma-pack.cpp Fri Apr 14 19:07:57 2017
@@ -0,0 +1,25 @@
+// RUN: rm -rf %t.cache %tlocal.cache
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fmodules \
+// RUN: -fimplicit-module-maps -x c++ -emit-module \
+// RUN: -fmodules-cache-path=%t.cache \
+// RUN: -fmodule-name=pragma_pack %S/Inputs/module.map
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fmodules \
+// RUN: -fimplicit-module-maps -x c++ -verify \
+// RUN: -fmodules-cache-path=%t.cache \
+// RUN: -I%S/Inputs %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fmodules \
+// RUN: -fmodules-local-submodule-visibility \
+// RUN: -fimplicit-module-maps -x c++ -emit-module \
+// RUN: -fmodules-cache-path=%tlocal.cache \
+// RUN: -fmodule-name=pragma_pack %S/Inputs/module.map
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fmodules \
+// RUN: -fmodules-local-submodule-visibility \
+// RUN: -fimplicit-module-maps -x c++ -verify \
+// RUN: -fmodules-cache-path=%tlocal.cache \
+// RUN: -I%S/Inputs %s
+
+// Check that we don't serialize pragma pack state until/unless including an
+// empty file from the same module (but different submodule) has no effect.
+#pragma pack (show) // expected-warning {{value of #pragma pack(show) == 8}}
+#include "empty.h"
+#pragma pack (show) // expected-warning {{value of #pragma pack(show) == 8}}
More information about the cfe-commits
mailing list