r278483 - This patch implements PR#22821.

Aaron Ballman via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 23 10:29:09 PDT 2016


On Tue, Aug 23, 2016 at 4:11 AM, Roger Ferrer Ibanez via cfe-commits
<cfe-commits at lists.llvm.org> wrote:
> Yes it was approved on IRC by Aaron Ballman:

Sorry for not mentioning this on the mailing list as well as in IRC!

~Aaron

>
>
>
> T 1470758009 <rogfer01> zygoloid: do you have any further thoughts about
> https://reviews.llvm.org/D20561 ? thank you very much :)
>
> 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
>
>
>
> Kind regards,
>
> Roger
>
>
>
> From: thakis at google.com [mailto:thakis at google.com] On Behalf Of Nico Weber
> Sent: 22 August 2016 17:59
> To: Roger Ferrer Ibanez
> Cc: cfe-commits
> Subject: Re: r278483 - This patch implements PR#22821.
>
>
>
> I don't see any approval after Aaron's "Please wait until someone has had
> the chance to review before committing" on https://reviews.llvm.org/D20561
> -- was this reviewed on IRC?
>
>
>
> On Fri, Aug 12, 2016 at 4:04 AM, Roger Ferrer Ibanez via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
>
> Author: rogfer01
> Date: Fri Aug 12 03:04:13 2016
> New Revision: 278483
>
> URL: http://llvm.org/viewvc/llvm-project?rev=278483&view=rev
> Log:
> This patch implements PR#22821.
>
> Taking the address of a packed member is dangerous since the reduced
> alignment of the pointee is lost. This can lead to memory alignment
> faults in some architectures if the pointer value is dereferenced.
>
> This change adds a new warning to clang emitted when taking the address
> of a packed member. A packed member is either a field/data member
> declared as attribute((packed)) or belonging to a struct/class
> declared as such. The associated flag is -Waddress-of-packed-member.
> Conversions (either implicit or via a valid casting) to pointer types
> with lower or equal alignment requirements (e.g. void* or char*)
> will silence the warning.
>
> Differential Revision: https://reviews.llvm.org/D20561
>
>
>
> Modified:
>     cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>     cfe/trunk/include/clang/Sema/Sema.h
>     cfe/trunk/lib/Sema/SemaCast.cpp
>     cfe/trunk/lib/Sema/SemaChecking.cpp
>     cfe/trunk/lib/Sema/SemaExpr.cpp
>     cfe/trunk/lib/Sema/SemaInit.cpp
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=278483&r1=278482&r2=278483&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Aug 12 03:04:13
> 2016
> @@ -5489,6 +5489,9 @@ def warn_pointer_indirection_from_incomp
>    "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)">;
>
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=278483&r1=278482&r2=278483&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Fri Aug 12 03:04:13 2016
> @@ -9570,6 +9570,10 @@ private:
>    void CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
>                                  const Expr * const *ExprArgs);
>
> +  /// \brief Check 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);
> +
>    /// \brief The parser's current scope.
>    ///
>    /// The parser maintains this state here.
> @@ -9664,6 +9668,51 @@ public:
>    // Emitting members of dllexported classes is delayed until the class
>    // (including field initializers) is fully parsed.
>    SmallVector<CXXRecordDecl*, 4> DelayedDllExportClasses;
> +
> +private:
> +  /// \brief Helper class that collects misaligned member designations and
> +  /// their location info for delayed diagnostics.
> +  struct MisalignedMember {
> +    Expr *E;
> +    RecordDecl *RD;
> +    ValueDecl *MD;
> +    CharUnits Alignment;
> +
> +    MisalignedMember() : E(), RD(), MD(), Alignment() {}
> +    MisalignedMember(Expr *E, RecordDecl *RD, ValueDecl *MD,
> +                     CharUnits Alignment)
> +        : E(E), RD(RD), MD(MD), Alignment(Alignment) {}
> +    explicit MisalignedMember(Expr *E)
> +        : MisalignedMember(E, nullptr, nullptr, CharUnits()) {}
> +
> +    bool operator==(const MisalignedMember &m) { return this->E == m.E; }
> +  };
> +  /// \brief Small set of gathered accesses to potentially misaligned
> members
> +  /// due to the packed attribute.
> +  SmallVector<MisalignedMember, 4> MisalignedMembers;
> +
> +  /// \brief Adds an expression to the set of gathered misaligned members.
> +  void AddPotentialMisalignedMembers(Expr *E, RecordDecl *RD, ValueDecl
> *MD,
> +                                     CharUnits Alignment);
> +
> +public:
> +  /// \brief Diagnoses the current set of gathered accesses. This typically
> +  /// happens at full expression level. The set is cleared after emitting
> the
> +  /// diagnostics.
> +  void DiagnoseMisalignedMembers();
> +
> +  /// \brief This function checks if the expression is in the sef of
> potentially
> +  /// misaligned members and it is converted to some pointer type T with
> lower
> +  /// or equal alignment requirements.  If so it removes it. This is used
> when
> +  /// we do not want to diagnose such misaligned access (e.g. in
> conversions to void*).
> +  void DiscardMisalignedMemberAddress(const Type *T, Expr *E);
> +
> +  /// \brief This function calls Action when it determines that E
> designates a
> +  /// misaligned member due to the packed attribute. This is used to emit
> +  /// local diagnostics like in reference binding.
> +  void RefersToMemberWithReducedAlignment(
> +      Expr *E,
> +      std::function<void(Expr *, RecordDecl *, ValueDecl *, CharUnits)>
> Action);
>  };
>
>  /// \brief RAII object that enters a new expression evaluation context.
>
> Modified: cfe/trunk/lib/Sema/SemaCast.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCast.cpp?rev=278483&r1=278482&r2=278483&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaCast.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaCast.cpp Fri Aug 12 03:04:13 2016
> @@ -256,6 +256,7 @@ Sema::BuildCXXNamedCast(SourceLocation O
>        Op.CheckConstCast();
>        if (Op.SrcExpr.isInvalid())
>          return ExprError();
> +      DiscardMisalignedMemberAddress(DestType.getTypePtr(), E);
>      }
>      return Op.complete(CXXConstCastExpr::Create(Context, Op.ResultType,
>                                    Op.ValueKind, Op.SrcExpr.get(),
> DestTInfo,
> @@ -279,6 +280,7 @@ Sema::BuildCXXNamedCast(SourceLocation O
>        Op.CheckReinterpretCast();
>        if (Op.SrcExpr.isInvalid())
>          return ExprError();
> +      DiscardMisalignedMemberAddress(DestType.getTypePtr(), E);
>      }
>      return Op.complete(CXXReinterpretCastExpr::Create(Context,
> Op.ResultType,
>                                      Op.ValueKind, Op.Kind,
> Op.SrcExpr.get(),
> @@ -291,6 +293,7 @@ Sema::BuildCXXNamedCast(SourceLocation O
>        Op.CheckStaticCast();
>        if (Op.SrcExpr.isInvalid())
>          return ExprError();
> +      DiscardMisalignedMemberAddress(DestType.getTypePtr(), E);
>      }
>
>      return Op.complete(CXXStaticCastExpr::Create(Context, Op.ResultType,
>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=278483&r1=278482&r2=278483&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Aug 12 03:04:13 2016
> @@ -8383,6 +8383,8 @@ void CheckImplicitConversion(Sema &S, Ex
>
>    DiagnoseNullConversion(S, E, T, CC);
>
> +  S.DiscardMisalignedMemberAddress(Target, E);
> +
>    if (!Source->isIntegerType() || !Target->isIntegerType())
>      return;
>
> @@ -9453,6 +9455,7 @@ void Sema::CheckCompletedExpr(Expr *E, S
>      CheckUnsequencedOperations(E);
>    if (!IsConstexpr && !E->isValueDependent())
>      CheckForIntOverflow(E);
> +  DiagnoseMisalignedMembers();
>  }
>
>  void Sema::CheckBitFieldInitialization(SourceLocation InitLoc,
> @@ -10998,3 +11001,67 @@ void Sema::CheckArgumentWithTypeTag(cons
>          << ArgumentExpr->getSourceRange()
>          << TypeTagExpr->getSourceRange();
>  }
> +
> +void Sema::AddPotentialMisalignedMembers(Expr *E, RecordDecl *RD, ValueDecl
> *MD,
> +                                         CharUnits Alignment) {
> +  MisalignedMembers.emplace_back(E, RD, MD, Alignment);
> +}
> +
> +void Sema::DiagnoseMisalignedMembers() {
> +  for (MisalignedMember &m : MisalignedMembers) {
> +    Diag(m.E->getLocStart(), diag::warn_taking_address_of_packed_member)
> +        << m.MD << m.RD << m.E->getSourceRange();
> +  }
> +  MisalignedMembers.clear();
> +}
> +
> +void Sema::DiscardMisalignedMemberAddress(const Type *T, Expr *E) {
> +  if (!T->isPointerType())
> +    return;
> +  if (isa<UnaryOperator>(E) &&
> +      cast<UnaryOperator>(E)->getOpcode() == UO_AddrOf) {
> +    auto *Op = cast<UnaryOperator>(E)->getSubExpr()->IgnoreParens();
> +    if (isa<MemberExpr>(Op)) {
> +      auto MA = std::find(MisalignedMembers.begin(),
> MisalignedMembers.end(),
> +                          MisalignedMember(Op));
> +      if (MA != MisalignedMembers.end() &&
> +          Context.getTypeAlignInChars(T->getPointeeType()) <=
> MA->Alignment)
> +        MisalignedMembers.erase(MA);
> +    }
> +  }
> +}
> +
> +void Sema::RefersToMemberWithReducedAlignment(
> +    Expr *E,
> +    std::function<void(Expr *, RecordDecl *, ValueDecl *, CharUnits)>
> Action) {
> +  const auto *ME = dyn_cast<MemberExpr>(E);
> +  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>()))) {
> +      CharUnits Alignment =
> std::min(Context.getTypeAlignInChars(MD->getType()),
> +
> Context.getTypeAlignInChars(BaseType));
> +      // Notify that this expression designates a member with reduced
> alignment
> +      Action(E, RD, MD, Alignment);
> +      break;
> +    }
> +    ME = dyn_cast<MemberExpr>(ME->getBase());
> +  }
> +}
> +
> +void Sema::CheckAddressOfPackedMember(Expr *rhs) {
> +  using namespace std::placeholders;
> +  RefersToMemberWithReducedAlignment(
> +      rhs, std::bind(&Sema::AddPotentialMisalignedMembers, std::ref(*this),
> _1,
> +                     _2, _3, _4));
> +}
> +
>
> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=278483&r1=278482&r2=278483&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Fri Aug 12 03:04:13 2016
> @@ -6022,7 +6022,9 @@ Sema::ActOnCastExpr(Scope *S, SourceLoca
>    CheckTollFreeBridgeCast(castType, CastExpr);
>
>    CheckObjCBridgeRelatedCast(castType, CastExpr);
> -
> +
> +  DiscardMisalignedMemberAddress(castType.getTypePtr(), CastExpr);
> +
>    return BuildCStyleCastExpr(LParenLoc, castTInfo, RParenLoc, CastExpr);
>  }
>
> @@ -10614,6 +10616,8 @@ QualType Sema::CheckAddressOfOperand(Exp
>    if (op->getType()->isObjCObjectType())
>      return Context.getObjCObjectPointerType(op->getType());
>
> +  CheckAddressOfPackedMember(op);
> +
>    return Context.getPointerType(op->getType());
>  }
>
>
> Modified: cfe/trunk/lib/Sema/SemaInit.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=278483&r1=278482&r2=278483&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug 12 03:04:13 2016
> @@ -6660,12 +6660,16 @@ InitializationSequence::Perform(Sema &S,
>                                      getAssignmentAction(Entity), CCK);
>        if (CurInitExprRes.isInvalid())
>          return ExprError();
> +
> +      S.DiscardMisalignedMemberAddress(Step->Type.getTypePtr(),
> CurInit.get());
> +
>        CurInit = CurInitExprRes;
>
>        if (Step->Kind == SK_ConversionSequenceNoNarrowing &&
>            S.getLangOpts().CPlusPlus && !CurInit.get()->isValueDependent())
>          DiagnoseNarrowingInInitList(S, *Step->ICS, SourceType,
> Entity.getType(),
>                                      CurInit.get());
> +
>        break;
>      }
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>


More information about the cfe-commits mailing list