<div dir="ltr">I don't see any approval after Aaron's "Please wait until someone has had the chance to review before committing" on <a href="https://reviews.llvm.org/D20561">https://reviews.llvm.org/D20561</a> -- was this reviewed on IRC?</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 12, 2016 at 4:04 AM, Roger Ferrer Ibanez via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: rogfer01<br>
Date: Fri Aug 12 03:04:13 2016<br>
New Revision: 278483<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=278483&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=278483&view=rev</a><br>
Log:<br>
This patch implements PR#22821.<br>
<br>
Taking the address of a packed member is dangerous since the reduced<br>
alignment of the pointee is lost. This can lead to memory alignment<br>
faults in some architectures if the pointer value is dereferenced.<br>
<br>
This change adds a new warning to clang emitted when taking the address<br>
of a packed member. A packed member is either a field/data member<br>
declared as attribute((packed)) or belonging to a struct/class<br>
declared as such. The associated flag is -Waddress-of-packed-member.<br>
Conversions (either implicit or via a valid casting) to pointer types<br>
with lower or equal alignment requirements (e.g. void* or char*)<br>
will silence the warning.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D20561" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D20561</a><br>
<br>
<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td<br>
cfe/trunk/include/clang/Sema/<wbr>Sema.h<br>
cfe/trunk/lib/Sema/SemaCast.<wbr>cpp<br>
cfe/trunk/lib/Sema/<wbr>SemaChecking.cpp<br>
cfe/trunk/lib/Sema/SemaExpr.<wbr>cpp<br>
cfe/trunk/lib/Sema/SemaInit.<wbr>cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=278483&r1=278482&r2=278483&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/include/<wbr>clang/Basic/<wbr>DiagnosticSemaKinds.td?rev=<wbr>278483&r1=278482&r2=278483&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td Fri Aug 12 03:04:13 2016<br>
@@ -5489,6 +5489,9 @@ def warn_pointer_indirection_from_<wbr>incomp<br>
"dereference of type %1 that was reinterpret_cast from type %0 has undefined "<br>
"behavior">,<br>
InGroup<<wbr>UndefinedReinterpretCast>, DefaultIgnore;<br>
+def warn_taking_address_of_packed_<wbr>member : Warning<<br>
+ "taking address of packed member %0 of class or structure %q1 may result in an unaligned pointer value">,<br>
+ InGroup<DiagGroup<"address-of-<wbr>packed-member">>;<br>
<br>
def err_objc_object_assignment : Error<<br>
"cannot assign to class object (%0 invalid)">;<br>
<br>
Modified: cfe/trunk/include/clang/Sema/<wbr>Sema.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=278483&r1=278482&r2=278483&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/include/<wbr>clang/Sema/Sema.h?rev=278483&<wbr>r1=278482&r2=278483&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/include/clang/Sema/<wbr>Sema.h (original)<br>
+++ cfe/trunk/include/clang/Sema/<wbr>Sema.h Fri Aug 12 03:04:13 2016<br>
@@ -9570,6 +9570,10 @@ private:<br>
void CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,<br>
const Expr * const *ExprArgs);<br>
<br>
+ /// \brief Check if we are taking the address of a packed field<br>
+ /// as this may be a problem if the pointer value is dereferenced.<br>
+ void CheckAddressOfPackedMember(<wbr>Expr *rhs);<br>
+<br>
/// \brief The parser's current scope.<br>
///<br>
/// The parser maintains this state here.<br>
@@ -9664,6 +9668,51 @@ public:<br>
// Emitting members of dllexported classes is delayed until the class<br>
// (including field initializers) is fully parsed.<br>
SmallVector<CXXRecordDecl*, 4> DelayedDllExportClasses;<br>
+<br>
+private:<br>
+ /// \brief Helper class that collects misaligned member designations and<br>
+ /// their location info for delayed diagnostics.<br>
+ struct MisalignedMember {<br>
+ Expr *E;<br>
+ RecordDecl *RD;<br>
+ ValueDecl *MD;<br>
+ CharUnits Alignment;<br>
+<br>
+ MisalignedMember() : E(), RD(), MD(), Alignment() {}<br>
+ MisalignedMember(Expr *E, RecordDecl *RD, ValueDecl *MD,<br>
+ CharUnits Alignment)<br>
+ : E(E), RD(RD), MD(MD), Alignment(Alignment) {}<br>
+ explicit MisalignedMember(Expr *E)<br>
+ : MisalignedMember(E, nullptr, nullptr, CharUnits()) {}<br>
+<br>
+ bool operator==(const MisalignedMember &m) { return this->E == m.E; }<br>
+ };<br>
+ /// \brief Small set of gathered accesses to potentially misaligned members<br>
+ /// due to the packed attribute.<br>
+ SmallVector<MisalignedMember, 4> MisalignedMembers;<br>
+<br>
+ /// \brief Adds an expression to the set of gathered misaligned members.<br>
+ void AddPotentialMisalignedMembers(<wbr>Expr *E, RecordDecl *RD, ValueDecl *MD,<br>
+ CharUnits Alignment);<br>
+<br>
+public:<br>
+ /// \brief Diagnoses the current set of gathered accesses. This typically<br>
+ /// happens at full expression level. The set is cleared after emitting the<br>
+ /// diagnostics.<br>
+ void DiagnoseMisalignedMembers();<br>
+<br>
+ /// \brief This function checks if the expression is in the sef of potentially<br>
+ /// misaligned members and it is converted to some pointer type T with lower<br>
+ /// or equal alignment requirements. If so it removes it. This is used when<br>
+ /// we do not want to diagnose such misaligned access (e.g. in conversions to void*).<br>
+ void DiscardMisalignedMemberAddress<wbr>(const Type *T, Expr *E);<br>
+<br>
+ /// \brief This function calls Action when it determines that E designates a<br>
+ /// misaligned member due to the packed attribute. This is used to emit<br>
+ /// local diagnostics like in reference binding.<br>
+ void RefersToMemberWithReducedAlign<wbr>ment(<br>
+ Expr *E,<br>
+ std::function<void(Expr *, RecordDecl *, ValueDecl *, CharUnits)> Action);<br>
};<br>
<br>
/// \brief RAII object that enters a new expression evaluation context.<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaCast.<wbr>cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCast.cpp?rev=278483&r1=278482&r2=278483&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Sema/<wbr>SemaCast.cpp?rev=278483&r1=<wbr>278482&r2=278483&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Sema/SemaCast.<wbr>cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaCast.<wbr>cpp Fri Aug 12 03:04:13 2016<br>
@@ -256,6 +256,7 @@ Sema::BuildCXXNamedCast(<wbr>SourceLocation O<br>
Op.CheckConstCast();<br>
if (Op.SrcExpr.isInvalid())<br>
return ExprError();<br>
+ DiscardMisalignedMemberAddress<wbr>(DestType.getTypePtr(), E);<br>
}<br>
return Op.complete(CXXConstCastExpr::<wbr>Create(Context, Op.ResultType,<br>
Op.ValueKind, Op.SrcExpr.get(), DestTInfo,<br>
@@ -279,6 +280,7 @@ Sema::BuildCXXNamedCast(<wbr>SourceLocation O<br>
Op.CheckReinterpretCast();<br>
if (Op.SrcExpr.isInvalid())<br>
return ExprError();<br>
+ DiscardMisalignedMemberAddress<wbr>(DestType.getTypePtr(), E);<br>
}<br>
return Op.complete(<wbr>CXXReinterpretCastExpr::<wbr>Create(Context, Op.ResultType,<br>
Op.ValueKind, Op.Kind, Op.SrcExpr.get(),<br>
@@ -291,6 +293,7 @@ Sema::BuildCXXNamedCast(<wbr>SourceLocation O<br>
Op.CheckStaticCast();<br>
if (Op.SrcExpr.isInvalid())<br>
return ExprError();<br>
+ DiscardMisalignedMemberAddress<wbr>(DestType.getTypePtr(), E);<br>
}<br>
<br>
return Op.complete(CXXStaticCastExpr:<wbr>:Create(Context, Op.ResultType,<br>
<br>
Modified: cfe/trunk/lib/Sema/<wbr>SemaChecking.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=278483&r1=278482&r2=278483&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Sema/<wbr>SemaChecking.cpp?rev=278483&<wbr>r1=278482&r2=278483&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Sema/<wbr>SemaChecking.cpp (original)<br>
+++ cfe/trunk/lib/Sema/<wbr>SemaChecking.cpp Fri Aug 12 03:04:13 2016<br>
@@ -8383,6 +8383,8 @@ void CheckImplicitConversion(Sema &S, Ex<br>
<br>
DiagnoseNullConversion(S, E, T, CC);<br>
<br>
+ S.<wbr>DiscardMisalignedMemberAddress<wbr>(Target, E);<br>
+<br>
if (!Source->isIntegerType() || !Target->isIntegerType())<br>
return;<br>
<br>
@@ -9453,6 +9455,7 @@ void Sema::CheckCompletedExpr(Expr *E, S<br>
CheckUnsequencedOperations(E);<br>
if (!IsConstexpr && !E->isValueDependent())<br>
CheckForIntOverflow(E);<br>
+ DiagnoseMisalignedMembers();<br>
}<br>
<br>
void Sema::<wbr>CheckBitFieldInitialization(<wbr>SourceLocation InitLoc,<br>
@@ -10998,3 +11001,67 @@ void Sema::<wbr>CheckArgumentWithTypeTag(cons<br>
<< ArgumentExpr->getSourceRange()<br>
<< TypeTagExpr->getSourceRange();<br>
}<br>
+<br>
+void Sema::<wbr>AddPotentialMisalignedMembers(<wbr>Expr *E, RecordDecl *RD, ValueDecl *MD,<br>
+ CharUnits Alignment) {<br>
+ MisalignedMembers.emplace_<wbr>back(E, RD, MD, Alignment);<br>
+}<br>
+<br>
+void Sema::<wbr>DiagnoseMisalignedMembers() {<br>
+ for (MisalignedMember &m : MisalignedMembers) {<br>
+ Diag(m.E->getLocStart(), diag::warn_taking_address_of_<wbr>packed_member)<br>
+ << m.MD << m.RD << m.E->getSourceRange();<br>
+ }<br>
+ MisalignedMembers.clear();<br>
+}<br>
+<br>
+void Sema::<wbr>DiscardMisalignedMemberAddress<wbr>(const Type *T, Expr *E) {<br>
+ if (!T->isPointerType())<br>
+ return;<br>
+ if (isa<UnaryOperator>(E) &&<br>
+ cast<UnaryOperator>(E)-><wbr>getOpcode() == UO_AddrOf) {<br>
+ auto *Op = cast<UnaryOperator>(E)-><wbr>getSubExpr()->IgnoreParens();<br>
+ if (isa<MemberExpr>(Op)) {<br>
+ auto MA = std::find(MisalignedMembers.<wbr>begin(), MisalignedMembers.end(),<br>
+ MisalignedMember(Op));<br>
+ if (MA != MisalignedMembers.end() &&<br>
+ Context.getTypeAlignInChars(T-<wbr>>getPointeeType()) <= MA->Alignment)<br>
+ MisalignedMembers.erase(MA);<br>
+ }<br>
+ }<br>
+}<br>
+<br>
+void Sema::<wbr>RefersToMemberWithReducedAlign<wbr>ment(<br>
+ Expr *E,<br>
+ std::function<void(Expr *, RecordDecl *, ValueDecl *, CharUnits)> Action) {<br>
+ const auto *ME = dyn_cast<MemberExpr>(E);<br>
+ while (ME && isa<FieldDecl>(ME-><wbr>getMemberDecl())) {<br>
+ QualType BaseType = ME->getBase()->getType();<br>
+ if (ME->isArrow())<br>
+ BaseType = BaseType->getPointeeType();<br>
+ RecordDecl *RD = BaseType->getAs<RecordType>()-<wbr>>getDecl();<br>
+<br>
+ ValueDecl *MD = ME->getMemberDecl();<br>
+ bool ByteAligned = Context.getTypeAlignInChars(<wbr>MD->getType()).isOne();<br>
+ if (ByteAligned) // Attribute packed does not have any effect.<br>
+ break;<br>
+<br>
+ if (!ByteAligned &&<br>
+ (RD->hasAttr<PackedAttr>() || (MD->hasAttr<PackedAttr>()))) {<br>
+ CharUnits Alignment = std::min(Context.<wbr>getTypeAlignInChars(MD-><wbr>getType()),<br>
+ Context.getTypeAlignInChars(<wbr>BaseType));<br>
+ // Notify that this expression designates a member with reduced alignment<br>
+ Action(E, RD, MD, Alignment);<br>
+ break;<br>
+ }<br>
+ ME = dyn_cast<MemberExpr>(ME-><wbr>getBase());<br>
+ }<br>
+}<br>
+<br>
+void Sema::<wbr>CheckAddressOfPackedMember(<wbr>Expr *rhs) {<br>
+ using namespace std::placeholders;<br>
+ RefersToMemberWithReducedAlign<wbr>ment(<br>
+ rhs, std::bind(&Sema::<wbr>AddPotentialMisalignedMembers, std::ref(*this), _1,<br>
+ _2, _3, _4));<br>
+}<br>
+<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaExpr.<wbr>cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=278483&r1=278482&r2=278483&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Sema/<wbr>SemaExpr.cpp?rev=278483&r1=<wbr>278482&r2=278483&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Sema/SemaExpr.<wbr>cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaExpr.<wbr>cpp Fri Aug 12 03:04:13 2016<br>
@@ -6022,7 +6022,9 @@ Sema::ActOnCastExpr(Scope *S, SourceLoca<br>
CheckTollFreeBridgeCast(<wbr>castType, CastExpr);<br>
<br>
CheckObjCBridgeRelatedCast(<wbr>castType, CastExpr);<br>
-<br>
+<br>
+ DiscardMisalignedMemberAddress<wbr>(castType.getTypePtr(), CastExpr);<br>
+<br>
return BuildCStyleCastExpr(LParenLoc, castTInfo, RParenLoc, CastExpr);<br>
}<br>
<br>
@@ -10614,6 +10616,8 @@ QualType Sema::CheckAddressOfOperand(<wbr>Exp<br>
if (op->getType()-><wbr>isObjCObjectType())<br>
return Context.<wbr>getObjCObjectPointerType(op-><wbr>getType());<br>
<br>
+ CheckAddressOfPackedMember(op)<wbr>;<br>
+<br>
return Context.getPointerType(op-><wbr>getType());<br>
}<br>
<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaInit.<wbr>cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=278483&r1=278482&r2=278483&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Sema/<wbr>SemaInit.cpp?rev=278483&r1=<wbr>278482&r2=278483&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Sema/SemaInit.<wbr>cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaInit.<wbr>cpp Fri Aug 12 03:04:13 2016<br>
@@ -6660,12 +6660,16 @@ InitializationSequence::<wbr>Perform(Sema &S,<br>
getAssignmentAction(Entity), CCK);<br>
if (CurInitExprRes.isInvalid())<br>
return ExprError();<br>
+<br>
+ S.<wbr>DiscardMisalignedMemberAddress<wbr>(Step->Type.getTypePtr(), CurInit.get());<br>
+<br>
CurInit = CurInitExprRes;<br>
<br>
if (Step->Kind == SK_<wbr>ConversionSequenceNoNarrowing &&<br>
S.getLangOpts().CPlusPlus && !CurInit.get()-><wbr>isValueDependent())<br>
DiagnoseNarrowingInInitList(S, *Step->ICS, SourceType, Entity.getType(),<br>
CurInit.get());<br>
+<br>
break;<br>
}<br>
<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>