[clang] Unify Sema and CodeGen implementation of isFlexibleArrayMemberExpr

[serge-sans-paille]

Summary

Turn it into a single Expr::isFlexibleArrayMemberLike method, as discussed in

https://discourse.llvm.org/t/rfc-harmonize-flexible-array-members-handling

Keep different behavior with respect to macro / template substitution, and
harmonize sharp edges: ObjC interface now behave as C struct wrt. FAM and
-fstrict-flex-arrays.

This does not impact __builtin_object_size interactions with FAM.

[msebor]

Thanks for the heads up! My only general comment is that unless this change has no externally observable effect I'd suggest adding tests. (Otherwise, noting it has no such effect would be helpful.) When I'm not familiar with the code (and often even what I am) I usually look for tests to understand the impact of a change.

clang/include/clang/AST/Expr.h

526

I was a little unsure what "this array" means in the context of an expression but after reading the code I think the intent of the function is to "Return true if this expression designates an array fits the idiom of a flexible array member..." If I have that right then that's what I would suggest to change the comment to.

[serge-sans-paille]

In D134791#3827722, @msebor wrote:

Thanks for the heads up! My only general comment is that unless this change has no externally observable effect I'd suggest adding tests. (Otherwise, noting it has no such effect would be helpful.) When I'm not familiar with the code (and often even what I am) I usually look for tests to understand the impact of a change.

Agredd! This change should only be a refactoring one, but I can see edge cases where it brings some subtle difference (e.g. for ObjCIvarRefExpr but I need to explore that. Will do!)

[serge-sans-paille]

Add test case for ObjC interface with FAM, as hinted by @msebor

[tschuett]

clang/lib/AST/Expr.cpp

225

Could this be an early exit?

267

Could this be an early exit by moving it up?

[serge-sans-paille]

Add an early exit

[serge-sans-paille]

clang/lib/AST/Expr.cpp

225

I don't think so, the first branch is more likely to trigger than this one, so keeping it here seems a better choice (and it's conservative wrt. previous implementation)

[serge-sans-paille]

@aaron.ballman : gentle ping :-)

[aaron.ballman]

In general, this looks reasonable to me.

clang/include/clang/AST/Expr.h

529

531

Do we want to make the array levels into an enumeration instead of letting the user pass arbitrary integers? (Perhaps not as part of this review.)

clang/lib/AST/Expr.cpp

244–247

[serge-sans-paille]

Address reviewers comment.

[serge-sans-paille]

clang/include/clang/AST/Expr.h

531

Something along https://reviews.llvm.org/D135107 :-) ?

[aaron.ballman]

bolt/lib/RuntimeLibs/RuntimeLibrary.cpp

31–36

These changes seem unrelated?

clang/include/clang/AST/Expr.h

531

LOL and that's why my suggestion seemed so familiar. :-D

[serge-sans-paille]

Removed a diff that sneaked in.

[aaron.ballman]

Assuming precommit CI comes back clean, this LGTM. Thanks for the cleanup!

[serge-sans-paille]

Thanks for the reviews!

[steakhal]

I wonder if we could come up with an overload or something for cases when we only have a FieldDecl.
For example, in the Clang Static Analyzer, we have something similar at MemRegion.cpp#L784-L811, and it would be great if we could also make use of this harmonized flexible-array-members detection.

Actually, @serge-sans-paille inserted a FIXME about this, and I believe it's correct.

const AnalyzerOptions &Opts = SVB.getAnalyzerOptions();
// FIXME: this option is probably redundant with -fstrict-flex-arrays=1.
if (Opts.ShouldConsiderSingleElementArraysAsFlexibleArrayMembers && Size.isOne())
  return true;

Unfortunately, in this context we don't have an Expr; we only have a FieldDecl, so I'm looking for some way to plug this isFlexibleArrayMemberLike() into this.
I'm thinking of like introducing another overload for it accepting FieldDecl instead of Expr along with the current one.

@serge-sans-paille what do you think about this?

bolt/lib/RuntimeLibs/RuntimeLibrary.cpp

 using namespace bolt;
 
 void RuntimeLibrary::anchor() {}
 
 std::string RuntimeLibrary::getLibPath(StringRef ToolPath,
                                        StringRef LibFileName) {
   StringRef Dir = llvm::sys::path::parent_path(ToolPath);
   SmallString<128> LibPath = llvm::sys::path::parent_path(Dir);
-  llvm::sys::path::append(LibPath, "lib");
+  llvm::sys::path::append(LibPath, "lib64");
   if (!llvm::sys::fs::exists(LibPath)) {
     // In some cases we install bolt binary into one level deeper in bin/,
     // we need to go back one more level to find lib directory.
     LibPath = llvm::sys::path::parent_path(llvm::sys::path::parent_path(Dir));
-    llvm::sys::path::append(LibPath, "lib");
+    llvm::sys::path::append(LibPath, "lib64");
   }
   llvm::sys::path::append(LibPath, LibFileName);
   if (!llvm::sys::fs::exists(LibPath)) {
     errs() << "BOLT-ERROR: library not found: " << LibPath << "\n";
     exit(1);
   }
   return std::string(LibPath.str());
 }

clang/include/clang/AST/Expr.h

public:
   ///
   /// \param Semantic If true, only return true for expressions that are known
   /// to be semantically boolean, which might not be true even for expressions
   /// that are known to evaluate to 0/1. For instance, reading an unsigned
   /// bit-field with width '1' will evaluate to 0/1, but doesn't necessarily
   /// semantically correspond to a bool.
   bool isKnownToHaveBooleanValue(bool Semantic = true) const;
 
+  /// Check whether this array fits the idiom of a flexible array member,
+  /// depending on the value of -fstrict-flex-array.
+  /// When IgnoreTemplateOrMacroSubstitution is set, it doesn't consider sizes
+  /// resulting from the substitution of a macro or a template as special sizes.
+  bool isFlexibleArrayMemberLike(
+      ASTContext &Context, unsigned StrictFlexArraysLevel,
+      bool IgnoreTemplateOrMacroSubstitution = false) const;
+
   /// isIntegerConstantExpr - Return the value if this expression is a valid
   /// integer constant expression.  If not a valid i-c-e, return None and fill
   /// in Loc (if specified) with the location of the invalid expression.
   ///
   /// Note: This does not perform the implicit conversions required by C++11
   /// [expr.const]p5.
   Optional<llvm::APSInt> getIntegerConstantExpr(const ASTContext &Ctx,
                                                 SourceLocation *Loc = nullptr,

clang/lib/AST/Expr.cpp

if (const FieldDecl *FD = E->getSourceBitField())
     if (!Semantic && FD->getType()->isUnsignedIntegerType() &&
         !FD->getBitWidth()->isValueDependent() &&
         FD->getBitWidthValue(FD->getASTContext()) == 1)
       return true;
 
   return false;
 }
 
+bool Expr::isFlexibleArrayMemberLike(
+    ASTContext &Context, unsigned StrictFlexArraysLevel,
+    bool IgnoreTemplateOrMacroSubstitution) const {
+
+  // For compatibility with existing code, we treat arrays of length 0 or
+  // 1 as flexible array members.
+  if (const auto *CAT = Context.getAsConstantArrayType(getType())) {
+    llvm::APInt Size = CAT->getSize();
+
+    // GCC extension, only allowed to represent a FAM.
+    if (Size == 0)
+      return true;
+
+    // FIXME: While the default -fstrict-flex-arrays=0 permits Size>1 trailing
+    // arrays to be treated as flexible-array-members, we still emit diagnostics
+    // as if they are not. Pending further discussion...
+    if (StrictFlexArraysLevel >= 2 || Size.uge(2))
+      return false;
+
+  } else if (!Context.getAsIncompleteArrayType(getType()))
+    return false;
+
+  const Expr *E = IgnoreParens();
+
+  const NamedDecl *ND = nullptr;
+  if (const auto *DRE = dyn_cast<DeclRefExpr>(E))
+    ND = DRE->getDecl();
+  else if (const auto *ME = dyn_cast<MemberExpr>(E))
+    ND = ME->getMemberDecl();
+  else if (const auto *IRE = dyn_cast<ObjCIvarRefExpr>(E))
+    return IRE->getDecl()->getNextIvar() == nullptr;
+
+  if (!ND)
+    return false;
+
+  // A flexible array member must be the last member in the class.
+  // FIXME: If the base type of the member expr is not FD->getParent(),
+  // this should not be treated as a flexible array member access.
+  if (const auto *FD = dyn_cast<FieldDecl>(ND)) {
+    if (FD->getParent()->isUnion())
+      return true;
+
+    // Don't consider sizes resulting from macro expansions or template argument
+    // substitution to form C89 tail-padded arrays.
+    if (IgnoreTemplateOrMacroSubstitution) {
+      TypeSourceInfo *TInfo = FD->getTypeSourceInfo();
+      while (TInfo) {
+        TypeLoc TL = TInfo->getTypeLoc();
+        // Look through typedefs.
+        if (TypedefTypeLoc TTL = TL.getAsAdjusted<TypedefTypeLoc>()) {
+          const TypedefNameDecl *TDL = TTL.getTypedefNameDecl();
+          TInfo = TDL->getTypeSourceInfo();
+          continue;
+        }
+        if (ConstantArrayTypeLoc CTL = TL.getAs<ConstantArrayTypeLoc>()) {
+          const Expr *SizeExpr = dyn_cast<IntegerLiteral>(CTL.getSizeExpr());
+          if (!SizeExpr || SizeExpr->getExprLoc().isMacroID())
+            return false;
+        }
+        break;
+      }
+    }
+
+    RecordDecl::field_iterator FI(
+        DeclContext::decl_iterator(const_cast<FieldDecl *>(FD)));
+    return ++FI == FD->getParent()->field_end();
+  }
+
+  return false;
+}
+
 const ValueDecl *
 Expr::getAsBuiltinConstantDeclRef(const ASTContext &Context) const {
   Expr::EvalResult Eval;
 
   if (EvaluateAsConstantExpr(Eval, Context)) {
     APValue &Value = Eval.Val;
 
     if (Value.isMemberPointer())

clang/lib/CodeGen/CGExpr.cpp

void CodeGenFunction::EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc,
   }
 
   if (Done) {
     Builder.CreateBr(Done);
     EmitBlock(Done);
   }
 }
 
-/// Determine whether this expression refers to a flexible array member in a
-/// struct. We disable array bounds checks for such members.
-static bool isFlexibleArrayMemberExpr(const Expr *E,
-                                      unsigned StrictFlexArraysLevel) {
-  // For compatibility with existing code, we treat arrays of length 0 or
-  // 1 as flexible array members.
-  // FIXME: This is inconsistent with the warning code in SemaChecking. Unify
-  // the two mechanisms.
-  const ArrayType *AT = E->getType()->castAsArrayTypeUnsafe();
-  if (const auto *CAT = dyn_cast<ConstantArrayType>(AT)) {
-    // FIXME: Sema doesn't treat [1] as a flexible array member if the bound
-    // was produced by macro expansion.
-    if (StrictFlexArraysLevel >= 2 && CAT->getSize().ugt(0))
-      return false;
-    // FIXME: While the default -fstrict-flex-arrays=0 permits Size>1 trailing
-    // arrays to be treated as flexible-array-members, we still emit ubsan
-    // checks as if they are not.
-    if (CAT->getSize().ugt(1))
-      return false;
-  } else if (!isa<IncompleteArrayType>(AT))
-    return false;
-
-  E = E->IgnoreParens();
-
-  // A flexible array member must be the last member in the class.
-  if (const auto *ME = dyn_cast<MemberExpr>(E)) {
-    // FIXME: If the base type of the member expr is not FD->getParent(),
-    // this should not be treated as a flexible array member access.
-    if (const auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl())) {
-      // FIXME: Sema doesn't treat a T[1] union member as a flexible array
-      // member, only a T[0] or T[] member gets that treatment.
-      if (FD->getParent()->isUnion())
-        return true;
-      RecordDecl::field_iterator FI(
-          DeclContext::decl_iterator(const_cast<FieldDecl *>(FD)));
-      return ++FI == FD->getParent()->field_end();
-    }
-  } else if (const auto *IRE = dyn_cast<ObjCIvarRefExpr>(E)) {
-    return IRE->getDecl()->getNextIvar() == nullptr;
-  }
-
-  return false;
-}
-
 llvm::Value *CodeGenFunction::LoadPassedObjectSize(const Expr *E,
                                                    QualType EltTy) {
   ASTContext &C = getContext();
   uint64_t EltSize = C.getTypeSizeInChars(EltTy).getQuantity();
   if (!EltSize)
     return nullptr;
 
   auto *ArrayDeclRef = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts());
if (const VectorType *VT = Base->getType()->getAs<VectorType>()) {
     IndexedType = Base->getType();
     return CGF.Builder.getInt32(VT->getNumElements());
   }
 
   Base = Base->IgnoreParens();
 
   if (const auto *CE = dyn_cast<CastExpr>(Base)) {
     if (CE->getCastKind() == CK_ArrayToPointerDecay &&
-        !isFlexibleArrayMemberExpr(CE->getSubExpr(), StrictFlexArraysLevel)) {
+        !CE->getSubExpr()->isFlexibleArrayMemberLike(CGF.getContext(),
+                                                     StrictFlexArraysLevel)) {
       IndexedType = CE->getSubExpr()->getType();
       const ArrayType *AT = IndexedType->castAsArrayTypeUnsafe();
       if (const auto *CAT = dyn_cast<ConstantArrayType>(AT))
         return CGF.Builder.getInt(CAT->getSize());
       else if (const auto *VAT = dyn_cast<VariableArrayType>(AT))
         return CGF.getVLASize(VAT).NumElts;
       // Ignore pass_object_size here. It's not applicable on decayed pointers.
     }

clang/lib/Sema/SemaChecking.cpp

void Sema::CheckCastAlign(Expr *Op, QualType T, SourceRange TRange) {
 
   Diag(TRange.getBegin(), diag::warn_cast_align)
     << Op->getType() << T
     << static_cast<unsigned>(SrcAlign.getQuantity())
     << static_cast<unsigned>(DestAlign.getQuantity())
     << TRange << Op->getSourceRange();
 }
 
-/// Check whether this array fits the idiom of a flexible array member,
-/// depending on the value of -fstrict-flex-array.
-///
-/// We avoid emitting out-of-bounds access warnings for such arrays.
-static bool isFlexibleArrayMemberExpr(Sema &S, const Expr *E,
-                                      const NamedDecl *ND,
-                                      unsigned StrictFlexArraysLevel) {
-
-  if (!ND)
-    return false;
-
-  const ConstantArrayType *ArrayTy =
-      S.Context.getAsConstantArrayType(E->getType());
-  llvm::APInt Size = ArrayTy->getSize();
-
-  if (Size == 0)
-    return true;
-
-  // FIXME: While the default -fstrict-flex-arrays=0 permits Size>1 trailing
-  // arrays to be treated as flexible-array-members, we still emit diagnostics
-  // as if they are not. Pending further discussion...
-  if (StrictFlexArraysLevel >= 2 || Size.uge(2))
-    return false;
-
-  const FieldDecl *FD = dyn_cast<FieldDecl>(ND);
-  if (!FD)
-    return false;
-
-  // Don't consider sizes resulting from macro expansions or template argument
-  // substitution to form C89 tail-padded arrays.
-
-  TypeSourceInfo *TInfo = FD->getTypeSourceInfo();
-  while (TInfo) {
-    TypeLoc TL = TInfo->getTypeLoc();
-    // Look through typedefs.
-    if (TypedefTypeLoc TTL = TL.getAsAdjusted<TypedefTypeLoc>()) {
-      const TypedefNameDecl *TDL = TTL.getTypedefNameDecl();
-      TInfo = TDL->getTypeSourceInfo();
-      continue;
-    }
-    if (ConstantArrayTypeLoc CTL = TL.getAs<ConstantArrayTypeLoc>()) {
-      const Expr *SizeExpr = dyn_cast<IntegerLiteral>(CTL.getSizeExpr());
-      if (!SizeExpr || SizeExpr->getExprLoc().isMacroID())
-        return false;
-    }
-    break;
-  }
-
-  const RecordDecl *RD = dyn_cast<RecordDecl>(FD->getDeclContext());
-  if (!RD)
-    return false;
-  if (RD->isUnion())
-    return false;
-  if (const CXXRecordDecl *CRD = dyn_cast<CXXRecordDecl>(RD)) {
-    if (!CRD->isStandardLayout())
-      return false;
-  }
-
-  // See if this is the last field decl in the record.
-  const Decl *D = FD;
-  while ((D = D->getNextDeclInContext()))
-    if (isa<FieldDecl>(D))
-      return false;
-  return true;
-}
-
 void Sema::CheckArrayAccess(const Expr *BaseExpr, const Expr *IndexExpr,
                             const ArraySubscriptExpr *ASE,
                             bool AllowOnePastEnd, bool IndexNegated) {
   // Already diagnosed by the constant evaluator.
   if (isConstantEvaluated())
     return;
 
   IndexExpr = IndexExpr->IgnoreParenImpCasts();
   if (IndexExpr->isValueDependent())
     return;
 
   const Type *EffectiveType =
       BaseExpr->getType()->getPointeeOrArrayElementType();
   BaseExpr = BaseExpr->IgnoreParenCasts();
   const ConstantArrayType *ArrayTy =
       Context.getAsConstantArrayType(BaseExpr->getType());
 
   unsigned StrictFlexArraysLevel = getLangOpts().StrictFlexArrays;
 
-  const NamedDecl *ND = nullptr;
-  if (const auto *DRE = dyn_cast<DeclRefExpr>(BaseExpr))
-    ND = DRE->getDecl();
-  else if (const auto *ME = dyn_cast<MemberExpr>(BaseExpr))
-    ND = ME->getMemberDecl();
-
   const Type *BaseType =
       ArrayTy == nullptr ? nullptr : ArrayTy->getElementType().getTypePtr();
   bool IsUnboundedArray =
-      BaseType == nullptr ||
-      isFlexibleArrayMemberExpr(*this, BaseExpr, ND, StrictFlexArraysLevel);
+      BaseType == nullptr || BaseExpr->isFlexibleArrayMemberLike(
+                                 Context, StrictFlexArraysLevel,
+                                 /*IgnoreTemplateOrMacroSubstitution=*/true);
   if (EffectiveType->isDependentType() ||
       (!IsUnboundedArray && BaseType->isDependentType()))
     return;
 
   Expr::EvalResult Result;
   if (!IndexExpr->EvaluateAsInt(Result, Context, Expr::SE_AllowSideEffects))
     return;
 
if (index.isUnsigned() || !index.isNegative()) {
                           PDiag(DiagID)
                               << toString(index, 10, true) << AddrBits
                               << (unsigned)ASTC.toBits(*ElemCharUnits)
                               << toString(ElemBytes, 10, false)
                               << toString(MaxElems, 10, false)
                               << (unsigned)MaxElems.getLimitedValue(~0U)
                               << IndexExpr->getSourceRange());
 
-      if (!ND) {
-        // Try harder to find a NamedDecl to point at in the note.
-        while (const auto *ASE = dyn_cast<ArraySubscriptExpr>(BaseExpr))
-          BaseExpr = ASE->getBase()->IgnoreParenCasts();
-        if (const auto *DRE = dyn_cast<DeclRefExpr>(BaseExpr))
-          ND = DRE->getDecl();
-        if (const auto *ME = dyn_cast<MemberExpr>(BaseExpr))
-          ND = ME->getMemberDecl();
-      }
+      const NamedDecl *ND = nullptr;
 
       if (ND)
         DiagRuntimeBehavior(ND->getBeginLoc(), BaseExpr,
                             PDiag(diag::note_array_declared_here) << ND);
     }
     return;
   }
 
if (!ASE) {
       if (index.isNegative()) index = -index;
     }
 
     DiagRuntimeBehavior(BaseExpr->getBeginLoc(), BaseExpr,
                         PDiag(DiagID) << toString(index, 10, true)
                                       << IndexExpr->getSourceRange());
   }
 
-  if (!ND) {
-    // Try harder to find a NamedDecl to point at in the note.
-    while (const auto *ASE = dyn_cast<ArraySubscriptExpr>(BaseExpr))
-      BaseExpr = ASE->getBase()->IgnoreParenCasts();
-    if (const auto *DRE = dyn_cast<DeclRefExpr>(BaseExpr))
-      ND = DRE->getDecl();
-    if (const auto *ME = dyn_cast<MemberExpr>(BaseExpr))
-      ND = ME->getMemberDecl();
-  }
+  const NamedDecl *ND = nullptr;
 
   if (ND)
     DiagRuntimeBehavior(ND->getBeginLoc(), BaseExpr,
                         PDiag(diag::note_array_declared_here) << ND);
 }
 
 void Sema::CheckArrayAccess(const Expr *expr) {
   int AllowOnePastEnd = 0;

clang/test/SemaObjC/flexible-array-bounds.m

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+//
+// RUN: %clang_cc1 -fsyntax-only -fstrict-flex-arrays=2 -verify=warn %s
+
+@interface Flexible {
+@public
+  char flexible[];
+}
+@end
+
+@interface Flexible0 {
+@public
+  char flexible[0];
+}
+@end
+
+@interface Flexible1 {
+@public
+  char flexible[1];
+}
+@end
+
+char readit(Flexible *p) { return p->flexible[2]; }
+char readit0(Flexible0 *p) { return p->flexible[2]; }
+char readit1(Flexible1 *p) { return p->flexible[2]; } // warn-warning {{array index 2 is past the end of the array (which contains 1 element)}}
+