[clang] Function type attribute to prevent CFI instrumentation #135836
+584
−7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This introduces the attribute discussed in https://discourse.llvm.org/t/rfc-function-type-attribute-to-prevent-cfi-instrumentation/85458. The proposed name has been changed from `no_cfi` to `cfi_unchecked_callee` to help differentiate from `no_sanitize("cfi")` more easily. The proposed attribute has the following semantics: 1. Indirect calls to a function type with this attribute will not be instrumented with CFI. That is, the indirect call will not be checked. Note that this only changes the behavior for indirect calls on pointers to function types having this attribute. It does not prevent all indirect function calls for a given type from being checked. 2. All direct references to a function whose type has this attribute will always reference the true function definition rather than an entry in the CFI jump table. 3. When a pointer to a function with this attribute is implicitly cast to a pointer to a function without this attribute, the compiler will give a warning saying this attribute is discarded. This warning can be silenced with an explicit C-style cast or C++ static_cast.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: None (PiJoules) ChangesThis introduces the attribute discussed in The proposed name has been changed from
Patch is 39.15 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/135836.diff 19 Files Affected:
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 74886ef0cd824..9716fd338f2f0 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -2730,6 +2730,15 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
bool isOverloadableType() const;
+ /// Return true if this type is marked with the `cfi_unchecked_callee`
+ /// attribute.
+ bool hasCFIUncheckedCallee(const ASTContext &) const;
+
+ /// Return true if this type is a pointer to a function or member function
+ /// marked with the `cfi_unchecked_callee` attribute.
+ bool isPointerToCFIUncheckedCalleeFunctionOrMemberFunction(
+ const ASTContext &) const;
+
/// Determine wither this type is a C++ elaborated-type-specifier.
bool isElaboratedTypeSpecifier() const;
@@ -8627,6 +8636,40 @@ inline bool Type::isOverloadableType() const {
!isMemberPointerType();
}
+inline bool Type::hasCFIUncheckedCallee(const ASTContext &Context) const {
+ // Carefully strip sugar coating the underlying attributed type. We don't
+ // want to remove all the sugar because this will remove any wrapping
+ // attributed types.
+ QualType Ty(this, /*Quals=*/0);
+
+ while (1) {
+ if (Ty->hasAttr(attr::CFIUncheckedCallee))
+ return true;
+
+ QualType Desugared = Ty.getSingleStepDesugaredType(Context);
+
+ // This means the type has no more sugar.
+ if (Ty == Desugared)
+ break;
+
+ Ty = Desugared;
+ }
+
+ return false;
+}
+
+inline bool Type::isPointerToCFIUncheckedCalleeFunctionOrMemberFunction(
+ const ASTContext &Context) const {
+ if (const auto *MFT = dyn_cast<MemberPointerType>(this)) {
+ if (MFT->isMemberFunctionPointer())
+ return MFT->getPointeeType()->hasCFIUncheckedCallee(Context);
+ } else if (const auto *PtrTy = dyn_cast<PointerType>(this)) {
+ return PtrTy->getPointeeType()->hasCFIUncheckedCallee(Context);
+ }
+
+ return false;
+}
+
/// Determines whether this type is written as a typedef-name.
inline bool Type::isTypedefNameType() const {
if (getAs<TypedefType>())
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index b7ad432738b29..1c2b62c2cc63e 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3053,6 +3053,11 @@ def NoDeref : TypeAttr {
let Documentation = [NoDerefDocs];
}
+def CFIUncheckedCallee : TypeAttr {
+ let Spellings = [Clang<"cfi_unchecked_callee">];
+ let Documentation = [CFIUncheckedCalleeDocs];
+}
+
def ReqdWorkGroupSize : InheritableAttr {
// Does not have a [[]] spelling because it is an OpenCL-related attribute.
let Spellings = [GNU<"reqd_work_group_size">];
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 97a5f24d35d7d..6477ee4d4c461 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -6821,6 +6821,54 @@ for references or Objective-C object pointers.
}];
}
+def CFIUncheckedCalleeDocs : Documentation {
+ let Category = DocCatType;
+ let Content = [{
+``cfi_unchecked_callee`` is a function type attribute which prevents the compiler from instrumenting
+`Control Flow Integrity <https://clang.llvm.org/docs/ControlFlowIntegrity.html>`_ checks on indirect
+function calls. Specifically, the attribute has the following semantics:
+
+1. Indirect calls to a function type with this attribute will not be instrumented with CFI. That is,
+ the indirect call will not be checked. Note that this only changes the behavior for indirect calls
+ on pointers to function types having this attribute. It does not prevent all indirect function calls
+ for a given type from being checked.
+2. All direct references to a function whose type has this attribute will always reference the
+ function definition rather than an entry in the CFI jump table.
+3. When a pointer to a function with this attribute is implicitly cast to a pointer to a function
+ without this attribute, the compiler will give a warning saying this attribute is discarded. This
+ warning can be silenced with an explicit cast. Note an explicit cast just disables the warning, so
+ direct references to a function with a ``cfi_unchecked_callee`` attribute will still reference the
+ function definition rather than the CFI jump table.
+
+.. code-block:: c
+
+ #define CFI_UNCHECKED_CALLEE __attribute__((cfi_unchecked_callee))
+
+ void no_cfi() CFI_UNCHECKED_CALLEE {}
+
+ void (*with_cfi)() = no_cfi; // warning: implicit conversion discards `cfi_unchecked_callee` attribute.
+ // `with_cfi` also points to the actual definition of `no_cfi` rather than
+ // its jump table entry.
+
+ void invoke(void (CFI_UNCHECKED_CALLEE *func)()) {
+ func(); // CFI will not instrument this indirect call.
+
+ void (*func2)() = func; // warning: implicit conversion discards `cfi_unchecked_callee` attribute.
+
+ func2(); // CFI will instrument this indirect call. Users should be careful however because if this
+ // references a function with type `cfi_unchecked_callee`, then the CFI check may incorrectly
+ // fail because the reference will be to the function definition rather than the CFI jump
+ // table entry.
+ }
+
+This attribute can only be applied on functions or member functions. This attribute can be a good
+alternative to ``no_sanitize("cfi")`` if you only want to disable innstrumentation for specific indirect
+calls rather than applying ``no_sanitize("cfi")`` on the whole function containing indirect call. Note
+that ``cfi_unchecked_attribute`` is a type attribute doesn't disable CFI instrumentation on a function
+body.
+}];
+}
+
def ReinitializesDocs : Documentation {
let Category = DocCatFunction;
let Content = [{
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index d97bbfee2e4d5..cab7d7a977637 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1541,6 +1541,8 @@ def FunctionMultiVersioning
def NoDeref : DiagGroup<"noderef">;
+def CFIUncheckedCallee : DiagGroup<"cfi-unchecked-callee">;
+
// -fbounds-safety and bounds annotation related warnings
def BoundsSafetyCountedByEltTyUnknownSize :
DiagGroup<"bounds-safety-counted-by-elt-type-unknown-size">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f4ab620ae61d2..3b1dbcf0009f0 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12468,6 +12468,15 @@ def warn_noderef_on_non_pointer_or_array : Warning<
def warn_noderef_to_dereferenceable_pointer : Warning<
"casting to dereferenceable pointer removes 'noderef' attribute">, InGroup<NoDeref>;
+def warn_cfi_unchecked_callee_on_non_function
+ : Warning<"use of `cfi_unchecked_callee` on %0; can only be used on "
+ "function types">,
+ InGroup<CFIUncheckedCallee>;
+def warn_cast_discards_cfi_unchecked_callee
+ : Warning<"implicit conversion from %0 to %1 discards "
+ "`cfi_unchecked_callee` attribute">,
+ InGroup<CFIUncheckedCallee>;
+
def err_builtin_launder_invalid_arg : Error<
"%select{non-pointer|function pointer|void pointer}0 argument to "
"'__builtin_launder' is not allowed">;
diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index cc8c874140167..b6be8e494f2f4 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -2088,6 +2088,9 @@ void TypePrinter::printAttributedAfter(const AttributedType *T,
case attr::NoDeref:
OS << "noderef";
break;
+ case attr::CFIUncheckedCallee:
+ OS << "cfi_unchecked_callee";
+ break;
case attr::AcquireHandle:
OS << "acquire_handle";
break;
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 3da21cebd9d68..81c5c0004fa58 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -2973,6 +2973,10 @@ static LValue EmitFunctionDeclLValue(CodeGenFunction &CGF, const Expr *E,
GlobalDecl GD) {
const FunctionDecl *FD = cast<FunctionDecl>(GD.getDecl());
llvm::Constant *V = CGF.CGM.getFunctionPointer(GD);
+ if (E->getType()->hasAttr(attr::CFIUncheckedCallee)) {
+ if (auto *GV = dyn_cast<llvm::GlobalValue>(V))
+ V = llvm::NoCFIValue::get(GV);
+ }
CharUnits Alignment = CGF.getContext().getDeclAlign(FD);
return CGF.MakeAddrLValue(V, E->getType(), Alignment,
AlignmentSource::Decl);
@@ -6064,7 +6068,7 @@ LValue CodeGenFunction::EmitStmtExprLValue(const StmtExpr *E) {
AlignmentSource::Decl);
}
-RValue CodeGenFunction::EmitCall(QualType CalleeType,
+RValue CodeGenFunction::EmitCall(QualType OriginalCalleeType,
const CGCallee &OrigCallee, const CallExpr *E,
ReturnValueSlot ReturnValue,
llvm::Value *Chain,
@@ -6072,7 +6076,7 @@ RValue CodeGenFunction::EmitCall(QualType CalleeType,
CGFunctionInfo const **ResolvedFnInfo) {
// Get the actual function type. The callee type will always be a pointer to
// function type or a block pointer type.
- assert(CalleeType->isFunctionPointerType() &&
+ assert(OriginalCalleeType->isFunctionPointerType() &&
"Call must have function pointer type!");
const Decl *TargetDecl =
@@ -6082,7 +6086,7 @@ RValue CodeGenFunction::EmitCall(QualType CalleeType,
!cast<FunctionDecl>(TargetDecl)->isImmediateFunction()) &&
"trying to emit a call to an immediate function");
- CalleeType = getContext().getCanonicalType(CalleeType);
+ QualType CalleeType = getContext().getCanonicalType(OriginalCalleeType);
auto PointeeType = cast<PointerType>(CalleeType)->getPointeeType();
@@ -6168,10 +6172,16 @@ RValue CodeGenFunction::EmitCall(QualType CalleeType,
FD && FD->hasAttr<OpenCLKernelAttr>())
CGM.getTargetCodeGenInfo().setOCLKernelStubCallingConvention(FnType);
+ // Use the original callee type because the canonical type will have
+ // attributes stripped.
+ bool CFIUnchecked =
+ OriginalCalleeType->isPointerToCFIUncheckedCalleeFunctionOrMemberFunction(
+ CGM.getContext());
+
// If we are checking indirect calls and this call is indirect, check that the
// function pointer is a member of the bit set for the function type.
if (SanOpts.has(SanitizerKind::CFIICall) &&
- (!TargetDecl || !isa<FunctionDecl>(TargetDecl))) {
+ (!TargetDecl || !isa<FunctionDecl>(TargetDecl)) && !CFIUnchecked) {
SanitizerScope SanScope(this);
EmitSanitizerStatReport(llvm::SanStat_CFI_ICall);
diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
index b016c6e36d1a8..3670a8495aefa 100644
--- a/clang/lib/CodeGen/CGExprConstant.cpp
+++ b/clang/lib/CodeGen/CGExprConstant.cpp
@@ -2219,8 +2219,12 @@ ConstantLValueEmitter::tryEmitBase(const APValue::LValueBase &base) {
return ConstantLValue(C);
};
- if (const auto *FD = dyn_cast<FunctionDecl>(D))
- return PtrAuthSign(CGM.getRawFunctionPointer(FD));
+ if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+ llvm::Constant *C = CGM.getRawFunctionPointer(FD);
+ if (FD->getType()->hasAttr(attr::CFIUncheckedCallee))
+ C = llvm::NoCFIValue::get(cast<llvm::GlobalValue>(C));
+ return PtrAuthSign(C);
+ }
if (const auto *VD = dyn_cast<VarDecl>(D)) {
// We can never refer to a variable with local storage.
diff --git a/clang/lib/CodeGen/CGPointerAuth.cpp b/clang/lib/CodeGen/CGPointerAuth.cpp
index 4b032306ead72..2302580a1d1bd 100644
--- a/clang/lib/CodeGen/CGPointerAuth.cpp
+++ b/clang/lib/CodeGen/CGPointerAuth.cpp
@@ -388,6 +388,11 @@ llvm::Constant *CodeGenModule::getMemberFunctionPointer(llvm::Constant *Pointer,
Pointer, PointerAuth.getKey(), nullptr,
cast_or_null<llvm::ConstantInt>(PointerAuth.getDiscriminator()));
+ if (const auto *MFT = dyn_cast<MemberPointerType>(FT.getTypePtr())) {
+ if (MFT->isPointerToCFIUncheckedCalleeFunctionOrMemberFunction(Context))
+ Pointer = llvm::NoCFIValue::get(cast<llvm::GlobalValue>(Pointer));
+ }
+
return Pointer;
}
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 70b53be7e77a3..62552fcec7ff6 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -693,6 +693,19 @@ CGCallee ItaniumCXXABI::EmitLoadOfMemberFunctionPointer(
llvm::Constant *CheckTypeDesc;
bool ShouldEmitCFICheck = CGF.SanOpts.has(SanitizerKind::CFIMFCall) &&
CGM.HasHiddenLTOVisibility(RD);
+
+ if (ShouldEmitCFICheck) {
+ if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
+ if (BinOp->isPtrMemOp()) {
+ if (BinOp->getRHS()
+ ->getType()
+ ->isPointerToCFIUncheckedCalleeFunctionOrMemberFunction(
+ CGM.getContext()))
+ ShouldEmitCFICheck = false;
+ }
+ }
+ }
+
bool ShouldEmitVFEInfo = CGM.getCodeGenOpts().VirtualFunctionElimination &&
CGM.HasHiddenLTOVisibility(RD);
bool ShouldEmitWPDInfo =
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 20ea38b7e05db..5dc63bf8e692f 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -916,6 +916,18 @@ static void handleDiagnoseIfAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
cast<NamedDecl>(D)));
}
+static void handleCFIUncheckedCalleeAttr(Sema &S, Decl *D,
+ const ParsedAttr &AL) {
+ // Just check for TagDecls (structs/classes) here. All other types are
+ // diagnosed elsewhere in Sema.
+ if (const auto *TD = dyn_cast<TagDecl>(D)) {
+ if (!D->getFunctionType() && !TD->isDependentType()) {
+ S.Diag(AL.getLoc(), diag::warn_cfi_unchecked_callee_on_non_function)
+ << QualType(TD->getTypeForDecl(), /*Quals=*/0);
+ }
+ }
+}
+
static void handleNoBuiltinAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
static constexpr const StringRef kWildcard = "*";
@@ -7103,6 +7115,9 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
case ParsedAttr::AT_NoBuiltin:
handleNoBuiltinAttr(S, D, AL);
break;
+ case ParsedAttr::AT_CFIUncheckedCallee:
+ handleCFIUncheckedCalleeAttr(S, D, AL);
+ break;
case ParsedAttr::AT_ExtVectorType:
handleExtVectorTypeAttr(S, D, AL);
break;
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index c25daaa022f49..9f8846c521419 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -9837,6 +9837,17 @@ Sema::CheckSingleAssignmentConstraints(QualType LHSType, ExprResult &CallerRHS,
Sema::AssignConvertType result =
CheckAssignmentConstraints(LHSType, RHS, Kind, ConvertRHS);
+ if (const auto *IC = dyn_cast_or_null<ImplicitCastExpr>(RHS.get())) {
+ if (result != Incompatible && !IC->isPartOfExplicitCast() &&
+ IC->getType()->isPointerToCFIUncheckedCalleeFunctionOrMemberFunction(
+ Context) &&
+ !LHSType->isPointerToCFIUncheckedCalleeFunctionOrMemberFunction(
+ Context)) {
+ Diag(IC->getExprLoc(), diag::warn_cast_discards_cfi_unchecked_callee)
+ << IC->getType() << LHSType;
+ }
+ }
+
// C99 6.5.16.1p2: The value of the right operand is converted to the
// type of the assignment expression.
// CheckAssignmentConstraints allows the left-hand side to be a reference,
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index dfb5824a1c3d7..d79c8b0410ed9 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -4619,6 +4619,16 @@ Sema::PerformImplicitConversion(Expr *From, QualType ToType,
return ExprError();
}
+ if (CCK == CheckedConversionKind::Implicit) {
+ if (From->getType()->isPointerToCFIUncheckedCalleeFunctionOrMemberFunction(
+ Context) &&
+ !ToType->isPointerToCFIUncheckedCalleeFunctionOrMemberFunction(
+ Context)) {
+ Diag(From->getExprLoc(), diag::warn_cast_discards_cfi_unchecked_callee)
+ << From->getType() << ToType;
+ }
+ }
+
// Everything went well.
return From;
}
@@ -7920,10 +7930,25 @@ QualType Sema::FindCompositePointerType(SourceLocation Loc,
EPI1.ExceptionSpec, EPI2.ExceptionSpec, ExceptionTypeStorage,
getLangOpts().CPlusPlus17);
+ // `cfi_unchecked_callee` needs to be preserved to prevent diagnosing on
+ // implicit casts when finding common types for binary operations. If
+ // one of the operands has the attribute, let's make the common type
+ // have it also.
+ bool HasCFIUncheckedCallee =
+ Composite1->hasCFIUncheckedCallee(Context) ||
+ Composite2->hasCFIUncheckedCallee(Context);
+
Composite1 = Context.getFunctionType(FPT1->getReturnType(),
FPT1->getParamTypes(), EPI1);
Composite2 = Context.getFunctionType(FPT2->getReturnType(),
FPT2->getParamTypes(), EPI2);
+
+ if (HasCFIUncheckedCallee) {
+ Composite1 = Context.getAttributedType(attr::CFIUncheckedCallee,
+ Composite1, Composite1);
+ Composite2 = Context.getAttributedType(attr::CFIUncheckedCallee,
+ Composite2, Composite2);
+ }
}
}
}
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index a1e4bb4321d53..b75eba41306b9 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -7936,7 +7936,7 @@ ExprResult InitializationSequence::Perform(Sema &S,
break;
}
- case SK_BindReference:
+ case SK_BindReference: {
// Reference binding does not have any corresponding ASTs.
// Check exception specifications
@@ -7957,7 +7957,21 @@ ExprResult InitializationSequence::Perform(Sema &S,
}
CheckForNullPointerDereference(S, CurInit.get());
+
+ QualType InitTy = CurInit.get()->getType();
+ const ASTContext &Ctx = S.Context;
+ bool DiscardingCFIUnchecked =
+ InitTy->isPointerToCFIUncheckedCalleeFunctionOrMemberFunction(Ctx) &&
+ !DestType->isPointerToCFIUncheckedCalleeFunctionOrMemberFunction(Ctx);
+ DiscardingCFIUnchecked |= InitTy->hasCFIUncheckedCallee(Ctx) &&
+ !DestType->hasCFIUncheckedCallee(Ctx);
+ if (DiscardingCFIUnchecked) {
+ S.Diag(CurInit.get()->getExprLoc(),
+ diag::warn_cast_discards_cfi_unchecked_callee)
+ << CurInit.get()->getType() << DestType;
+ }
break;
+ }
case SK_BindReferenceToTemporary: {
// Make sure the "temporary" is actually an rvalue.
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 33b1d8ca4dfa0..6ccd8761fbd45 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8815,6 +8815,19 @@ static void processTypeAttrs(TypeProcessingState &state, QualType &type,
break;
}
+ case ParsedAttr::AT_CFIUncheckedCallee: {
+ if (!type->isFunctionType() && !type->isDependentType()) {
+ state.getSema().Diag(attr.getLoc(),
+ diag::warn_cfi_unchecked_callee_on_non_function)
+ << type;
+ }
+ ASTContext &Ctx = state.getSema().Context;
+ type = state.getAttributedType(
+ createSimpleAttr<CFIUncheckedCalleeAttr>(Ctx, attr), type, type);
+ attr.setUsedAsTypeAttr();
+ break;
+ }
+
case ParsedAttr::AT_MatrixType:
HandleMatrixTypeAttr(type, attr, state.getSema());
attr.setUsedAsTypeAttr();
diff --git a/clang/test/CodeGen/cfi-unchecked-callee-attribute-member-function.cpp b/clang/test/CodeGen/cfi-unchecked-callee-attribute-member-function.cpp
new file mode 100644
index 0000000000000..637b5201ad6b4
--- /dev/null
+++ b/clang/test/CodeGen/cfi-unchecked-callee-attribute-member-function.cpp
@@ ...
[truncated]
|
|
@erichkeane We wanted C++ support so I tried adding exhaustive tests but I'm not sure if I caught everything. Would you be able to give a pass and see if I missed anything and what else should be tested? Also I'm having trouble coming up with a "struct redefinition" test. I think I'm misinterpreting N3037, but that looks like it has to do with type compatibility between different TUs rather than types within the same TU? |
erichkeane
reviewed
Apr 15, 2025
There was a problem hiding this comment.
I'm concerned about the approach of trying to leave this as AttributedType, as that is going to result in some particularly sharp edges to this. Consider:
using WithAttr = __attribute__((cfi_unchecked_callee)) BaseType;
using WithoutAttr = __attribute__((cfi_unchecked_callee)) BaseType;
using MyType = WithAttr;
using MyType = WithoutAttr; // still valid, since the attribute hasn't changed the type.
The diagnostic-inconsistency here mixed with the strange-never-before-an-odr-like-violation odr-like-violation is going to be extra weird.
This, IMO, needs its own AST type, with its own proper conversion rules.
@AaronBallman for his opinion if it differs.
| // attributed types. | ||
| QualType Ty(this, /*Quals=*/0); | ||
|
|
||
| while (1) { |
There was a problem hiding this comment.
I think I'd rather this do a bit of a while(!Ty.isCanonical()) instead.
| if (Ty->hasAttr(attr::CFIUncheckedCallee)) | ||
| return true; | ||
|
|
||
| QualType Desugared = Ty.getSingleStepDesugaredType(Context); |
There was a problem hiding this comment.
The fact that we're doing this I think means we're not properly adding this as a type, and I'm against it. I believe we should be adding a new type to the AST to represent this.
| @@ -8627,6 +8636,40 @@ inline bool Type::isOverloadableType() const { | |||
| !isMemberPointerType(); | |||
| } | |||
|
|
|||
| inline bool Type::hasCFIUncheckedCallee(const ASTContext &Context) const { | |||
There was a problem hiding this comment.
This function name doesn't really make sense either, It isnt' really checking the current type's callee, just the current type?
N3037 allows redefinition of the same struct within the same TU. We only recently ahve an implementation of it (not sure if it has been merged yet). I did a quick look, I didn't see anything about variable-type-aliases, static member functions, or explicit-this-type member functions, operators, etc. |
|
I also think this needs to be a real type. (Probably you can just add a bit to FunctionTypeExtraBitfields.) |
ilovepi
reviewed
Apr 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This introduces the attribute discussed in
https://discourse.llvm.org/t/rfc-function-type-attribute-to-prevent-cfi-instrumentation/85458.
The proposed name has been changed from
no_cfitocfi_unchecked_calleeto help differentiate fromno_sanitize("cfi")more easily. The proposed attribute has the following semantics: