[Clang] add wraps and no_wraps attributes #115094
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen Author: Justin Stitt (JustinStitt) Changes> [!NOTE] Implement These attributes serve as convenient in-source annotations for the expected overflow behavior of code. In addition, these attributes also have functional changes when used alongside certain sanitizers or warnings. The This is most useful for users that want to enable overflow or truncation sanitizers but also have some code they want to not be instrumented. Without this PR, they can only achieve translation unit level or at best function level granularity without changing arithmetic expressions directly. In an effort to not make basic arithmetic rely on wrappers like Examplevoid foo(int __attribute__((wraps)) A) {
++A; // No overflow/truncation sanitizers will instrument this code
}no_wraps
What makes typedef int __attribute__((no_wraps)) non_wrapping_int;
void bar(int A, non_wrapping_int B) {
++A; // will not have instrumentation (ignored within the ignorelist)
++B; // will have instrumentation (ignored within ignorelist, but overridden by no_wraps)
}CC@vitalybuka @kees @melver @bwendling Patch is 44.62 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/115094.diff 23 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index dc45202f6b2e86..c18410e6544081 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -386,6 +386,27 @@ Attribute Changes in Clang
- Fix a bug where clang doesn't automatically apply the ``[[gsl::Owner]]`` or
``[[gsl::Pointer]]`` to STL explicit template specialization decls. (#GH109442)
+- Introduced ``__attribute__((wraps))`` which can be added to type or variable
+ declarations. Using an attributed type or variable in an arithmetic
+ expression will define the overflow behavior for that expression as having
+ two's complement wrap-around. These expressions will not be instrumented by
+ overflow sanitizers nor will they cause integer overflow warnings. They also
+ cannot be optimized away by some eager UB optimizations as the behavior of
+ the arithmetic is no longer "undefined".
+
+ There is also ``__attribute__((no_wraps))`` which can be added to types or
+ variable declarations. Types or variables with this attribute may be
+ instrumented by overflow sanitizers, if enabled. Note that this matches the
+ default behavior of integer types. So, in most cases, ``no_wraps`` serves
+ purely as an annotation to readers of code that a type or variable really
+ shouldn't wrap-around. ``__attribute__((no_wraps))`` has the most function
+ when paired with `Sanitizer Special Case Lists (SSCL)
+ <https://clang.llvm.org/docs/SanitizerSpecialCaseList.html>`_.
+
+
+ These attributes are only valid for C, as there are built-in language
+ alternatives for other languages.
+
Improvements to Clang's diagnostics
-----------------------------------
diff --git a/clang/docs/SanitizerSpecialCaseList.rst b/clang/docs/SanitizerSpecialCaseList.rst
index 96a7b2fba4ae43..10462643b69c7f 100644
--- a/clang/docs/SanitizerSpecialCaseList.rst
+++ b/clang/docs/SanitizerSpecialCaseList.rst
@@ -67,9 +67,11 @@ types specified within an ignorelist.
int a = 2147483647; // INT_MAX
++a; // Normally, an overflow with -fsanitize=signed-integer-overflow
}
+
$ cat ignorelist.txt
[signed-integer-overflow]
type:int
+
$ clang -fsanitize=signed-integer-overflow -fsanitize-ignorelist=ignorelist.txt foo.c ; ./a.out
# no signed-integer-overflow error
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 466c65a9685ad3..4472c941ed5c79 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -4142,6 +4142,12 @@ class BinaryOperator : public Expr {
return getFPFeaturesInEffect(LO).getAllowFEnvAccess();
}
+ /// Does one of the subexpressions have the wraps attribute?
+ bool hasWrappingOperand(const ASTContext &Ctx) const;
+
+ /// How about the no_wraps attribute?
+ bool hasNonWrappingOperand(const ASTContext &Ctx) const;
+
protected:
BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs, Opcode opc,
QualType ResTy, ExprValueKind VK, ExprObjectKind OK,
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 1bcc7ee0b70dee..c7f368c0193664 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -1458,6 +1458,9 @@ class QualType {
return getQualifiers().hasStrongOrWeakObjCLifetime();
}
+ bool hasWrapsAttr() const;
+ bool hasNoWrapsAttr() const;
+
// true when Type is objc's weak and weak is enabled but ARC isn't.
bool isNonWeakInMRRWithObjCWeak(const ASTContext &Context) const;
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 156fbd1c4442eb..134395abf6e8f8 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4838,3 +4838,18 @@ def ClspvLibclcBuiltin: InheritableAttr {
let Documentation = [ClspvLibclcBuiltinDoc];
let SimpleHandler = 1;
}
+
+def Wraps : DeclOrTypeAttr {
+ let Spellings = [Clang<"wraps">];
+ let Subjects = SubjectList<[Var, TypedefName, Field]>;
+ let Documentation = [WrapsDocs];
+ let LangOpts = [COnly];
+}
+
+def NoWraps : DeclOrTypeAttr {
+ let Spellings = [Clang<"no_wraps">];
+ let Subjects = SubjectList<[Var, TypedefName, Field]>;
+ let Documentation = [NoWrapsDocs];
+ let LangOpts = [COnly];
+}
+
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index b497cce37625c9..5d9f77b05ee96d 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -8526,3 +8526,103 @@ Declares that a function potentially allocates heap memory, and prevents any pot
of ``nonallocating`` by the compiler.
}];
}
+
+def WrapsDocs : Documentation {
+ let Category = DocCatField;
+ let Content = [{
+The ``wraps`` attribute can be used with type or variable declarations to
+denote that arithmetic containing attributed types or variables have defined
+overflow behavior. Specifically, the behavior is defined as being consistent
+with two's complement wrap-around. For the purposes of sanitizers or warnings
+that concern themselves with the definedness of integer arithmetic, they will
+cease to instrument or warn about arithmetic that directly involves operands
+attributed with the ``wraps`` attribute.
+
+The ``signed-integer-overflow``, ``unsigned-integer-overflow``,
+``implicit-signed-integer-truncation`` and the
+``implicit-unsigned-integer-truncation`` sanitizers will not instrument
+arithmetic containing any operands attributed by ``wraps``. Similarly, the
+``-Winteger-overflow`` warning is disabled for these instances.
+
+The following example shows how one may disable ``signed-integer-overflow``
+sanitizer instrumentation using ``__attribute__((wraps))`` on a type definition
+when building with ``-fsanitize=signed-integer-overflow``:
+
+.. code-block:: c
+
+ typedef int __attribute__((wraps)) wrapping_int;
+
+ void foo(void) {
+ wrapping_int A = INT_MAX;
+ ++A; // no sanitizer instrumentation
+ }
+
+``wraps`` may also be used with function parameters or declarations of
+variables as well as members of structures. Using ``wraps`` on non-integer
+types will result in a `-Wuseless-wraps-attribute`. One may disable this
+warning with ``-Wno-useless-wraps-attribute``.
+
+``wraps`` persists through implicit type promotions and will be applied to the
+result type of arithmetic expressions containing a wrapping operand.
+``-Wimplicitly-discarded-wraps-attribute`` warnings can be caused in situations
+where the ``wraps`` attribute cannot persist through implicit type conversions.
+Disable this with ``-Wno-implicitly-discarded-wraps-attribute``.
+}];
+}
+
+def NoWrapsDocs : Documentation {
+ let Category = DocCatField;
+ let Content = [{
+The ``no_wraps`` attribute can be used to annotate types or variables as
+non-wrapping. This may serve as a helpful annotation to readers of code that
+particular arithmetic expressions involving these types or variables are not
+meant to wrap-around.
+
+When overflow or truncation sanitizer instrumentation is modified at the
+type-level through `SSCLs
+<https://clang.llvm.org/docs/SanitizerSpecialCaseList.html>`_, ``no_wraps`` or
+``wraps`` may be used to override sanitizer behavior.
+
+For example, one may specify an ignorelist (with ``-fsanitize-ignorelist=``) to
+disable the ``signed-integer-overflow`` sanitizer for all types:
+
+.. code-block:: text
+
+ [signed-integer-overflow]
+ type:*
+
+``no_wraps`` can override the behavior provided by the ignorelist to
+effectively re-enable instrumentation for specific types or variables.
+
+.. code-block:: c
+
+ typedef int __attribute__((no_wraps)) non_wrapping_int;
+
+ void foo(non_wrapping_int A, int B) {
+ ++A; // will be instrumented if built with -fsanitize=signed-integer-overflow
+ ++B; // won't be instrumented as it is ignored by the ignorelist
+ }
+
+Like ``wraps``, ``no_wraps`` persists through implicit type promotions and will
+be automatically applied to the result type of arithmetic expressions
+containing a wrapping operand.
+
+If a type or variable is attributed by both ``wraps`` and ``no_wraps``, then
+``no_wraps`` takes precedence -- regardless of the order of attribution.
+
+Note that ``no_wraps`` makes no guarantees about the definedness of arithmetic
+overflow. Instead, use ``-fwrapv`` or ``-fno-strict-overflow``.
+
+Like ``wraps``, ``no_wraps`` may also be used with function parameters or
+declarations of variables as well as members of structures. Using ``wraps`` on
+non-integer types will result in a `-Wuseless-wraps-attribute`. One may disable
+this warning with ``-Wno-useless-wraps-attribute``.
+
+``no_wraps`` also persists through implicit type promotions and will be applied
+to the result type of arithmetic expressions containing a wrapping operand.
+``-Wimplicitly-discarded-wraps-attribute`` warnings can be caused in situations
+where the ``wraps`` attribute cannot persist through implicit type conversions.
+Disable this with ``-Wno-implicitly-discarded-wraps-attribute``.
+}];
+}
+
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 72eada50a56cc9..0c4d0fa5528047 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1588,3 +1588,9 @@ def ExplicitSpecializationStorageClass : DiagGroup<"explicit-specialization-stor
// A warning for options that enable a feature that is not yet complete
def ExperimentalOption : DiagGroup<"experimental-option">;
+
+// Warnings regarding the usage of __attribute__((wraps)) on non-integer types.
+def UselessWrapsAttr : DiagGroup<"useless-wraps-attribute">;
+
+// Warnings about the wraps attribute getting implicitly discarded
+def ImpDiscardedWrapsAttr : DiagGroup<"implicitly-discarded-wraps-attribute">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d697e6d61afa9a..84d62d4fcb561b 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6652,6 +6652,13 @@ def warn_counted_by_attr_elt_type_unknown_size :
Warning<err_counted_by_attr_pointee_unknown_size.Summary>,
InGroup<BoundsSafetyCountedByEltTyUnknownSize>;
+def warn_wraps_attr_var_decl_type_not_integer : Warning<
+ "using attribute '%select{wraps|no_wraps}0' with non-integer type '%1' has no function and is potentially misleading">,
+ InGroup<UselessWrapsAttr>;
+def warn_wraps_attr_maybe_lost : Warning<
+ "'%select{wraps|no_wraps}0' attribute may be implicitly discarded when converted to %1">,
+ InGroup<ImpDiscardedWrapsAttr>;
+
let CategoryName = "ARC Semantic Issue" in {
// ARC-mode diagnostics.
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index bf2c1b92fa6b49..7de87039cc95c2 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -2236,6 +2236,16 @@ bool BinaryOperator::isNullPointerArithmeticExtension(ASTContext &Ctx,
return true;
}
+bool BinaryOperator::hasWrappingOperand(const ASTContext &Ctx) const {
+ return getLHS()->getType().hasWrapsAttr() ||
+ getRHS()->getType().hasWrapsAttr();
+}
+
+bool BinaryOperator::hasNonWrappingOperand(const ASTContext &Ctx) const {
+ return getLHS()->getType().hasNoWrapsAttr() ||
+ getRHS()->getType().hasNoWrapsAttr();
+}
+
SourceLocExpr::SourceLocExpr(const ASTContext &Ctx, SourceLocIdentKind Kind,
QualType ResultTy, SourceLocation BLoc,
SourceLocation RParenLoc,
@@ -4852,6 +4862,11 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
if (hasStoredFPFeatures())
setStoredFPFeatures(FPFeatures);
setDependence(computeDependence(this));
+ if (hasWrappingOperand(Ctx))
+ setType(Ctx.getAttributedType(attr::Wraps, getType(), getType()));
+ if (hasNonWrappingOperand(Ctx))
+ setType(Ctx.getAttributedType(attr::NoWraps, getType(), getType()));
+
}
BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
@@ -4870,6 +4885,11 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
if (hasStoredFPFeatures())
setStoredFPFeatures(FPFeatures);
setDependence(computeDependence(this));
+ if (hasWrappingOperand(Ctx))
+ setType(Ctx.getAttributedType(attr::Wraps, getType(), getType()));
+ if (hasNonWrappingOperand(Ctx))
+ setType(Ctx.getAttributedType(attr::NoWraps, getType(), getType()));
+
}
BinaryOperator *BinaryOperator::CreateEmpty(const ASTContext &C,
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index d664c503655ba6..f2758f5d6c7f35 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -2898,7 +2898,7 @@ static bool CheckedIntArithmetic(EvalInfo &Info, const Expr *E,
APSInt Value(Op(LHS.extend(BitWidth), RHS.extend(BitWidth)), false);
Result = Value.trunc(LHS.getBitWidth());
if (Result.extend(BitWidth) != Value) {
- if (Info.checkingForUndefinedBehavior())
+ if (Info.checkingForUndefinedBehavior() && !E->getType().hasWrapsAttr())
Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
diag::warn_integer_constant_overflow)
<< toString(Result, 10, Result.isSigned(), /*formatAsCLiteral=*/false,
@@ -14694,7 +14694,7 @@ bool IntExprEvaluator::VisitUnaryOperator(const UnaryOperator *E) {
if (!Result.isInt()) return Error(E);
const APSInt &Value = Result.getInt();
if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow()) {
- if (Info.checkingForUndefinedBehavior())
+ if (Info.checkingForUndefinedBehavior() && !E->getType().hasWrapsAttr())
Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
diag::warn_integer_constant_overflow)
<< toString(Value, 10, Value.isSigned(), /*formatAsCLiteral=*/false,
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 6bf2908e667c07..a6348a0f406263 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -2863,6 +2863,15 @@ bool QualType::isWebAssemblyFuncrefType() const {
getAddressSpace() == LangAS::wasm_funcref;
}
+bool QualType::hasWrapsAttr() const {
+ return !isNull() && getTypePtr()->hasAttr(attr::Wraps) &&
+ !getTypePtr()->hasAttr(attr::NoWraps);
+}
+
+bool QualType::hasNoWrapsAttr() const {
+ return !isNull() && getTypePtr()->hasAttr(attr::NoWraps);
+}
+
QualType::PrimitiveDefaultInitializeKind
QualType::isNonTrivialToPrimitiveDefaultInitialize() const {
if (const auto *RT =
diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index 6d8db5cf4ffd22..9825abcade9afc 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -2037,6 +2037,12 @@ void TypePrinter::printAttributedAfter(const AttributedType *T,
case attr::AArch64SVEPcs: OS << "aarch64_sve_pcs"; break;
case attr::AMDGPUKernelCall: OS << "amdgpu_kernel"; break;
case attr::IntelOclBicc: OS << "inteloclbicc"; break;
+ case attr::Wraps:
+ OS << "wraps";
+ break;
+ case attr::NoWraps:
+ OS << "no_wraps";
+ break;
case attr::PreserveMost:
OS << "preserve_most";
break;
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 287d911e10ba58..a4efb0cd5da010 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -158,6 +158,12 @@ struct BinOpInfo {
}
return false;
}
+
+ /// Does the BinaryOperator have the wraps attribute?
+ /// If so, we can elide overflow sanitizer checks.
+ bool hasWrappingOperand() const {
+ return E->getType().hasWrapsAttr() && !E->getType().hasNoWrapsAttr();
+ }
};
static bool MustVisitNullValue(const Expr *E) {
@@ -197,13 +203,13 @@ static bool CanElideOverflowCheck(const ASTContext &Ctx, const BinOpInfo &Op) {
if (!Op.mayHaveIntegerOverflow())
return true;
- if (Op.Ty->isSignedIntegerType() &&
+ if (Op.Ty->isSignedIntegerType() && !Op.Ty.hasNoWrapsAttr() &&
Ctx.isTypeIgnoredBySanitizer(SanitizerKind::SignedIntegerOverflow,
Op.Ty)) {
return true;
}
- if (Op.Ty->isUnsignedIntegerType() &&
+ if (Op.Ty->isUnsignedIntegerType() && !Op.Ty.hasNoWrapsAttr() &&
Ctx.isTypeIgnoredBySanitizer(SanitizerKind::UnsignedIntegerOverflow,
Op.Ty)) {
return true;
@@ -766,7 +772,8 @@ class ScalarExprEmitter
// Binary Operators.
Value *EmitMul(const BinOpInfo &Ops) {
- if (Ops.Ty->isSignedIntegerOrEnumerationType()) {
+ if (Ops.Ty->isSignedIntegerOrEnumerationType() &&
+ !Ops.hasWrappingOperand()) {
switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
case LangOptions::SOB_Defined:
if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
@@ -802,7 +809,8 @@ class ScalarExprEmitter
if (Ops.Ty->isUnsignedIntegerType() &&
CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) &&
- !CanElideOverflowCheck(CGF.getContext(), Ops))
+ !CanElideOverflowCheck(CGF.getContext(), Ops) &&
+ !Ops.hasWrappingOperand())
return EmitOverflowCheckedBinOp(Ops);
if (Ops.LHS->getType()->isFPOrFPVectorTy()) {
@@ -1134,7 +1142,7 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value *Src, QualType SrcType,
// If the comparison result is 'i1 false', then the truncation was lossy.
// Do we care about this type of truncation?
- if (!CGF.SanOpts.has(Check.second.second))
+ if (!CGF.SanOpts.has(Check.second.second) || DstType.hasWrapsAttr())
return;
// Does some SSCL ignore this type?
@@ -1380,6 +1388,11 @@ void CodeGenFunction::EmitBitfieldConversionCheck(Value *Src, QualType SrcType,
bool SrcSigned = SrcType->isSignedIntegerOrEnumerationType();
bool DstSigned = DstType->isSignedIntegerOrEnumerationType();
+ // The wraps attribute will silence any sanitizer warnings
+ // regarding truncation or overflow
+ if (SrcType.hasWrapsAttr() || DstType.hasWrapsAttr())
+ return;
+
CodeGenFunction::SanitizerScope SanScope(this);
std::pair<ScalarExprEmitter::ImplicitConversionCheckKind,
@@ -2956,6 +2969,9 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,
bool excludeOverflowPattern =
matchesPostDecrInWhile(E, isInc, isPre, CGF.getContext());
+ BinOpInfo Ops = createBinOpInfoFromIncDec(
+ E, value, isInc, E->getFPFeaturesInEffect(CGF.getLangOpts()));
+
if (CGF.getContext().isPromotableIntegerType(type)) {
promotedType = CGF.getContext().getPromotedIntegerType(type);
assert(promotedType != type && "Shouldn't promote to the same type.");
@@ -3012,10 +3028,12 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,
// Note that signed integer inc/dec with width less than int can't
// overflow because of promotion rules; we're just eliding a few steps
// here.
- } else if (E->canOverflow() && type->isSignedIntegerOrEnumerationType()) {
+ } else if (E->canOverflow() && type->isSignedIntegerOrEnumerationType() &&
+ !Ops.hasWrappingOperand()) {
value = EmitIncDecConsiderOverflowBehavior(E, value, isInc);
} else if (E->canOverflow() && type->isUnsignedIntegerType() &&
CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) &&
+ !Ops.hasWrappingOperand() &&
!excludeOverflowPattern &&
!CGF.getContext().isTypeIgnoredBySanitizer(
SanitizerKind::UnsignedIntegerOverflow, E->getType())) {
@@ -3807,7 +3825,8 @@ Value *ScalarExprEmitter::EmitDiv(const BinOpInfo &Ops) {
if ((CGF.SanOpts.has(SanitizerKind::IntegerDivideByZero) ||
CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow)) &&
Ops.Ty->isIntegerType() &&
- (Ops.mayHaveIntegerDivisionByZero() || Ops.mayHaveIntegerOverflow())) {
+ (Ops.mayHaveIntegerDivisionByZero() || Ops.mayHaveIntegerOverflow()) &&
+ !Ops.hasWrappingOperand()) {
llvm::Value *Zero = llvm::Constant::getNullValue(ConvertType(Ops.Ty));
EmitUndefinedBehaviorIntegerDivAndRemCheck(Ops, Zero, true);
} else if (CGF.SanOpts.has(SanitizerKind::FloatDivideByZero) &&
@@ -3856,7 +3875,8 @@ Value *ScalarExprEmitter::EmitRem(const BinOpInfo &Ops) {
if ((CGF.SanOpts.has(SanitizerKind::Inte...
[truncated]
|
|
@llvm/pr-subscribers-clang Author: Justin Stitt (JustinStitt) Changes> [!NOTE] Implement These attributes serve as convenient in-source annotations for the expected overflow behavior of code. In addition, these attributes also have functional changes when used alongside certain sanitizers or warnings. The This is most useful for users that want to enable overflow or truncation sanitizers but also have some code they want to not be instrumented. Without this PR, they can only achieve translation unit level or at best function level granularity without changing arithmetic expressions directly. In an effort to not make basic arithmetic rely on wrappers like Examplevoid foo(int __attribute__((wraps)) A) {
++A; // No overflow/truncation sanitizers will instrument this code
}no_wraps
What makes typedef int __attribute__((no_wraps)) non_wrapping_int;
void bar(int A, non_wrapping_int B) {
++A; // will not have instrumentation (ignored within the ignorelist)
++B; // will have instrumentation (ignored within ignorelist, but overridden by no_wraps)
}CC@vitalybuka @kees @melver @bwendling Patch is 44.62 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/115094.diff 23 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index dc45202f6b2e86..c18410e6544081 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -386,6 +386,27 @@ Attribute Changes in Clang
- Fix a bug where clang doesn't automatically apply the ``[[gsl::Owner]]`` or
``[[gsl::Pointer]]`` to STL explicit template specialization decls. (#GH109442)
+- Introduced ``__attribute__((wraps))`` which can be added to type or variable
+ declarations. Using an attributed type or variable in an arithmetic
+ expression will define the overflow behavior for that expression as having
+ two's complement wrap-around. These expressions will not be instrumented by
+ overflow sanitizers nor will they cause integer overflow warnings. They also
+ cannot be optimized away by some eager UB optimizations as the behavior of
+ the arithmetic is no longer "undefined".
+
+ There is also ``__attribute__((no_wraps))`` which can be added to types or
+ variable declarations. Types or variables with this attribute may be
+ instrumented by overflow sanitizers, if enabled. Note that this matches the
+ default behavior of integer types. So, in most cases, ``no_wraps`` serves
+ purely as an annotation to readers of code that a type or variable really
+ shouldn't wrap-around. ``__attribute__((no_wraps))`` has the most function
+ when paired with `Sanitizer Special Case Lists (SSCL)
+ <https://clang.llvm.org/docs/SanitizerSpecialCaseList.html>`_.
+
+
+ These attributes are only valid for C, as there are built-in language
+ alternatives for other languages.
+
Improvements to Clang's diagnostics
-----------------------------------
diff --git a/clang/docs/SanitizerSpecialCaseList.rst b/clang/docs/SanitizerSpecialCaseList.rst
index 96a7b2fba4ae43..10462643b69c7f 100644
--- a/clang/docs/SanitizerSpecialCaseList.rst
+++ b/clang/docs/SanitizerSpecialCaseList.rst
@@ -67,9 +67,11 @@ types specified within an ignorelist.
int a = 2147483647; // INT_MAX
++a; // Normally, an overflow with -fsanitize=signed-integer-overflow
}
+
$ cat ignorelist.txt
[signed-integer-overflow]
type:int
+
$ clang -fsanitize=signed-integer-overflow -fsanitize-ignorelist=ignorelist.txt foo.c ; ./a.out
# no signed-integer-overflow error
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 466c65a9685ad3..4472c941ed5c79 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -4142,6 +4142,12 @@ class BinaryOperator : public Expr {
return getFPFeaturesInEffect(LO).getAllowFEnvAccess();
}
+ /// Does one of the subexpressions have the wraps attribute?
+ bool hasWrappingOperand(const ASTContext &Ctx) const;
+
+ /// How about the no_wraps attribute?
+ bool hasNonWrappingOperand(const ASTContext &Ctx) const;
+
protected:
BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs, Opcode opc,
QualType ResTy, ExprValueKind VK, ExprObjectKind OK,
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 1bcc7ee0b70dee..c7f368c0193664 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -1458,6 +1458,9 @@ class QualType {
return getQualifiers().hasStrongOrWeakObjCLifetime();
}
+ bool hasWrapsAttr() const;
+ bool hasNoWrapsAttr() const;
+
// true when Type is objc's weak and weak is enabled but ARC isn't.
bool isNonWeakInMRRWithObjCWeak(const ASTContext &Context) const;
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 156fbd1c4442eb..134395abf6e8f8 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4838,3 +4838,18 @@ def ClspvLibclcBuiltin: InheritableAttr {
let Documentation = [ClspvLibclcBuiltinDoc];
let SimpleHandler = 1;
}
+
+def Wraps : DeclOrTypeAttr {
+ let Spellings = [Clang<"wraps">];
+ let Subjects = SubjectList<[Var, TypedefName, Field]>;
+ let Documentation = [WrapsDocs];
+ let LangOpts = [COnly];
+}
+
+def NoWraps : DeclOrTypeAttr {
+ let Spellings = [Clang<"no_wraps">];
+ let Subjects = SubjectList<[Var, TypedefName, Field]>;
+ let Documentation = [NoWrapsDocs];
+ let LangOpts = [COnly];
+}
+
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index b497cce37625c9..5d9f77b05ee96d 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -8526,3 +8526,103 @@ Declares that a function potentially allocates heap memory, and prevents any pot
of ``nonallocating`` by the compiler.
}];
}
+
+def WrapsDocs : Documentation {
+ let Category = DocCatField;
+ let Content = [{
+The ``wraps`` attribute can be used with type or variable declarations to
+denote that arithmetic containing attributed types or variables have defined
+overflow behavior. Specifically, the behavior is defined as being consistent
+with two's complement wrap-around. For the purposes of sanitizers or warnings
+that concern themselves with the definedness of integer arithmetic, they will
+cease to instrument or warn about arithmetic that directly involves operands
+attributed with the ``wraps`` attribute.
+
+The ``signed-integer-overflow``, ``unsigned-integer-overflow``,
+``implicit-signed-integer-truncation`` and the
+``implicit-unsigned-integer-truncation`` sanitizers will not instrument
+arithmetic containing any operands attributed by ``wraps``. Similarly, the
+``-Winteger-overflow`` warning is disabled for these instances.
+
+The following example shows how one may disable ``signed-integer-overflow``
+sanitizer instrumentation using ``__attribute__((wraps))`` on a type definition
+when building with ``-fsanitize=signed-integer-overflow``:
+
+.. code-block:: c
+
+ typedef int __attribute__((wraps)) wrapping_int;
+
+ void foo(void) {
+ wrapping_int A = INT_MAX;
+ ++A; // no sanitizer instrumentation
+ }
+
+``wraps`` may also be used with function parameters or declarations of
+variables as well as members of structures. Using ``wraps`` on non-integer
+types will result in a `-Wuseless-wraps-attribute`. One may disable this
+warning with ``-Wno-useless-wraps-attribute``.
+
+``wraps`` persists through implicit type promotions and will be applied to the
+result type of arithmetic expressions containing a wrapping operand.
+``-Wimplicitly-discarded-wraps-attribute`` warnings can be caused in situations
+where the ``wraps`` attribute cannot persist through implicit type conversions.
+Disable this with ``-Wno-implicitly-discarded-wraps-attribute``.
+}];
+}
+
+def NoWrapsDocs : Documentation {
+ let Category = DocCatField;
+ let Content = [{
+The ``no_wraps`` attribute can be used to annotate types or variables as
+non-wrapping. This may serve as a helpful annotation to readers of code that
+particular arithmetic expressions involving these types or variables are not
+meant to wrap-around.
+
+When overflow or truncation sanitizer instrumentation is modified at the
+type-level through `SSCLs
+<https://clang.llvm.org/docs/SanitizerSpecialCaseList.html>`_, ``no_wraps`` or
+``wraps`` may be used to override sanitizer behavior.
+
+For example, one may specify an ignorelist (with ``-fsanitize-ignorelist=``) to
+disable the ``signed-integer-overflow`` sanitizer for all types:
+
+.. code-block:: text
+
+ [signed-integer-overflow]
+ type:*
+
+``no_wraps`` can override the behavior provided by the ignorelist to
+effectively re-enable instrumentation for specific types or variables.
+
+.. code-block:: c
+
+ typedef int __attribute__((no_wraps)) non_wrapping_int;
+
+ void foo(non_wrapping_int A, int B) {
+ ++A; // will be instrumented if built with -fsanitize=signed-integer-overflow
+ ++B; // won't be instrumented as it is ignored by the ignorelist
+ }
+
+Like ``wraps``, ``no_wraps`` persists through implicit type promotions and will
+be automatically applied to the result type of arithmetic expressions
+containing a wrapping operand.
+
+If a type or variable is attributed by both ``wraps`` and ``no_wraps``, then
+``no_wraps`` takes precedence -- regardless of the order of attribution.
+
+Note that ``no_wraps`` makes no guarantees about the definedness of arithmetic
+overflow. Instead, use ``-fwrapv`` or ``-fno-strict-overflow``.
+
+Like ``wraps``, ``no_wraps`` may also be used with function parameters or
+declarations of variables as well as members of structures. Using ``wraps`` on
+non-integer types will result in a `-Wuseless-wraps-attribute`. One may disable
+this warning with ``-Wno-useless-wraps-attribute``.
+
+``no_wraps`` also persists through implicit type promotions and will be applied
+to the result type of arithmetic expressions containing a wrapping operand.
+``-Wimplicitly-discarded-wraps-attribute`` warnings can be caused in situations
+where the ``wraps`` attribute cannot persist through implicit type conversions.
+Disable this with ``-Wno-implicitly-discarded-wraps-attribute``.
+}];
+}
+
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 72eada50a56cc9..0c4d0fa5528047 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1588,3 +1588,9 @@ def ExplicitSpecializationStorageClass : DiagGroup<"explicit-specialization-stor
// A warning for options that enable a feature that is not yet complete
def ExperimentalOption : DiagGroup<"experimental-option">;
+
+// Warnings regarding the usage of __attribute__((wraps)) on non-integer types.
+def UselessWrapsAttr : DiagGroup<"useless-wraps-attribute">;
+
+// Warnings about the wraps attribute getting implicitly discarded
+def ImpDiscardedWrapsAttr : DiagGroup<"implicitly-discarded-wraps-attribute">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d697e6d61afa9a..84d62d4fcb561b 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6652,6 +6652,13 @@ def warn_counted_by_attr_elt_type_unknown_size :
Warning<err_counted_by_attr_pointee_unknown_size.Summary>,
InGroup<BoundsSafetyCountedByEltTyUnknownSize>;
+def warn_wraps_attr_var_decl_type_not_integer : Warning<
+ "using attribute '%select{wraps|no_wraps}0' with non-integer type '%1' has no function and is potentially misleading">,
+ InGroup<UselessWrapsAttr>;
+def warn_wraps_attr_maybe_lost : Warning<
+ "'%select{wraps|no_wraps}0' attribute may be implicitly discarded when converted to %1">,
+ InGroup<ImpDiscardedWrapsAttr>;
+
let CategoryName = "ARC Semantic Issue" in {
// ARC-mode diagnostics.
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index bf2c1b92fa6b49..7de87039cc95c2 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -2236,6 +2236,16 @@ bool BinaryOperator::isNullPointerArithmeticExtension(ASTContext &Ctx,
return true;
}
+bool BinaryOperator::hasWrappingOperand(const ASTContext &Ctx) const {
+ return getLHS()->getType().hasWrapsAttr() ||
+ getRHS()->getType().hasWrapsAttr();
+}
+
+bool BinaryOperator::hasNonWrappingOperand(const ASTContext &Ctx) const {
+ return getLHS()->getType().hasNoWrapsAttr() ||
+ getRHS()->getType().hasNoWrapsAttr();
+}
+
SourceLocExpr::SourceLocExpr(const ASTContext &Ctx, SourceLocIdentKind Kind,
QualType ResultTy, SourceLocation BLoc,
SourceLocation RParenLoc,
@@ -4852,6 +4862,11 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
if (hasStoredFPFeatures())
setStoredFPFeatures(FPFeatures);
setDependence(computeDependence(this));
+ if (hasWrappingOperand(Ctx))
+ setType(Ctx.getAttributedType(attr::Wraps, getType(), getType()));
+ if (hasNonWrappingOperand(Ctx))
+ setType(Ctx.getAttributedType(attr::NoWraps, getType(), getType()));
+
}
BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
@@ -4870,6 +4885,11 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
if (hasStoredFPFeatures())
setStoredFPFeatures(FPFeatures);
setDependence(computeDependence(this));
+ if (hasWrappingOperand(Ctx))
+ setType(Ctx.getAttributedType(attr::Wraps, getType(), getType()));
+ if (hasNonWrappingOperand(Ctx))
+ setType(Ctx.getAttributedType(attr::NoWraps, getType(), getType()));
+
}
BinaryOperator *BinaryOperator::CreateEmpty(const ASTContext &C,
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index d664c503655ba6..f2758f5d6c7f35 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -2898,7 +2898,7 @@ static bool CheckedIntArithmetic(EvalInfo &Info, const Expr *E,
APSInt Value(Op(LHS.extend(BitWidth), RHS.extend(BitWidth)), false);
Result = Value.trunc(LHS.getBitWidth());
if (Result.extend(BitWidth) != Value) {
- if (Info.checkingForUndefinedBehavior())
+ if (Info.checkingForUndefinedBehavior() && !E->getType().hasWrapsAttr())
Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
diag::warn_integer_constant_overflow)
<< toString(Result, 10, Result.isSigned(), /*formatAsCLiteral=*/false,
@@ -14694,7 +14694,7 @@ bool IntExprEvaluator::VisitUnaryOperator(const UnaryOperator *E) {
if (!Result.isInt()) return Error(E);
const APSInt &Value = Result.getInt();
if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow()) {
- if (Info.checkingForUndefinedBehavior())
+ if (Info.checkingForUndefinedBehavior() && !E->getType().hasWrapsAttr())
Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
diag::warn_integer_constant_overflow)
<< toString(Value, 10, Value.isSigned(), /*formatAsCLiteral=*/false,
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 6bf2908e667c07..a6348a0f406263 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -2863,6 +2863,15 @@ bool QualType::isWebAssemblyFuncrefType() const {
getAddressSpace() == LangAS::wasm_funcref;
}
+bool QualType::hasWrapsAttr() const {
+ return !isNull() && getTypePtr()->hasAttr(attr::Wraps) &&
+ !getTypePtr()->hasAttr(attr::NoWraps);
+}
+
+bool QualType::hasNoWrapsAttr() const {
+ return !isNull() && getTypePtr()->hasAttr(attr::NoWraps);
+}
+
QualType::PrimitiveDefaultInitializeKind
QualType::isNonTrivialToPrimitiveDefaultInitialize() const {
if (const auto *RT =
diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index 6d8db5cf4ffd22..9825abcade9afc 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -2037,6 +2037,12 @@ void TypePrinter::printAttributedAfter(const AttributedType *T,
case attr::AArch64SVEPcs: OS << "aarch64_sve_pcs"; break;
case attr::AMDGPUKernelCall: OS << "amdgpu_kernel"; break;
case attr::IntelOclBicc: OS << "inteloclbicc"; break;
+ case attr::Wraps:
+ OS << "wraps";
+ break;
+ case attr::NoWraps:
+ OS << "no_wraps";
+ break;
case attr::PreserveMost:
OS << "preserve_most";
break;
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 287d911e10ba58..a4efb0cd5da010 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -158,6 +158,12 @@ struct BinOpInfo {
}
return false;
}
+
+ /// Does the BinaryOperator have the wraps attribute?
+ /// If so, we can elide overflow sanitizer checks.
+ bool hasWrappingOperand() const {
+ return E->getType().hasWrapsAttr() && !E->getType().hasNoWrapsAttr();
+ }
};
static bool MustVisitNullValue(const Expr *E) {
@@ -197,13 +203,13 @@ static bool CanElideOverflowCheck(const ASTContext &Ctx, const BinOpInfo &Op) {
if (!Op.mayHaveIntegerOverflow())
return true;
- if (Op.Ty->isSignedIntegerType() &&
+ if (Op.Ty->isSignedIntegerType() && !Op.Ty.hasNoWrapsAttr() &&
Ctx.isTypeIgnoredBySanitizer(SanitizerKind::SignedIntegerOverflow,
Op.Ty)) {
return true;
}
- if (Op.Ty->isUnsignedIntegerType() &&
+ if (Op.Ty->isUnsignedIntegerType() && !Op.Ty.hasNoWrapsAttr() &&
Ctx.isTypeIgnoredBySanitizer(SanitizerKind::UnsignedIntegerOverflow,
Op.Ty)) {
return true;
@@ -766,7 +772,8 @@ class ScalarExprEmitter
// Binary Operators.
Value *EmitMul(const BinOpInfo &Ops) {
- if (Ops.Ty->isSignedIntegerOrEnumerationType()) {
+ if (Ops.Ty->isSignedIntegerOrEnumerationType() &&
+ !Ops.hasWrappingOperand()) {
switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
case LangOptions::SOB_Defined:
if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
@@ -802,7 +809,8 @@ class ScalarExprEmitter
if (Ops.Ty->isUnsignedIntegerType() &&
CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) &&
- !CanElideOverflowCheck(CGF.getContext(), Ops))
+ !CanElideOverflowCheck(CGF.getContext(), Ops) &&
+ !Ops.hasWrappingOperand())
return EmitOverflowCheckedBinOp(Ops);
if (Ops.LHS->getType()->isFPOrFPVectorTy()) {
@@ -1134,7 +1142,7 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value *Src, QualType SrcType,
// If the comparison result is 'i1 false', then the truncation was lossy.
// Do we care about this type of truncation?
- if (!CGF.SanOpts.has(Check.second.second))
+ if (!CGF.SanOpts.has(Check.second.second) || DstType.hasWrapsAttr())
return;
// Does some SSCL ignore this type?
@@ -1380,6 +1388,11 @@ void CodeGenFunction::EmitBitfieldConversionCheck(Value *Src, QualType SrcType,
bool SrcSigned = SrcType->isSignedIntegerOrEnumerationType();
bool DstSigned = DstType->isSignedIntegerOrEnumerationType();
+ // The wraps attribute will silence any sanitizer warnings
+ // regarding truncation or overflow
+ if (SrcType.hasWrapsAttr() || DstType.hasWrapsAttr())
+ return;
+
CodeGenFunction::SanitizerScope SanScope(this);
std::pair<ScalarExprEmitter::ImplicitConversionCheckKind,
@@ -2956,6 +2969,9 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,
bool excludeOverflowPattern =
matchesPostDecrInWhile(E, isInc, isPre, CGF.getContext());
+ BinOpInfo Ops = createBinOpInfoFromIncDec(
+ E, value, isInc, E->getFPFeaturesInEffect(CGF.getLangOpts()));
+
if (CGF.getContext().isPromotableIntegerType(type)) {
promotedType = CGF.getContext().getPromotedIntegerType(type);
assert(promotedType != type && "Shouldn't promote to the same type.");
@@ -3012,10 +3028,12 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,
// Note that signed integer inc/dec with width less than int can't
// overflow because of promotion rules; we're just eliding a few steps
// here.
- } else if (E->canOverflow() && type->isSignedIntegerOrEnumerationType()) {
+ } else if (E->canOverflow() && type->isSignedIntegerOrEnumerationType() &&
+ !Ops.hasWrappingOperand()) {
value = EmitIncDecConsiderOverflowBehavior(E, value, isInc);
} else if (E->canOverflow() && type->isUnsignedIntegerType() &&
CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) &&
+ !Ops.hasWrappingOperand() &&
!excludeOverflowPattern &&
!CGF.getContext().isTypeIgnoredBySanitizer(
SanitizerKind::UnsignedIntegerOverflow, E->getType())) {
@@ -3807,7 +3825,8 @@ Value *ScalarExprEmitter::EmitDiv(const BinOpInfo &Ops) {
if ((CGF.SanOpts.has(SanitizerKind::IntegerDivideByZero) ||
CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow)) &&
Ops.Ty->isIntegerType() &&
- (Ops.mayHaveIntegerDivisionByZero() || Ops.mayHaveIntegerOverflow())) {
+ (Ops.mayHaveIntegerDivisionByZero() || Ops.mayHaveIntegerOverflow()) &&
+ !Ops.hasWrappingOperand()) {
llvm::Value *Zero = llvm::Constant::getNullValue(ConvertType(Ops.Ty));
EmitUndefinedBehaviorIntegerDivAndRemCheck(Ops, Zero, true);
} else if (CGF.SanOpts.has(SanitizerKind::FloatDivideByZero) &&
@@ -3856,7 +3875,8 @@ Value *ScalarExprEmitter::EmitRem(const BinOpInfo &Ops) {
if ((CGF.SanOpts.has(SanitizerKind::Inte...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
JustinStitt
force-pushed
the
wraps-and-no-wraps-with-scl
branch
from
bd63565 to
b656e94
Compare
JustinStitt
force-pushed
the
wraps-and-no-wraps-with-scl
branch
from
cfa9cb9 to
0932bcb
Compare
Signed-off-by: Justin Stitt <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
This commit is a combination of two commits. I'm force pushing because the build bot had some problems with an (empty?) or whitespace-only commit during checkout. Hopefully it takes this one. Signed-off-by: Justin Stitt <[email protected]>
Changing the attr base class from DeclOrTypeAttr to TypeAttr allows us to better isolate the attribute handling logic just in SemaType.cpp in the handleWrapsAttr() method. Furthermore, check for language compatibility at this step as well. Rely on `COnly` to provide the warnings to the user, then silently discard the attribute. | let LangOpts = [COnly]; Signed-off-by: Justin Stitt <[email protected]>
JustinStitt
force-pushed
the
wraps-and-no-wraps-with-scl
branch
from
3a17f3b to
b9f3a5a
Compare
| when paired with `Sanitizer Special Case Lists (SSCL) | ||
| <https://clang.llvm.org/docs/SanitizerSpecialCaseList.html>`_. | ||
|
|
||
| These attributes are only valid for C, as there are built-in language |
There was a problem hiding this comment.
If the attribute is used with C++ code will it break the build?
I.e. if someone includes a C header into a C++ project that uses these attributes, are we going to run into problems? If yes, this doesn't sound right to me.
There was a problem hiding this comment.
No, it won't break the build. You will get an ignored attribute warning and the attributes won't be applied or have any effect. (I'll document this).
These attributes are relatively well supported in C++ but that language has many more ways for the attribute to disappear unexpectedly.
These attributes are mostly needed in C (as there aren't any alternatives).
There was a problem hiding this comment.
I guess it wouldn't be the worst idea to just let the attribute exist in C++ and document common breakages? We should really urge C++ users to use the type system and operator overloading to their advantage, though, and not use wraps/no_wraps.
What do we think?
There was a problem hiding this comment.
My view is that the semantics of C code should, where possible, not change when compiling as C++. This also holds for your type-based proposal.
|
I am comfortable accepting sanitization related stuff, and will be happy to look into implementation details. |
There was a problem hiding this comment.
I guess we already discussed that before. My preference is the wrap and sanitize attributes are interdependent. Open to be convinced otherwise.
But seems like we want consistency with https://godbolt.org/z/crhdaczx1
__attribute__((wraps,no_sanitize("signed-integer-overflow")))wraps quietly__attribute__((wraps))wraps, but may report if runs with-fsanitize=signed-integer-overflow__attribute__((no_sanitize("signed-integer-overflow")))reports, but in recovery mode, compile UB any way it likes.
Note: as-is attribute((no_sanitize("signed-integer-overflow"))) is not supported on variables, but if we can do wraps, we can do no_sanitize.
| There is also ``__attribute__((no_wraps))`` which can be added to types or | ||
| variable declarations. Types or variables with this attribute may be | ||
| instrumented by overflow sanitizers, if enabled. Note that this matches the | ||
| default behavior of integer types. So, in most cases, ``no_wraps`` serves |
There was a problem hiding this comment.
So this is essentially no opt?
I don't thing this will help, to me it will trigger readers to think what it's needed, unnecessary noise.
Regular comment will be as good enough?
There was a problem hiding this comment.
When used alongside very strict SSCLs there is great utility to be gained from no_wraps.
In the kernel we want to use an SSCL ignorelist like this:
[{signed-integer-overflow,unsigned-integer-overflow}]
type:*
then annotate specific types like size_t in source:
typedef __kernel_size_t size_t __attribute__((no_wraps));... Otherwise, yes, this is just a simple code annotation.
| attributed with the ``wraps`` attribute. | ||
|
|
||
| The ``signed-integer-overflow``, ``unsigned-integer-overflow``, | ||
| ``implicit-signed-integer-truncation`` and the |
There was a problem hiding this comment.
I don't think it should affect instrumentation at all.
We have -fsanitize-recover= mode.
In this case we need instrumentation to print report, regardless of "wrap", and the "wrap" or -fwrap will control how to calculate after the report.
to suppress report we have no_sanitize attribute already
There was a problem hiding this comment.
The entire intention of wraps is to disable instrumentation entirely. We never want a report or trap out of such expressions. Developers who have very intentional wrapping arithmetic should be able to delegate types as "wrapping" and move on with their day while still getting high-signal overflow sanitizer reports from actual bugs (stuff not marked wraps).
Furthermore, most of the use of no_wraps is in conjunction with SSCLs to further control overflow behavior at the type-level. Sanitizer recovery modes remain separate from all of this. I predict no_wraps would be infrequently used outside of the Linux kernel. It still has value, though.
| [signed-integer-overflow] | ||
| type:* | ||
|
|
||
| ``no_wraps`` can override the behavior provided by the ignorelist to |
There was a problem hiding this comment.
I think wrap and sanitize should be independent, because of "recover" mode.
In this case no_wraps is redundant, unless it's going to override -fwrap.
There was a problem hiding this comment.
Read my other comments about no_wraps + SSCLs and recovery modes. I don't think any consideration to recovery modes should be made from wraps or no_wraps. These attributes only care about sanitizer instrumentation and not the handling thereof.
There was a problem hiding this comment.
But I see what you're saying about __attribute__((sanitize(...))). Do you think we should use __attribute__((sanitize(...))) instead of __attribute__((no_wraps))? I think this may become confusing to users who think this is turning on a sanitizer, which is not what it does; It really is stating that something is "sanitizeable".
And for wraps, maybe you prefer __attribute__((no_sanitize(...))) over __attribute__((wraps))?
There was a problem hiding this comment.
But I see what you're saying about
__attribute__((sanitize(...))). Do you think we should use__attribute__((sanitize(...)))instead of__attribute__((no_wraps))? I think this may become confusing to users who think this is turning on a sanitizer, which is not what it does; It really is stating that something is "sanitizeable".And for
wraps, maybe you prefer__attribute__((no_sanitize(...)))over__attribute__((wraps))?
Yes, if, like you said before, wraps is about disabling sanitizers, than no_sanitize looks better. We already can apply no_sanitizer(address) on some globals.
So, no_wraps(or sanitize) is here just to inverse ignore list for particular variable?
I don't think it's worth of it.
In general idea of sanitizers is to check code with minimal modification.
Some stuff required to avoid false-positives, but false-negatives we usually quite easy on them for sake of usability.
So if you need to suppress type, I don't think cherry-picking particular variables to suppress will have wide adoption, and may only annoy opposing users even more.
Saying from my experience in google3, I would rather over-suppress than introduce additional burden of maintenance.
BTW. you have "typedef" support in ignore list. Wouldn't e nicer just to change e.g type from e.g. int to sanitizer_int typedef or something?
So I see value in wraps/no_sanitize, but the reverse looks questionable.
There was a problem hiding this comment.
Patch itself, if we introduce wrap/no_sanitize, I don't think no_wrap/sanitize is a big problem for clang maintainers. They may think otherwise :)
However, this just smells like YAGNI to me.
I would see Linux roll-out, as multi step process, something like:
- Roll-out with over-suppression: ignore list and no_sanitizer attribute.
- If Linux is able to keep up with those checks, maybe reduce suppression scope with available tools.
- Then you may want to go further and need no_wrap/sanitize attribute. But I suspect ROI from step 3 will be so low, that it's not going to happen.
There was a problem hiding this comment.
- Roll-out with over-suppression: ignore list and no_sanitizer attribute.
The current plan for the Kernel is ignoring all types and then using no_wraps or sanitize (the name of the thing doesn't matter) on a few types. This helps towards (2.)
- If Linux is able to keep up with those checks, maybe reduce suppression scope with available tools.
We will be able to keep up with the checks because we will iteratively expand the scope of no_wraps to more types in accordance with what our overflow-fixing bandwidth is.
- Then you may want to go further and need no_wrap/sanitize attribute. But I suspect ROI from step 3 will be so low, that it's not going to happen.
Integer overflow (the unexpected kind) cause a lot of bugs and are an entire attack surface of their own. If we can provide more tools to developers such that enabling sanitizers makes more sense and causes less headaches (false-positives) then I see the ROI being potentially high.
This is what wraps or no_wraps does. It will help codebases better handle wrapping arithmetic and suddenly it becomes less magic and more safe
There was a problem hiding this comment.
@vitalybuka Look at AOSP for example: https://android-review.googlesource.com/c/kernel/common/+/3343205
Sanitizing signed integer overflow in Linux kernel might be useful to
spot bugs, however there are cases where overflow is intended, for
example,atomic_long_ttype and such cases can't be annotated to not
trigger UBSAN.
They, and other kernel projects that use sanitizers, really want a wraps or wraps-equivalent thing.
There was a problem hiding this comment.
The current plan for the Kernel is ignoring all types and then using no_wraps or sanitize (the name of the thing doesn't matter) on a few types. This helps towards (2.)
I see, you are doing this in opposite way. My intuition it's more complicated than needed. But I didn't work on Kernel, maybe there are own specifics. It looks like you need to allow all types used in kernel one by one in the end.
We tried allow list like approach in google3. It usually goes slower than ignore list approach - sanitizer everything that pass, the rest go in ignore list, usually by src: or fun:. Then next stage shrinking ignore list by fixing or converting to attribute on functions.
@ramosian-glider has experience with other sanitizers, maybe you have opinion on the approach #115094 (comment)?
Signed-off-by: Justin Stitt <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
There was a problem hiding this comment.
It seems to me that this should just modify the type, rather than risk getting lost like it does. If you want to define the conversion to the underlying integer type(likely via cast!) that is sensible as well, but this is going to end up getting lost pretty easily, so likely worth trying to see if this is worht a new type in the AST.
Also, did we have an RFC on this? I'd like to see what the community at large has to say.
| @@ -2863,6 +2863,15 @@ bool QualType::isWebAssemblyFuncrefType() const { | |||
| getAddressSpace() == LangAS::wasm_funcref; | |||
| } | |||
|
|
|||
| bool QualType::hasWrapsAttr() const { | |||
| return !isNull() && getTypePtr()->hasAttr(attr::Wraps) && | |||
There was a problem hiding this comment.
we probably want to do 'more work' to pull out the type you want/canonicalize the type here.
| for (unsigned i = 0; i < NumArgs; ++i) { | ||
| Expr *CurrA = TheCall->getArg(i); | ||
|
|
||
| if (IsWrapsAttrLost(S, TheCall, FD, i)) |
There was a problem hiding this comment.
This is looking an AWFUL lot like this needing to be a type with a defined conversion. I don't think AttributedType is sufficient here, this should also probably be a diagnosed-error, which you get when it is a part of the actual type.
There was a problem hiding this comment.
This is looking an AWFUL lot like this needing to be a type with a defined conversion.
Do you mean make wraps or no_wraps a type in the same way that ext_vector_type works? Or even BTFTagAttributedType?
|
|
||
| // wraps and no_wraps are most useful and consistent when used with C. Other | ||
| // languages have better alternatives within their type systems. | ||
| if (S.LangOpts.CPlusPlus || S.LangOpts.ObjC) |
There was a problem hiding this comment.
I'm relying on the default diagnostic provided from the LangOpt COnly in the table gen. The message is the following:
test.cc:2:28: warning: 'wraps' attribute ignored [-Wignored-attributes]
2 | typedef int __attribute__((wraps)) wrapping_int;
| ^
1 warning generated.
Should it have its own diagnostic beyond this builtin?
There was a problem hiding this comment.
Ah! Right... do we need this check then? Or can this be an assert?
There was a problem hiding this comment.
For whatever reason COnly doesn't stop the execution from ending up there during parsing. Can't be an assert
Yes, here's the RFC and the previous PR (of which the scope has been expanded a bit so it deserves its own new PR, IMO) |
Signed-off-by: Justin Stitt <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
... and prefer no_wraps if both want to exist on the same binary expression Signed-off-by: Justin Stitt <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
|
Naming wise I prefer
Would it make sense to support attributes like |
Is there some other precedence for this I am working on the PR for supporting Once I have that PR up can compare it to this PR to find the best way forward. |
No.
sanitizable is LGTM
|
|
RFC regarding canonical wrapping/non-wrapping types in Clang: https://discourse.llvm.org/t/rfc-clang-canonical-wrapping-and-non-wrapping-types/84356 Ultimately, a type like what the RFC describes would supersede this PR in terms of feature completeness and usefulness. I'll close this PR if that RFC suggests a new direction is required. pings: |
+1 - I'd be in favor of the type-based approach. It does make more sense that this is a type qualifier, vs. an attribute which the compiler might loose as soon as type conversions or take-pointer operations are involved. |
Note
This PR is an expansion of #86618, with a larger scope including
no_wrapsand SSCL integration. There's more background and review that is worth reading in #86618 as well.Implement
__attribute__((wraps))and__attribute__((no_wraps)). These attributes are used to annotate the expected wrap-around behavior for types, variables, struct members, or function parameters.These attributes serve as convenient in-source annotations for the expected overflow behavior of code. In addition, these attributes also have functional changes when used alongside certain sanitizers or warnings.
The
signed-integer-overflow,unsigned-integer-overflow,implicit-signed-integer-truncationand theimplicit-unsigned-integer-truncationsanitizers have different behavior when interacting with thewrapsattribute.This is most useful for users that want to enable overflow or truncation sanitizers but also have some code they want to not be instrumented. Without this PR, they can only achieve translation unit level or at best function level granularity without changing arithmetic expressions directly. In an effort to not make basic arithmetic rely on wrappers like
__builtin_add_overflow(), this PR introduces type and variable level granularity.Example
no_wraps
no_wrapsis interesting because it reflects the default case for integer operations and the associated sanitizers. In the general case,no_wrapsjust serves as a friendly annotation to readers of code that something is really not supposed to wrap-around. The sanitizers will instrument them all the same as they would if this attribute was not present.What makes
no_wrapsuseful is when SSCLs are used alongside them. #107332 introduced type-filtering for the overflow and truncation sanitizers which can be overriden byno_wrapsandwraps. To illustrate, let's look at another example:CC
@vitalybuka @kees @melver @bwendling