<div dir="ltr">Ok, thanks!</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 23, 2016 at 4:11 AM, Roger Ferrer Ibanez <span dir="ltr"><<a href="mailto:Roger.FerrerIbanez@arm.com" target="_blank">Roger.FerrerIbanez@arm.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-GB" link="blue" vlink="purple">
<div class="m_7209254671121522884WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Yes it was approved on IRC by Aaron Ballman:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">T 1470758009 <rogfer01> zygoloid: do you have any further thoughts about <a href="https://reviews.llvm.org/D20561" target="_blank">https://reviews.llvm.org/<wbr>D20561</a> ? thank you very much :)<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">T 1470758207 <AaronBallman> rogfer01: if zygoloid doesn't review in the next two days, you can go ahead and commit. I've accepted it, and Richard's comments
suggest you're following the correct approach with your latest patch. If there are problems, we can always revert again if needed<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Kind regards,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Roger<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> <a href="mailto:thakis@google.com" target="_blank">thakis@google.com</a> [mailto:<a href="mailto:thakis@google.com" target="_blank">thakis@google.com</a>]
<b>On Behalf Of </b>Nico Weber<br>
<b>Sent:</b> 22 August 2016 17:59<br>
<b>To:</b> Roger Ferrer Ibanez<br>
<b>Cc:</b> cfe-commits<br>
<b>Subject:</b> Re: r278483 - This patch implements PR#22821.<u></u><u></u></span></p>
</div>
</div><div><div class="h5">
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">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" target="_blank">https://reviews.llvm.org/<wbr>D20561</a> -- was this reviewed on IRC?<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Fri, Aug 12, 2016 at 4:04 AM, Roger Ferrer Ibanez via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<u></u><u></u></p>
<p class="MsoNormal">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" 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" 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" 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" 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" 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" 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)<wbr>;<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" 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" 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(<wbr>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" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><u></u><u></u></p>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div></div></div>
</div>
</div>
</blockquote></div><br></div>