rogfer01 removed rL LLVM as the repository for this revision.
rogfer01 updated this revision to Diff 60664.
rogfer01 added a comment.
This new patch adds a whitelist for memcpy, memmove, memset and memcmp.
I'm not entirely happy with the strategy that I'm using, where Parser
communicates to Sema which is the LHS of a direct function call being parsed.
Better approaches are of course welcome.
http://reviews.llvm.org/D20561
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Parse/ParseExpr.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaExpr.cpp
test/Sema/address-packed-member-whitelist.c
test/Sema/address-packed.c
test/SemaCXX/address-packed-member-whitelist.cpp
test/SemaCXX/address-packed.cpp
Index: test/SemaCXX/address-packed.cpp
===================================================================
--- /dev/null
+++ test/SemaCXX/address-packed.cpp
@@ -0,0 +1,100 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct __attribute__((packed)) Arguable {
+ int x;
+ char c;
+ static void foo();
+};
+
+extern void f3(void());
+
+namespace Foo {
+struct __attribute__((packed)) Arguable {
+ char c;
+ int x;
+ static void foo();
+};
+}
+
+struct Arguable *get_arguable();
+
+void g0() {
+ {
+ Foo::Arguable arguable;
+ f1(&arguable.x); // expected-warning {{packed member 'x' of class or structure 'Foo::Arguable'}}
+ f2(&arguable.c); // no-warning
+ f3(&arguable.foo); // no-warning
+ }
+ {
+ Arguable arguable1;
+ Arguable &arguable(arguable1);
+ f1(&arguable.x); // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+ f2(&arguable.c); // no-warning
+ f3(&arguable.foo); // no-warning
+ }
+ {
+ Arguable *arguable1;
+ Arguable *&arguable(arguable1);
+ f1(&arguable->x); // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+ f2(&arguable->c); // no-warning
+ f3(&arguable->foo); // no-warning
+ }
+}
+
+struct __attribute__((packed)) A {
+ int x;
+ char c;
+
+ int *f0() {
+ return &this->x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+ }
+
+ int *g0() {
+ return &x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+ }
+
+ char *h0() {
+ return &c; // no-warning
+ }
+};
+
+struct B : A {
+ int *f1() {
+ return &this->x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+ }
+
+ int *g1() {
+ return &x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+ }
+
+ char *h1() {
+ return &c; // no-warning
+ }
+};
+
+template <typename Ty>
+class __attribute__((packed)) S {
+ Ty X;
+
+public:
+ const Ty *get() const {
+ return &X; // expected-warning {{packed member 'X' of class or structure 'S<int>'}}
+ // expected-warning@-1 {{packed member 'X' of class or structure 'S<float>'}}
+ }
+};
+
+template <typename Ty>
+void h(Ty *);
+
+void g1() {
+ S<int> s1;
+ s1.get(); // expected-note {{in instantiation of member function 'S<int>::get'}}
+
+ S<char> s2;
+ s2.get();
+
+ S<float> s3;
+ s3.get(); // expected-note {{in instantiation of member function 'S<float>::get'}}
+}
Index: test/SemaCXX/address-packed-member-whitelist.cpp
===================================================================
--- /dev/null
+++ test/SemaCXX/address-packed-member-whitelist.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct B {
+ int x, y, z, w;
+} b;
+
+struct __attribute__((packed)) A {
+ struct B b;
+} a;
+
+typedef __typeof__(sizeof(int)) size_t;
+
+extern "C" {
+void *memcpy(void *dest, const void *src, size_t n);
+int memcmp(const void *s1, const void *s2, size_t n);
+void *memmove(void *dest, const void *src, size_t n);
+void *memset(void *s, int c, size_t n);
+}
+
+void *my_memcpy(void *dest, const void *src, size_t n);
+int my_memcmp(const void *s1, const void *s2, size_t n);
+void *my_memmove(void *dest, const void *src, size_t n);
+void *my_memset(void *s, int c, size_t n);
+
+int x;
+
+void foo() {
+ memcpy(&a.b, &b, sizeof(b)); // no-warning
+ memmove(&a.b, &b, sizeof(b)); // no-warning
+ memset(&a.b, 0, sizeof(b)); // no-warning
+ x = memcmp(&a.b, &b, sizeof(b)); // no-warning
+
+ (memcpy)(&a.b, &b, sizeof(b)); // no-warning
+ (memmove)(&a.b, &b, sizeof(b)); // no-warning
+ (memset)(&a.b, 0, sizeof(b)); // no-warning
+ x = (memcmp)(&a.b, &b, sizeof(b)); // no-warning
+
+ ::memcpy(&a.b, &b, sizeof(b)); // no-warning
+ ::memmove(&a.b, &b, sizeof(b)); // no-warning
+ ::memset(&a.b, 0, sizeof(b)); // no-warning
+ x = ::memcmp(&a.b, &b, sizeof(b)); // no-warning
+
+ (::memcpy)(&a.b, &b, sizeof(b)); // no-warning
+ (::memmove)(&a.b, &b, sizeof(b)); // no-warning
+ (::memset)(&a.b, 0, sizeof(b)); // no-warning
+ x = (::memcmp)(&a.b, &b, sizeof(b)); // no-warning
+
+ my_memcpy(&a.b, &b, sizeof(b)); // expected-warning {{packed member 'b' of class or structure 'A'}}
+ my_memmove(&a.b, &b, sizeof(b)); // expected-warning {{packed member 'b' of class or structure 'A'}}
+ my_memset(&a.b, 0, sizeof(b)); // expected-warning {{packed member 'b' of class or structure 'A'}}
+ x = my_memcmp(&a.b, &b, sizeof(b)); // expected-warning {{packed member 'b' of class or structure 'A'}}
+}
Index: test/Sema/address-packed.c
===================================================================
--- /dev/null
+++ test/Sema/address-packed.c
@@ -0,0 +1,126 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct Ok {
+ char c;
+ int x;
+};
+
+struct __attribute__((packed)) Arguable {
+ char c0;
+ int x;
+ char c1;
+};
+
+union __attribute__((packed)) UnionArguable {
+ char c;
+ int x;
+};
+
+typedef struct Arguable ArguableT;
+
+struct Arguable *get_arguable();
+
+void g0(void) {
+ {
+ struct Ok ok;
+ f1(&ok.x); // no-warning
+ f2(&ok.c); // no-warning
+ }
+ {
+ struct Arguable arguable;
+ f2(&arguable.c0); // no-warning
+ f1(&arguable.x); // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+ f2(&arguable.c1); // no-warning
+ }
+ {
+ union UnionArguable arguable;
+ f2(&arguable.c); // no-warning
+ f1(&arguable.x); // expected-warning {{packed member 'x' of class or structure 'UnionArguable'}}
+ }
+ {
+ ArguableT arguable;
+ f2(&arguable.c0); // no-warning
+ f1(&arguable.x); // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+ f2(&arguable.c1); // no-warning
+ }
+ {
+ struct Arguable *arguable = get_arguable();
+ // These do not produce any warning because of the parentheses.
+ f2(&(arguable->c0)); // no-warning
+ f1(&(arguable->x)); // no-warning
+ f2(&(arguable->c1)); // no-warning
+ }
+ {
+ ArguableT *arguable = get_arguable();
+ // These do not produce any warning because of the parentheses.
+ f2(&(arguable->c0)); // no-warning
+ f1(&(arguable->x)); // no-warning
+ f2(&(arguable->c1)); // no-warning
+ }
+}
+
+struct S1 {
+ char c;
+ int i __attribute__((packed));
+};
+
+int *g1(struct S1 *s1) {
+ return &s1->i; // expected-warning {{packed member 'i' of class or structure 'S1'}}
+}
+
+struct S2_i {
+ int i;
+};
+struct __attribute__((packed)) S2 {
+ char c;
+ struct S2_i inner;
+};
+
+int *g2(struct S2 *s2) {
+ return &s2->inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2'}}
+}
+
+struct S2_a {
+ char c;
+ struct S2_i inner __attribute__((packed));
+};
+
+int *g2_a(struct S2_a *s2_a) {
+ return &s2_a->inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2_a'}}
+}
+
+struct __attribute__((packed)) S3 {
+ char c;
+ struct {
+ int i;
+ } inner;
+};
+
+int *g3(struct S3 *s3) {
+ return &s3->inner.i; // expected-warning {{packed member 'inner' of class or structure 'S3'}}
+}
+
+struct S4 {
+ char c;
+ struct __attribute__((packed)) {
+ int i;
+ } inner;
+};
+
+int *g4(struct S4 *s4) {
+ return &s4->inner.i; // expected-warning {{packed member 'i' of class or structure 'S4::(anonymous)'}}
+}
+
+struct S5 {
+ char c;
+ struct {
+ char c1;
+ int i __attribute__((packed));
+ } inner;
+};
+
+int *g5(struct S5 *s5) {
+ return &s5->inner.i; // expected-warning {{packed member 'i' of class or structure 'S5::(anonymous)'}}
+}
Index: test/Sema/address-packed-member-whitelist.c
===================================================================
--- /dev/null
+++ test/Sema/address-packed-member-whitelist.c
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct B {
+ int x, y, z, w;
+} b;
+
+struct __attribute__((packed)) A {
+ struct B b;
+} a;
+
+typedef __typeof__(sizeof(int)) size_t;
+
+void *memcpy(void *dest, const void *src, size_t n);
+int memcmp(const void *s1, const void *s2, size_t n);
+void *memmove(void *dest, const void *src, size_t n);
+void *memset(void *s, int c, size_t n);
+
+void *my_memcpy(void *dest, const void *src, size_t n);
+int my_memcmp(const void *s1, const void *s2, size_t n);
+void *my_memmove(void *dest, const void *src, size_t n);
+void *my_memset(void *s, int c, size_t n);
+
+int x;
+
+void foo(void) {
+ memcpy(&a.b, &b, sizeof(b)); // no-warning
+ memmove(&a.b, &b, sizeof(b)); // no-warning
+ memset(&a.b, 0, sizeof(b)); // no-warning
+ x = memcmp(&a.b, &b, sizeof(b)); // no-warning
+
+ (memcpy)(&a.b, &b, sizeof(b)); // no-warning
+ (memmove)(&a.b, &b, sizeof(b)); // no-warning
+ (memset)(&a.b, 0, sizeof(b)); // no-warning
+ x = (memcmp)(&a.b, &b, sizeof(b)); // no-warning
+
+ my_memcpy(&a.b, &b, sizeof(b)); // expected-warning {{packed member 'b' of class or structure 'A'}}
+ my_memmove(&a.b, &b, sizeof(b)); // expected-warning {{packed member 'b' of class or structure 'A'}}
+ my_memset(&a.b, 0, sizeof(b)); // expected-warning {{packed member 'b' of class or structure 'A'}}
+ x = my_memcmp(&a.b, &b, sizeof(b)); // expected-warning {{packed member 'b' of class or structure 'A'}}
+}
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -10514,6 +10514,8 @@
return QualType();
}
+ CheckAddressOfPackedMember(OrigOp.get(), OpLoc);
+
return Context.getPointerType(op->getType());
}
@@ -15023,3 +15025,14 @@
return new (Context)
ObjCBoolLiteralExpr(Kind == tok::kw___objc_yes, BoolT, OpLoc);
}
+
+void Sema::PushFunctionCallLHS(Expr *e) { FunctionCallLHSStack.push_back(e); }
+
+void Sema::PopFunctionCallLHS() {
+ assert(!FunctionCallLHSStack.empty() && "Empty stack");
+ FunctionCallLHSStack.pop_back();
+}
+
+Expr *Sema::CurrentFunctionCallLHS() const {
+ return !FunctionCallLHSStack.empty() ? FunctionCallLHSStack.back() : nullptr;
+}
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -10704,3 +10704,63 @@
<< ArgumentExpr->getSourceRange()
<< TypeTagExpr->getSourceRange();
}
+
+void Sema::CheckAddressOfPackedMember(Expr *rhs, clang::SourceLocation &OpLoc) {
+ Expr *FunctionCallLHS = CurrentFunctionCallLHS();
+ if (FunctionCallLHS)
+ FunctionCallLHS = FunctionCallLHS->IgnoreParens();
+
+ auto WhiteListedFunction = [](FunctionDecl *FDecl) -> bool {
+ if (FDecl) {
+ unsigned CMId = FDecl->getMemoryFunctionKind();
+ switch (CMId) {
+ case Builtin::BImemcmp:
+ case Builtin::BImemcpy:
+ case Builtin::BImemmove:
+ case Builtin::BImemset:
+ // We skip the diagnostic if we are calling these functions since they
+ // are a source of false positives.
+ return true;
+ default:
+ break;
+ }
+ }
+ return false;
+ };
+
+ if (auto *ULE = dyn_cast_or_null<UnresolvedLookupExpr>(FunctionCallLHS)) {
+ for (NamedDecl *D : ULE->decls()) {
+ if (auto *FDecl = dyn_cast<FunctionDecl>(D))
+ if (WhiteListedFunction(FDecl))
+ return;
+ }
+ } else if (auto *DRE = dyn_cast_or_null<DeclRefExpr>(FunctionCallLHS)) {
+ FunctionDecl *FDecl = nullptr;
+ if (DRE)
+ FDecl = dyn_cast_or_null<FunctionDecl>(DRE->getDecl());
+ if (FDecl && WhiteListedFunction(FDecl))
+ return;
+ }
+
+ const auto *ME = dyn_cast<MemberExpr>(rhs);
+ while (ME && isa<FieldDecl>(ME->getMemberDecl())) {
+ QualType BaseType = ME->getBase()->getType();
+ if (ME->isArrow())
+ BaseType = BaseType->getPointeeType();
+ RecordDecl *RD = BaseType->getAs<RecordType>()->getDecl();
+
+ ValueDecl *MD = ME->getMemberDecl();
+ bool ByteAligned = Context.getTypeAlignInChars(MD->getType()).isOne();
+ if (ByteAligned) // Attribute packed does not have any effect.
+ break;
+
+ if (!ByteAligned &&
+ (RD->hasAttr<PackedAttr>() || (MD->hasAttr<PackedAttr>()))) {
+ Diag(OpLoc, diag::warn_taking_address_of_packed_member)
+ << MD << RD << rhs->getSourceRange();
+ break;
+ }
+ ME = dyn_cast<MemberExpr>(ME->getBase());
+ }
+}
+
Index: lib/Parse/ParseExpr.cpp
===================================================================
--- lib/Parse/ParseExpr.cpp
+++ lib/Parse/ParseExpr.cpp
@@ -1561,15 +1561,17 @@
if (OpKind == tok::l_paren || !LHS.isInvalid()) {
if (Tok.isNot(tok::r_paren)) {
+ Actions.PushFunctionCallLHS(LHS.get());
if (ParseExpressionList(ArgExprs, CommaLocs, [&] {
Actions.CodeCompleteCall(getCurScope(), LHS.get(), ArgExprs);
- })) {
+ })) {
(void)Actions.CorrectDelayedTyposInExpr(LHS);
LHS = ExprError();
} else if (LHS.isInvalid()) {
for (auto &E : ArgExprs)
Actions.CorrectDelayedTyposInExpr(E);
}
+ Actions.PopFunctionCallLHS();
}
}
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -465,6 +465,11 @@
/// that's used to parse every top-level function.
SmallVector<sema::FunctionScopeInfo *, 4> FunctionScopes;
+ /// \brief Stack containing the current LHS of a direct function call being
+ /// parsed. It is empty if we are not parsing an expression of the argument
+ /// list of a function call.
+ SmallVector<Expr *, 4> FunctionCallLHSStack;
+
typedef LazyVector<TypedefNameDecl *, ExternalSemaSource,
&ExternalSemaSource::ReadExtVectorDecls, 2, 2>
ExtVectorDeclsType;
@@ -1204,6 +1209,10 @@
void PushCompoundScope();
void PopCompoundScope();
+ void PushFunctionCallLHS(Expr*);
+ void PopFunctionCallLHS();
+ Expr* CurrentFunctionCallLHS() const;
+
sema::CompoundScopeInfo &getCurCompoundScope() const;
bool hasAnyUnrecoverableErrorsInThisFunction() const;
@@ -9417,6 +9426,10 @@
void CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
const Expr * const *ExprArgs);
+ /// \brief Diagnose if we are taking the address of a packed field
+ /// as this may be a problem if the pointer value is dereferenced.
+ void CheckAddressOfPackedMember(Expr *rhs, clang::SourceLocation &OpLoc);
+
/// \brief The parser's current scope.
///
/// The parser maintains this state here.
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5387,6 +5387,9 @@
"dereference of type %1 that was reinterpret_cast from type %0 has undefined "
"behavior">,
InGroup<UndefinedReinterpretCast>, DefaultIgnore;
+def warn_taking_address_of_packed_member : Warning<
+ "taking address of packed member %0 of class or structure %q1 may result in an unaligned pointer value">,
+ InGroup<DiagGroup<"address-of-packed-member">>;
def err_objc_object_assignment : Error<
"cannot assign to class object (%0 invalid)">;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
