Skip to content

[Clang] add wraps and no_wraps attributes #115094

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,26 @@ 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@JustinStitt JustinStitt Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@JustinStitt JustinStitt Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

alternatives for other languages.

Improvements to Clang's diagnostics
-----------------------------------

Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/AST/Expr.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/// Does one of the subexpressions have 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,
Expand Down
3 changes: 3 additions & 0 deletions clang/include/clang/AST/Type.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
15 changes: 15 additions & 0 deletions clang/include/clang/Basic/Attr.td
Original file line number Diff line number Diff line change
Expand Up @@ -4855,3 +4855,18 @@ def ClspvLibclcBuiltin: InheritableAttr {
let Documentation = [ClspvLibclcBuiltinDoc];
let SimpleHandler = 1;
}

def Wraps : TypeAttr {
let Spellings = [Clang<"wraps">];
let Subjects = SubjectList<[Var, TypedefName, Field]>;
let Documentation = [WrapsDocs];
let LangOpts = [COnly];
}

def NoWraps : TypeAttr {
let Spellings = [Clang<"no_wraps">];
let Subjects = SubjectList<[Var, TypedefName, Field]>;
let Documentation = [NoWrapsDocs];
let LangOpts = [COnly];
}

103 changes: 103 additions & 0 deletions clang/include/clang/Basic/AttrDocs.td
Original file line number Diff line number Diff line change
Expand Up @@ -8710,3 +8710,106 @@ 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. This is similar to ``-fwrapv`` but at the
type and declaration level. 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@JustinStitt JustinStitt Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

``implicit-unsigned-integer-truncation`` sanitizers will not instrument
arithmetic containing any operands attributed by ``wraps``. Similarly, the
``-Winteger-overflow`` and ``-Wconstant-conversion`` warnings are 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 an error.

``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.

``no_wraps`` only interacts with sanitizers and
`Sanitizer special case lists <https://clang.llvm.org/docs/SanitizerSpecialCaseList.html>`_ but,
unlike ``wraps``, it does not modify the signed overflow behavior of types it
affects. Instead, you should use ``-fwrapv`` or ``-fno-wrapv`` to define signed
overflow behavior at the language level in combination with ``wraps`` for
type-level modifications.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))?

Copy link
Collaborator

@vitalybuka vitalybuka Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@vitalybuka vitalybuka Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Roll-out with over-suppression: ignore list and no_sanitizer attribute.
  2. If Linux is able to keep up with those checks, maybe reduce suppression scope with available tools.
  3. 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.

Copy link
Contributor Author

@JustinStitt JustinStitt Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.)

  1. 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.

  1. 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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_t type 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.

Copy link
Collaborator

@vitalybuka vitalybuka Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

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. ``-Wimplicitly-discarded-wraps-attribute``
warnings can be caused in situations where the ``wraps`` or ``no_wraps``
attribute cannot persist through implicit type conversions. Disable this with
``-Wno-implicitly-discarded-wraps-attribute``.

Applying both ``__attribute__((wraps))`` and ``__attribute__((no_wraps))``
results in an error, as you should only use one or the other. Arithmetic binary
operators containing both `wraps` and `no_wraps`-attributed types will only
persist the `no_wraps` attribute towards the final resulting type.

Like ``wraps``, ``no_wraps`` may also be used with function parameters or
declarations of variables as well as members of structures but can not be used
with non-integer types.
}];
}

2 changes: 2 additions & 0 deletions clang/include/clang/Basic/DiagnosticGroups.td
Original file line number Diff line number Diff line change
Expand Up @@ -1588,3 +1588,5 @@ 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 about the wraps attribute getting implicitly discarded
def ImpDiscardedWrapsAttr : DiagGroup<"implicitly-discarded-wraps-attribute">;
10 changes: 10 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -6664,6 +6664,16 @@ def err_builtin_counted_by_ref_invalid_lhs_use : Error<
def err_builtin_counted_by_ref_has_side_effects : Error<
"'__builtin_counted_by_ref' argument cannot have side-effects">;

// __attribute__((wraps)) and __attribute__((no_wraps)) diagnostics:
def warn_wraps_attr_maybe_lost : Warning<
"'%select{wraps|no_wraps}0' attribute may be implicitly discarded when converted to %1">,
InGroup<ImpDiscardedWrapsAttr>;
def err_wraps_attr_var_decl_type_not_integer : Error<
"cannot use attribute '%select{wraps|no_wraps}0' with non-integer type '%1'">;
def err_wraps_attr_with_no_wraps_attr : Error<
"attribute 'wraps' cannot be used alongside 'no_wraps'">;


let CategoryName = "ARC Semantic Issue" in {

// ARC-mode diagnostics.
Expand Down
18 changes: 18 additions & 0 deletions clang/lib/AST/Expr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -4852,6 +4862,10 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
if (hasStoredFPFeatures())
setStoredFPFeatures(FPFeatures);
setDependence(computeDependence(this));
if (hasNonWrappingOperand(Ctx))
setType(Ctx.getAttributedType(attr::NoWraps, getType(), getType()));
else if (hasWrappingOperand(Ctx))
setType(Ctx.getAttributedType(attr::Wraps, getType(), getType()));
}

BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
Expand All @@ -4870,6 +4884,10 @@ BinaryOperator::BinaryOperator(const ASTContext &Ctx, Expr *lhs, Expr *rhs,
if (hasStoredFPFeatures())
setStoredFPFeatures(FPFeatures);
setDependence(computeDependence(this));
if (hasNonWrappingOperand(Ctx))
setType(Ctx.getAttributedType(attr::NoWraps, getType(), getType()));
else if (hasWrappingOperand(Ctx))
setType(Ctx.getAttributedType(attr::Wraps, getType(), getType()));
}

BinaryOperator *BinaryOperator::CreateEmpty(const ASTContext &C,
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/AST/ExprConstant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
9 changes: 9 additions & 0 deletions clang/lib/AST/Type.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2863,6 +2863,15 @@ bool QualType::isWebAssemblyFuncrefType() const {
getAddressSpace() == LangAS::wasm_funcref;
}

bool QualType::hasWrapsAttr() const {
return !isNull() && getTypePtr()->hasAttr(attr::Wraps) &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably want to do 'more work' to pull out the type you want/canonicalize the type here.

!getTypePtr()->hasAttr(attr::NoWraps);
}

bool QualType::hasNoWrapsAttr() const {
return !isNull() && getTypePtr()->hasAttr(attr::NoWraps);
}

QualType::PrimitiveDefaultInitializeKind
QualType::isNonTrivialToPrimitiveDefaultInitialize() const {
if (const auto *RT =
Expand Down
6 changes: 6 additions & 0 deletions clang/lib/AST/TypePrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading