This is an archive of the discontinued LLVM Phabricator instance.

Add a warning for missing '#pragma pack (pop)' and suspicious '#pragma pack' uses when including files
ClosedPublic

Authored by arphaman on Jul 17 2017, 7:38 AM.

Details

Summary

This patch adds a new -Wpragma-pack warning. It warns in the following cases:

  • When a translation unit is missing terminating #pragma pack (pop) directives.
  • When entering an included file if the current alignment value as determined by '#pragma pack' directives is different from the default alignment value.
  • When leaving an included file that changed the state of the current alignment value.

The change in the parser is required to avoid a missing diagnostic in the following scenario:

#pragma pack (push, 1)
#include “foo.h” // Without the change, the diagnostic for the 2nd case won’t be emitted since include will get processed by the Sema before the pragma

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Jul 17 2017, 7:38 AM
aaron.ballman added inline comments.Jul 18 2017, 5:47 AM
include/clang/Basic/DiagnosticSemaKinds.td
716 ↗(On Diff #106870)

We don't use "record" in the text for diagnostics. Perhaps replace record with "struct or union members"?

718 ↗(On Diff #106870)

This can move up a line.

include/clang/Sema/Sema.h
1162 ↗(On Diff #106870)

Please group this with the other private Sema members and give it a \brief comment.

lib/Sema/Sema.cpp
95 ↗(On Diff #106870)

This comment doesn't add much value.

lib/Sema/SemaAttr.cpp
235–236 ↗(On Diff #106870)

Can you use for (const auto &StackSlot : llvm::reverse(PackStack.Stack)) instead?

287–288 ↗(On Diff #106870)

This would be better written using emplace_back() rather than constructing and using push_back().

arphaman updated this revision to Diff 107090.Jul 18 2017, 6:43 AM
arphaman marked 5 inline comments as done.

Address review comments.

This revision is now accepted and ready to land.Jul 18 2017, 7:02 AM
This revision was automatically updated to reflect the committed changes.

This triggered a warning in LLVM itself, in CoverageMapping.h :

error: non-default #pragma pack value might change the alignment of
struct or union members in the included file [-Werror,-Wpragma-pack]
#include "llvm/ProfileData/InstrProfData.inc"
         ^
/mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:513:1:
note: previous '#pragma pack' directive that modifies alignment is
here
LLVM_PACKED_START
^
/mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/include/llvm/Support/Compiler.h:349:28:
note: expanded from macro 'LLVM_PACKED_START'
# define LLVM_PACKED_START _Pragma("pack(push, 1)")
                           ^
<scratch space>:14:2: note: expanded from here
 pack(push, 1)

I think I might tweak this diagnostic to avoid warning in that case. It might be better to just avoid warning about "non-default #pragma pack value might change the alignment of
struct or union members in the included file" until a first record declaration is encountered in the included file. This will avoid the warning in LLVM as the packed class is declared in the file that includes the header.