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. - -@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}} - -@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) - -@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}} - -@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}} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits