llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: PiJoules <details> <summary>Changes</summary> This refactors the `noderef` attribute implementation to correctly associate the attribute with pointer and array types rather than the pointee type, fulfilling the requirements for the bracket attribute syntax `[[clang::noderef]]`. Changes include: - Remove the `parsedNoDeref` logic which would apply noderef to the right (pointee) type rather than the left (pointer) type. - Use `getAs<Type>` instead of `dyn_cast<Type>` where appropriate to strip type sugar. - Fixes for some of the tests where we incorrectly associate a noderef with a pointee type rather than a pointer. - Add regression tests Note the bulk of this PR was made by gemini, but it has been thoroughly reviewed and edited by me to the best of my ability. Fixes: #<!-- -->55790 --- Patch is 29.32 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/190100.diff 10 Files Affected: - (modified) clang/include/clang/Sema/Sema.h (+5-1) - (modified) clang/lib/Sema/SemaCast.cpp (+5-9) - (modified) clang/lib/Sema/SemaDeclAttr.cpp (-7) - (modified) clang/lib/Sema/SemaExpr.cpp (+32-25) - (modified) clang/lib/Sema/SemaExprMember.cpp (+3-4) - (modified) clang/lib/Sema/SemaInit.cpp (+4-5) - (modified) clang/lib/Sema/SemaType.cpp (+72-31) - (modified) clang/test/Frontend/noderef.c (+37-24) - (modified) clang/test/Frontend/noderef.cpp (+27-4) - (modified) clang/test/Sema/address_space_print_macro.c (+3-3) ``````````diff diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 1b8a9803be472..9e17c5ba254eb 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -8417,7 +8417,11 @@ class Sema final : public SemaBase { /// keeping a container of all pending expressions and checking if the address /// of them are eventually taken. void CheckSubscriptAccessOfNoDeref(const ArraySubscriptExpr *E); - void CheckAddressOfNoDeref(const Expr *E); + + /// Check if the address of a `noderef` expression is taken. Return true if + /// the expression was marked with `noderef` and the attribute should be + /// propagated to the resulting pointer type. + bool CheckAddressOfNoDeref(const Expr *E); ///@} diff --git a/clang/lib/Sema/SemaCast.cpp b/clang/lib/Sema/SemaCast.cpp index 0040f3aa1a891..d3f1259a3f15a 100644 --- a/clang/lib/Sema/SemaCast.cpp +++ b/clang/lib/Sema/SemaCast.cpp @@ -225,15 +225,11 @@ namespace { void CheckNoDeref(Sema &S, const QualType FromType, const QualType ToType, SourceLocation OpLoc) { - if (const auto *PtrType = dyn_cast<PointerType>(FromType)) { - if (PtrType->getPointeeType()->hasAttr(attr::NoDeref)) { - if (const auto *DestType = dyn_cast<PointerType>(ToType)) { - if (!DestType->getPointeeType()->hasAttr(attr::NoDeref)) { - S.Diag(OpLoc, diag::warn_noderef_to_dereferenceable_pointer); - } - } - } - } + // Note `getAs` strips the sugar to ensure it's a real pointer, but we still + // use the original type since the attribute is part of the sugar. + if (FromType->getAs<PointerType>() && FromType->hasAttr(attr::NoDeref) && + ToType->getAs<PointerType>() && !ToType->hasAttr(attr::NoDeref)) + S.Diag(OpLoc, diag::warn_noderef_to_dereferenceable_pointer); } struct CheckNoDerefRAII { diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index 63a70e4b77bc4..30111b912dd36 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -7445,13 +7445,6 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL, // processTypeAttr(). break; } - - if (AL.getKind() == ParsedAttr::AT_NoDeref) { - // FIXME: `noderef` currently doesn't work correctly in [[]] syntax. - // See https://github.com/llvm/llvm-project/issues/55790 for details. - // We allow processTypeAttrs() to emit a warning and silently move on. - break; - } } // N.B., ClangAttrEmitter.cpp emits a diagnostic helper that ensures a // statement attribute is not written on a declaration, but this code is diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index c9642ed298bf3..2758103fe9af5 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -549,8 +549,14 @@ ExprResult Sema::DefaultFunctionArrayConversion(Expr *E, bool Diagnose) { // T" can be converted to an rvalue of type "pointer to T". // if (getLangOpts().C99 || getLangOpts().CPlusPlus || E->isLValue()) { - ExprResult Res = ImpCastExprToType(E, Context.getArrayDecayedType(Ty), - CK_ArrayToPointerDecay); + QualType PtrTy = Context.getArrayDecayedType(Ty); + + // When the array is `noderef` and we promote to a pointer, also apply + // `noderef` on the pointer. + if (Ty->hasAttr(attr::NoDeref)) + PtrTy = Context.getAttributedType(attr::NoDeref, PtrTy, PtrTy); + + ExprResult Res = ImpCastExprToType(E, PtrTy, CK_ArrayToPointerDecay); if (Res.isInvalid()) return ExprError(); E = Res.get(); @@ -5327,7 +5333,7 @@ ExprResult Sema::CreateBuiltinMatrixSubscriptExpr(Expr *Base, Expr *RowIdx, MTy->getElementType(), RBLoc); } -void Sema::CheckAddressOfNoDeref(const Expr *E) { +bool Sema::CheckAddressOfNoDeref(const Expr *E) { ExpressionEvaluationContextRecord &LastRecord = ExprEvalContexts.back(); const Expr *StrippedExpr = E->IgnoreParenImpCasts(); @@ -5337,7 +5343,7 @@ void Sema::CheckAddressOfNoDeref(const Expr *E) { while ((Member = dyn_cast<MemberExpr>(StrippedExpr)) && !Member->isArrow()) StrippedExpr = Member->getBase()->IgnoreParenImpCasts(); - LastRecord.PossibleDerefs.erase(StrippedExpr); + return LastRecord.PossibleDerefs.erase(StrippedExpr); } void Sema::CheckSubscriptAccessOfNoDeref(const ArraySubscriptExpr *E) { @@ -5351,15 +5357,16 @@ void Sema::CheckSubscriptAccessOfNoDeref(const ArraySubscriptExpr *E) { if (isa<ArrayType>(ResultTy)) return; - if (ResultTy->hasAttr(attr::NoDeref)) { + const Expr *Base = E->getBase(); + QualType BaseTy = Base->getType(); + + if (BaseTy->hasAttr(attr::NoDeref)) { LastRecord.PossibleDerefs.insert(E); return; } // Check if the base type is a pointer to a member access of a struct // marked with noderef. - const Expr *Base = E->getBase(); - QualType BaseTy = Base->getType(); if (!(isa<ArrayType>(BaseTy) || isa<PointerType>(BaseTy))) // Not a pointer access return; @@ -5369,8 +5376,8 @@ void Sema::CheckSubscriptAccessOfNoDeref(const ArraySubscriptExpr *E) { Member->isArrow()) Base = Member->getBase(); - if (const auto *Ptr = dyn_cast<PointerType>(Base->getType())) { - if (Ptr->getPointeeType()->hasAttr(attr::NoDeref)) + if (Base->getType()->getAs<PointerType>()) { + if (Base->getType()->hasAttr(attr::NoDeref)) LastRecord.PossibleDerefs.insert(E); } } @@ -10127,10 +10134,10 @@ AssignConvertType Sema::CheckSingleAssignmentConstraints(QualType LHSType, ExprResult LocalRHS = CallerRHS; ExprResult &RHS = ConvertRHS ? CallerRHS : LocalRHS; - if (const auto *LHSPtrType = LHSType->getAs<PointerType>()) { - if (const auto *RHSPtrType = RHS.get()->getType()->getAs<PointerType>()) { - if (RHSPtrType->getPointeeType()->hasAttr(attr::NoDeref) && - !LHSPtrType->getPointeeType()->hasAttr(attr::NoDeref)) { + if (LHSType->getAs<PointerType>()) { + if (RHS.get()->getType()->getAs<PointerType>()) { + if (RHS.get()->getType()->hasAttr(attr::NoDeref) && + !LHSType->hasAttr(attr::NoDeref)) { Diag(RHS.get()->getExprLoc(), diag::warn_noderef_to_dereferenceable_pointer) << RHS.get()->getSourceRange(); @@ -16203,7 +16210,13 @@ ExprResult Sema::CreateBuiltinUnaryOp(SourceLocation OpLoc, break; case UO_AddrOf: resultType = CheckAddressOfOperand(Input, OpLoc); - CheckAddressOfNoDeref(InputExpr); + + // Since the expression is noderef, the resulting type should also be + // noderef. + if (CheckAddressOfNoDeref(InputExpr)) + resultType = + Context.getAttributedType(attr::NoDeref, resultType, resultType); + RecordModifiableNonNullParam(*this, InputExpr); break; case UO_Deref: { @@ -16404,7 +16417,7 @@ ExprResult Sema::CreateBuiltinUnaryOp(SourceLocation OpLoc, UnaryOperator::Create(Context, Input.get(), Opc, resultType, VK, OK, OpLoc, CanOverflow, CurFPFeatureOverrides()); - if (Opc == UO_Deref && UO->getType()->hasAttr(attr::NoDeref) && + if (Opc == UO_Deref && Input.get()->getType()->hasAttr(attr::NoDeref) && !isa<ArrayType>(UO->getType().getDesugaredType(Context)) && !isUnevaluatedContext()) ExprEvalContexts.back().PossibleDerefs.insert(UO); @@ -18199,17 +18212,11 @@ const DeclRefExpr *CheckPossibleDeref(Sema &S, const Expr *PossibleDeref) { } else if (const auto *E = dyn_cast<MemberExpr>(PossibleDeref)) { return CheckPossibleDeref(S, E->getBase()); } else if (const auto E = dyn_cast<DeclRefExpr>(PossibleDeref)) { - QualType Inner; QualType Ty = E->getType(); - if (const auto *Ptr = Ty->getAs<PointerType>()) - Inner = Ptr->getPointeeType(); - else if (const auto *Arr = S.Context.getAsArrayType(Ty)) - Inner = Arr->getElementType(); - else - return nullptr; - - if (Inner->hasAttr(attr::NoDeref)) - return E; + if (Ty->getAs<PointerType>() || S.Context.getAsArrayType(Ty)) { + if (Ty->hasAttr(attr::NoDeref)) + return E; + } } return nullptr; } diff --git a/clang/lib/Sema/SemaExprMember.cpp b/clang/lib/Sema/SemaExprMember.cpp index ff7c844f66123..f06aaad0663fb 100644 --- a/clang/lib/Sema/SemaExprMember.cpp +++ b/clang/lib/Sema/SemaExprMember.cpp @@ -1766,9 +1766,8 @@ void Sema::CheckMemberAccessOfNoDeref(const MemberExpr *E) { CheckAddressOfNoDeref(E->getBase()); } } else if (E->isArrow()) { - if (const auto *Ptr = dyn_cast<PointerType>( - E->getBase()->getType().getDesugaredType(Context))) { - if (Ptr->getPointeeType()->hasAttr(attr::NoDeref)) + if (E->getBase()->getType()->getAs<PointerType>()) { + if (E->getBase()->getType()->hasAttr(attr::NoDeref)) ExprEvalContexts.back().PossibleDerefs.insert(E); } } @@ -1831,7 +1830,7 @@ Sema::BuildFieldReferenceExpr(Expr *BaseExpr, bool IsArrow, // result. E.g. the expression // &someNoDerefPtr->pointerMember // should be a noderef pointer again. - if (BaseType->hasAttr(attr::NoDeref)) + if (BaseExpr->getType()->hasAttr(attr::NoDeref)) MemberType = Context.getAttributedType(attr::NoDeref, MemberType, MemberType); } diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp index 6c929de9418ed..acbc547060fef 100644 --- a/clang/lib/Sema/SemaInit.cpp +++ b/clang/lib/Sema/SemaInit.cpp @@ -8332,11 +8332,10 @@ ExprResult InitializationSequence::Perform(Sema &S, case SK_ConversionSequence: case SK_ConversionSequenceNoNarrowing: { - if (const auto *FromPtrType = - CurInit.get()->getType()->getAs<PointerType>()) { - if (const auto *ToPtrType = Step->Type->getAs<PointerType>()) { - if (FromPtrType->getPointeeType()->hasAttr(attr::NoDeref) && - !ToPtrType->getPointeeType()->hasAttr(attr::NoDeref)) { + if (CurInit.get()->getType()->getAs<PointerType>()) { + if (Step->Type->getAs<PointerType>()) { + if (CurInit.get()->getType()->hasAttr(attr::NoDeref) && + !Step->Type->hasAttr(attr::NoDeref)) { // Do not check static casts here because they are checked earlier // in Sema::ActOnCXXNamedCast() if (!Kind.isStaticCast()) { diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp index 846474fe94adf..799fc100eb27d 100644 --- a/clang/lib/Sema/SemaType.cpp +++ b/clang/lib/Sema/SemaType.cpp @@ -47,6 +47,7 @@ #include "llvm/ADT/StringExtras.h" #include "llvm/IR/DerivedTypes.h" #include "llvm/Support/ErrorHandling.h" +#include <algorithm> #include <bitset> #include <optional> @@ -217,10 +218,6 @@ namespace { /// stored in a MacroQualifiedTypeLoc. llvm::DenseMap<const MacroQualifiedType *, SourceLocation> LocsForMacros; - /// Flag to indicate we parsed a noderef attribute. This is used for - /// validating that noderef was used on a pointer or array. - bool parsedNoDeref; - // Flag to indicate that we already parsed a HLSL parameter modifier // attribute. This prevents double-mutating the type. bool ParsedHLSLParamMod; @@ -228,7 +225,7 @@ namespace { public: TypeProcessingState(Sema &sema, Declarator &declarator) : sema(sema), declarator(declarator), - chunkIndex(declarator.getNumTypeObjects()), parsedNoDeref(false), + chunkIndex(declarator.getNumTypeObjects()), ParsedHLSLParamMod(false) {} Sema &getSema() const { @@ -360,10 +357,6 @@ namespace { LocsForMacros[MQT] = Loc; } - void setParsedNoDeref(bool parsed) { parsedNoDeref = parsed; } - - bool didParseNoDeref() const { return parsedNoDeref; } - void setParsedHLSLParamMod(bool Parsed) { ParsedHLSLParamMod = Parsed; } bool didParseHLSLParamMod() const { return ParsedHLSLParamMod; } @@ -4688,8 +4681,20 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state, } } + // GNU attributes on a declaration (e.g., __attribute__((noderef)) int *p;) + // are parsed as part of the DeclSpec, but they are intended to apply to + // the first pointer or array declarator chunk encountered. Standard + // C++11 attribute syntax `[[clang::noderef]]` has stricter placement + // rules and should not "float" to the pointer. If a standard noderef + // attribute is found on the DeclSpec, we skip setting `ExpectNoDerefChunk` + // to ensure it is diagnosed for being applied to a non-pointer type, + // rather than silently moving it to the pointer. bool ExpectNoDerefChunk = - state.getCurrentAttributes().hasAttribute(ParsedAttr::AT_NoDeref); + std::any_of(state.getCurrentAttributes().begin(), + state.getCurrentAttributes().end(), [](const ParsedAttr &AL) { + return AL.getKind() == ParsedAttr::AT_NoDeref && + !AL.isStandardAttributeSyntax(); + }); // Walk the DeclTypeInfo, building the recursive type as we go. // DeclTypeInfos are ordered from the identifier out, which is @@ -5471,17 +5476,58 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state, processTypeAttrs(state, T, TAL_DeclChunk, DeclType.getAttrs(), S.CUDA().IdentifyTarget(D.getAttributes())); - if (DeclType.Kind != DeclaratorChunk::Paren) { - if (ExpectNoDerefChunk && !IsNoDerefableChunk(DeclType)) + if (DeclType.Kind != DeclaratorChunk::Paren && ExpectNoDerefChunk) { + // This block handles explicit indirection in the declaration + // + // __attribute__((noderef)) int *p; + // + // If we are looking for a GNU-style noderef attribute that was declared + // on the DeclSpec, we try to apply it to the first pointer or array + // declarator chunk encountered. Once we apply it (or diagnose a warning), + // we set ExpectNoDerefChunk to false so that it only applies to the + // outermost pointer or array chunk and doesn't "bleed" into deeper + // indirection levels as this loop continues. + if (!IsNoDerefableChunk(DeclType)) { S.Diag(DeclType.Loc, diag::warn_noderef_on_non_pointer_or_array); - - ExpectNoDerefChunk = state.didParseNoDeref(); + } else { + for (ParsedAttr &AL : + state.getDeclarator().getMutableDeclSpec().getAttributes()) { + if (AL.getKind() == ParsedAttr::AT_NoDeref) { + T = state.getAttributedType( + createSimpleAttr<NoDerefAttr>(S.Context, AL), T, T); + break; + } + } + } + ExpectNoDerefChunk = false; } } - if (ExpectNoDerefChunk) - S.Diag(state.getDeclarator().getBeginLoc(), - diag::warn_noderef_on_non_pointer_or_array); + // This block handles implicit indirection, such as through a typedef or when + // the base type is already a pointer + // + // typedef int *IntPtr; + // __attribute__((noderef)) IntPtr p; + // + // If the loop above finished without finding an explicit pointer or array + // chunk to attach the attribute to, we check the final resolved type and + // apply it if it's a pointer or array. + if (ExpectNoDerefChunk) { + if (T->isPointerType() || T->isArrayType()) { + for (ParsedAttr &AL : + state.getDeclarator().getMutableDeclSpec().getAttributes()) { + if (AL.getKind() == ParsedAttr::AT_NoDeref) { + ASTContext &Ctx = S.Context; + T = state.getAttributedType(createSimpleAttr<NoDerefAttr>(Ctx, AL), T, + T); + break; + } + } + } else { + S.Diag(state.getDeclarator().getBeginLoc(), + diag::warn_noderef_on_non_pointer_or_array); + } + } // GNU warning -Wstrict-prototypes // Warn if a function declaration or definition is without a prototype. @@ -9008,7 +9054,6 @@ static void processTypeAttrs(TypeProcessingState &state, QualType &type, const ParsedAttributesView &attrs, CUDAFunctionTarget CFT) { - state.setParsedNoDeref(false); if (attrs.empty()) return; @@ -9175,20 +9220,16 @@ static void processTypeAttrs(TypeProcessingState &state, QualType &type, break; case ParsedAttr::AT_NoDeref: { - // FIXME: `noderef` currently doesn't work correctly in [[]] syntax. - // See https://github.com/llvm/llvm-project/issues/55790 for details. - // For the time being, we simply emit a warning that the attribute is - // ignored. - if (attr.isStandardAttributeSyntax()) { - state.getSema().Diag(attr.getLoc(), diag::warn_attribute_ignored) - << attr; - break; - } - ASTContext &Ctx = state.getSema().Context; - type = state.getAttributedType(createSimpleAttr<NoDerefAttr>(Ctx, attr), - type, type); attr.setUsedAsTypeAttr(); - state.setParsedNoDeref(true); + if (!state.isProcessingDeclSpec() || attr.isStandardAttributeSyntax()) { + if (!type->isPointerType() && !type->isArrayType()) { + state.getSema().Diag(attr.getLoc(), + diag::warn_noderef_on_non_pointer_or_array); + } + ASTContext &Ctx = state.getSema().Context; + type = state.getAttributedType(createSimpleAttr<NoDerefAttr>(Ctx, attr), + type, type); + } break; } diff --git a/clang/test/Frontend/noderef.c b/clang/test/Frontend/noderef.c index 3f0d8269ca88d..dbbc39e66d75b 100644 --- a/clang/test/Frontend/noderef.c +++ b/clang/test/Frontend/noderef.c @@ -31,11 +31,11 @@ int test(void) { x = *((int NODEREF *)p2); // expected-warning{{dereferencing expression marked as 'noderef'}} int NODEREF **q; - int *NODEREF *q2; // expected-note 4 {{q2 declared here}} + int *NODEREF *q2; // Indirection x = **q; // expected-warning{{dereferencing expression marked as 'noderef'}} - p2 = *q2; // expected-warning{{dereferencing q2; was declared with a 'noderef' type}} + p2 = *q2; // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}} **q; // expected-warning{{dereferencing expression marked as 'noderef'}} @@ -51,7 +51,7 @@ int test(void) { *p = 2; // expected-warning{{dereferencing p; was declared with a 'noderef' type}} *q = p; // ok **q = 2; // expected-warning{{dereferencing expression marked as 'noderef'}} - *q2 = p2; // expected-warning{{dereferencing q2; was declared with a 'noderef' type}} + *q2 = p2; // ok - casting from normal int pointer to a NODEREF int pointer p = p + 1; p = &*(p + 1); @@ -99,7 +99,7 @@ int test(void) { // Subscript access x = p[1]; // expected-warning{{dereferencing p; was declared with a 'noderef' type}} x = q[0][0]; // expected-warning{{dereferencing expression marked as 'noderef'}} - p2 = q2[0]; // expected-warning{{dereferencing q2; was declared with a 'noderef' type}} + p2 = q2[0]; // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}} p = q[*p]; // expected-warning{{dereferencing p; was declared with a 'noderef' type}} x = p[*p]; // expected-warning{{dereferencing p; was declared with a 'noderef' type}} // expected-warning@-1{{dereferencing p; was declared with a 'noderef' type}} @@ -119,7 +119,11 @@ int test(void) { p = s2_arr[1]->a; p = s2_arr[1]->b; // expected-warning{{dereferencing expression marked as 'noderef'}} - int *NODEREF *bptr = &s2_arr[1]->b; + + // `&s2_arr[1]->b` is a NODEREF pointer to a normal int pointer. + // `bptr` is a normal pointer to a NODEREF int pointer. + // Assigning the former to the latter strips NODEREF from the outer pointer. + int *NODEREF *bptr = &s2_arr[1]->b; // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}} x = s2->s2->a; // expected-warning{{dereferencing expression marked as 'noderef'}} x = s2_noderef->a[1]; // expected-warning{{dereferencing s2_noderef; was declared with a 'noderef' type}} @@ -155,18 +159,25 @@ int test(void) { typedef int_t NODEREF *(noderef_int_t); typedef noderef_int_t *noderef_int_nest... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/190100 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
