Skip to content

[Clang] Improve Sema diagnostic performance for __builtin_counted_by_ref #116719

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.

Sign up for GitHub

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

Merged
bwendling merged 9 commits into from
Nov 27, 2024
+113 −111
Merged

[Clang] Improve Sema diagnostic performance for __builtin_counted_by_ref #116719

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2dcf181
[Clang] Improve Sema diagnostic performance for __builtin_counted_by_ref
bwendling Nov 15, 2024
b32983c
Don't use a placeholder. Instead, place diagnostics in the situations…
bwendling Nov 20, 2024
b8d93b6
Remove removed placeholder type.
bwendling Nov 20, 2024
74109c4
Remove unused code.
bwendling Nov 20, 2024
ee9e2b6
Extract check for __builtin_counted_by_ref CallExpr into a helper met…
bwendling Nov 20, 2024
54b277c
Reformat.
bwendling Nov 20, 2024
8ab376a
Turn into a const method.
bwendling Nov 20, 2024
90cdf04
Use helper function.
bwendling Nov 21, 2024
f34c187
Create a separate checking function.
bwendling Nov 27, 2024
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
2 changes: 1 addition & 1 deletion clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6673,7 +6673,7 @@ def err_builtin_counted_by_ref_must_be_flex_array_member : Error<
def err_builtin_counted_by_ref_cannot_leak_reference : Error<
"value returned by '__builtin_counted_by_ref' cannot be assigned to a "
"variable, have its address taken, or passed into or returned from a function">;
def err_builtin_counted_by_ref_invalid_lhs_use : Error<
def err_builtin_counted_by_ref_invalid_use : Error<
"value returned by '__builtin_counted_by_ref' cannot be used in "
"%select{an array subscript|a binary}0 expression">;
def err_builtin_counted_by_ref_has_side_effects : Error<
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2518,6 +2518,11 @@ class Sema final : public SemaBase {

bool BuiltinNonDeterministicValue(CallExpr *TheCall);

bool IsBuiltinCountedByRef(const Expr *E) const {
const CallExpr *CE =
E ? dyn_cast<CallExpr>(E->IgnoreParenImpCasts()) : nullptr;
return CE && CE->getBuiltinCallee() == Builtin::BI__builtin_counted_by_ref;
}
bool BuiltinCountedByRef(CallExpr *TheCall);

// Matrix builtin handling.
Expand Down
7 changes: 7 additions & 0 deletions clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14690,6 +14690,13 @@ void Sema::FinalizeDeclaration(Decl *ThisDecl) {
}
}

// The result of __builtin_counted_by_ref cannot be assigned to a variable.
// It allows leaking and modification of bounds safety information.
if (IsBuiltinCountedByRef(VD->getInit()))
Diag(VD->getInit()->getExprLoc(),
diag::err_builtin_counted_by_ref_cannot_leak_reference)
<< VD->getInit()->getSourceRange();
Copy link
Collaborator

@AaronBallman AaronBallman Nov 26, 2024

Choose a reason for hiding this comment

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

Should we split this off into a helper function like bool CheckInvalidBuiltinCountedByRef(const Expr *E); ?

Copy link
Member

@Sirraide Sirraide Nov 26, 2024

Choose a reason for hiding this comment

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

I was going to comment on that too, but ended up forgetting about it; it’s used in like 3–4 places, so I personally would prefer making this a helper function.

Copy link
Collaborator Author

@bwendling bwendling Nov 27, 2024

Choose a reason for hiding this comment

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

I'm not sure I agree. The diagnostic isn't the same in all places. And one of them takes arguments not available from the Expr. I would say that if we were adding several more diagnostics then a separate function would make sense. But for four, it's probably a bit more trouble than it's worth.

Copy link
Collaborator

@AaronBallman AaronBallman Nov 27, 2024

Choose a reason for hiding this comment

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

It is the same in every case. Each time, the logic is:

if (IsBuiltinCountedByRef(SomeExpr))
  Diag(SomeExpr->getExprLoc(), diag::err_builtin_counted_by_ref_cannot_leak_reference) << SomeExpr->getSourceRange();

This would require changing some later to this equivalent:

  auto CheckBuiltinCountedByRef = [&](const Expr *E) {
    if (BinaryOperator::isAssignmentOp(Opc))
      CheckInvalidBuiltinCountedByRef(E);
    else if (IsBuiltinCountedByRef(E))
        Diag(E->getExprLoc(), diag::err_builtin_counted_by_ref_invalid_use)
            << 1 << E->getSourceRange();
  };


checkAttributesAfterMerging(*this, *VD);

if (VD->isStaticLocal())
Expand Down
101 changes: 33 additions & 68 deletions clang/lib/Sema/SemaExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4894,6 +4894,12 @@ ExprResult Sema::ActOnArraySubscriptExpr(Scope *S, Expr *base,
return ExprError();
}

// We cannot use __builtin_counted_by_ref in a binary expression. It's
// possible to leak the reference and violate bounds security.
if (IsBuiltinCountedByRef(base))
Diag(base->getExprLoc(), diag::err_builtin_counted_by_ref_invalid_use)
<< 0 << base->getSourceRange();

// Handle any non-overload placeholder types in the base and index
// expressions. We can't handle overloads here because the other
// operand might be an overloadable type, in which case the overload
Expand Down Expand Up @@ -6486,6 +6492,16 @@ ExprResult Sema::BuildCallExpr(Scope *Scope, Expr *Fn, SourceLocation LParenLoc,
if (CheckArgsForPlaceholders(ArgExprs))
return ExprError();

// The result of __builtin_counted_by_ref cannot be used as a function
// argument. It allows leaking and modification of bounds safety information.
for (const Expr *Arg : ArgExprs)
if (IsBuiltinCountedByRef(Arg)) {
Diag(Arg->getExprLoc(),
diag::err_builtin_counted_by_ref_cannot_leak_reference)
<< Arg->getSourceRange();
return ExprError();
}

if (getLangOpts().CPlusPlus) {
// If this is a pseudo-destructor expression, build the call immediately.
if (isa<CXXPseudoDestructorExpr>(Fn)) {
Expand Down Expand Up @@ -9196,38 +9212,6 @@ Sema::CheckAssignmentConstraints(QualType LHSType, ExprResult &RHS,
LHSType = Context.getCanonicalType(LHSType).getUnqualifiedType();
RHSType = Context.getCanonicalType(RHSType).getUnqualifiedType();

// __builtin_counted_by_ref cannot be assigned to a variable, used in
// function call, or in a return.
auto FindBuiltinCountedByRefExpr = [&](Expr *E) -> CallExpr * {
struct BuiltinCountedByRefVisitor : DynamicRecursiveASTVisitor {
CallExpr *TheCall = nullptr;
bool VisitCallExpr(CallExpr *CE) override {
if (CE->getBuiltinCallee() == Builtin::BI__builtin_counted_by_ref) {
TheCall = CE;
return false;
}
return true;
}
bool
VisitUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *UE) override {
// A UnaryExprOrTypeTraitExpr---e.g. sizeof, __alignof, etc.---isn't
// the same as a CallExpr, so if we find a __builtin_counted_by_ref()
// call in one, ignore it.
return false;
}
} V;
V.TraverseStmt(E);
return V.TheCall;
};
static llvm::SmallPtrSet<CallExpr *, 4> Diagnosed;
if (auto *CE = FindBuiltinCountedByRefExpr(RHS.get());
CE && !Diagnosed.count(CE)) {
Diagnosed.insert(CE);
Diag(CE->getExprLoc(),
diag::err_builtin_counted_by_ref_cannot_leak_reference)
<< CE->getSourceRange();
}

// Common case: no conversion required.
if (LHSType == RHSType) {
Kind = CK_NoOp;
Expand Down Expand Up @@ -13778,42 +13762,6 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
ConvTy = CheckAssignmentConstraints(Loc, LHSType, RHSType);
}

// __builtin_counted_by_ref can't be used in a binary expression or array
// subscript on the LHS.
int DiagOption = -1;
auto FindInvalidUseOfBoundsSafetyCounter = [&](Expr *E) -> CallExpr * {
struct BuiltinCountedByRefVisitor : DynamicRecursiveASTVisitor {
CallExpr *CE = nullptr;
bool InvalidUse = false;
int Option = -1;

bool VisitCallExpr(CallExpr *E) override {
if (E->getBuiltinCallee() == Builtin::BI__builtin_counted_by_ref) {
CE = E;
return false;
}
return true;
}

bool VisitArraySubscriptExpr(ArraySubscriptExpr *E) override {
InvalidUse = true;
Option = 0; // report 'array expression' in diagnostic.
return true;
}
bool VisitBinaryOperator(BinaryOperator *E) override {
InvalidUse = true;
Option = 1; // report 'binary expression' in diagnostic.
return true;
}
} V;
V.TraverseStmt(E);
DiagOption = V.Option;
return V.InvalidUse ? V.CE : nullptr;
};
if (auto *CE = FindInvalidUseOfBoundsSafetyCounter(LHSExpr))
Diag(CE->getExprLoc(), diag::err_builtin_counted_by_ref_invalid_lhs_use)
<< DiagOption << CE->getSourceRange();

if (DiagnoseAssignmentResult(ConvTy, Loc, LHSType, RHSType, RHS.get(),
AssignmentAction::Assigning))
return QualType();
Expand Down Expand Up @@ -15274,6 +15222,23 @@ ExprResult Sema::ActOnBinOp(Scope *S, SourceLocation TokLoc,
if (Kind == tok::TokenKind::slash)
DetectPrecisionLossInComplexDivision(*this, TokLoc, LHSExpr);

// We cannot use __builtin_counted_by_ref in a binary expression. It's
// possible to leak the reference and violate bounds security.
auto CheckBuiltinCountedByRef = [&](const Expr *E) {
if (IsBuiltinCountedByRef(E)) {
if (BinaryOperator::isAssignmentOp(Opc))
Diag(E->getExprLoc(),
diag::err_builtin_counted_by_ref_cannot_leak_reference)
<< E->getSourceRange();
else
Diag(E->getExprLoc(), diag::err_builtin_counted_by_ref_invalid_use)
<< 1 << E->getSourceRange();
}
};

CheckBuiltinCountedByRef(LHSExpr);
CheckBuiltinCountedByRef(RHSExpr);

return BuildBinOp(S, TokLoc, Opc, LHSExpr, RHSExpr);
}

Expand Down
5 changes: 5 additions & 0 deletions clang/lib/Sema/SemaStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3765,6 +3765,11 @@ Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
<< FSI->getFirstCoroutineStmtKeyword();
}

if (IsBuiltinCountedByRef(RetVal.get()))
Diag(RetVal.get()->getExprLoc(),
diag::err_builtin_counted_by_ref_cannot_leak_reference)
<< RetVal.get()->getSourceRange();

StmtResult R =
BuildReturnStmt(ReturnLoc, RetVal.get(), /*AllowRecovery=*/true);
if (R.isInvalid() || ExprEvalContexts.back().isDiscardedStatementContext())
Expand Down
77 changes: 39 additions & 38 deletions clang/test/Sema/builtin-counted-by-ref.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,62 +11,63 @@ struct fam_struct {
int array[] __attribute__((counted_by(count)));
};

void test1(struct fam_struct *ptr, int size, int idx) {
size_t size_of = sizeof(__builtin_counted_by_ref(ptr->array)); // ok

*__builtin_counted_by_ref(ptr->array) = size; // ok
void g(char *);
void h(char);

{
size_t __ignored_assignment;
*_Generic(__builtin_counted_by_ref(ptr->array),
void *: &__ignored_assignment,
default: __builtin_counted_by_ref(ptr->array)) = 42; // ok
}
void test1(struct fam_struct *ptr, int size, int idx) {
size_t size_of = sizeof(__builtin_counted_by_ref(ptr->array)); // ok
int align_of = __alignof(__builtin_counted_by_ref(ptr->array)); // ok
size_t __ignored_assignment;

*__builtin_counted_by_ref(ptr->array) = size; // ok
*_Generic(__builtin_counted_by_ref(ptr->array),
void *: &__ignored_assignment,
default: __builtin_counted_by_ref(ptr->array)) = 42; // ok
h(*__builtin_counted_by_ref(ptr->array)); // ok
}

void test2(struct fam_struct *ptr, int idx) {
__builtin_counted_by_ref(); // expected-error {{too few arguments to function call, expected 1, have 0}}
__builtin_counted_by_ref(ptr->array, ptr->x, ptr->count); // expected-error {{too many arguments to function call, expected 1, have 3}}
__builtin_counted_by_ref(); // expected-error {{too few arguments to function call, expected 1, have 0}}
__builtin_counted_by_ref(ptr->array, ptr->x, ptr->count); // expected-error {{too many arguments to function call, expected 1, have 3}}
}

void test3(struct fam_struct *ptr, int idx) {
__builtin_counted_by_ref(&ptr->array[0]); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(&ptr->array[idx]); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(&ptr->array); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(ptr->x); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(&ptr->x); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(global_array); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(global_int); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(&global_int); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(&ptr->array[0]); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(&ptr->array[idx]); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(&ptr->array); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(ptr->x); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(&ptr->x); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(global_array); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(global_int); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
__builtin_counted_by_ref(&global_int); // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
}

void test4(struct fam_struct *ptr, int idx) {
__builtin_counted_by_ref(ptr++->array); // expected-error {{'__builtin_counted_by_ref' argument cannot have side-effects}}
__builtin_counted_by_ref(&ptr->array[idx++]); // expected-error {{'__builtin_counted_by_ref' argument cannot have side-effects}}
__builtin_counted_by_ref(ptr++->array); // expected-error {{'__builtin_counted_by_ref' argument cannot have side-effects}}
__builtin_counted_by_ref(&ptr->array[idx++]); // expected-error {{'__builtin_counted_by_ref' argument cannot have side-effects}}
}

void foo(char *);

void *test5(struct fam_struct *ptr, int size, int idx) {
char *ref = __builtin_counted_by_ref(ptr->array); // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
char *ref = __builtin_counted_by_ref(ptr->array); // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
char *int_ptr;
char *p;

ref = __builtin_counted_by_ref(ptr->array); // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
ref = (char *)(int *)(42 + &*__builtin_counted_by_ref(ptr->array)); // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
foo(__builtin_counted_by_ref(ptr->array)); // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
foo(ref = __builtin_counted_by_ref(ptr->array)); // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
ref = __builtin_counted_by_ref(ptr->array); // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
g(__builtin_counted_by_ref(ptr->array)); // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
g(ref = __builtin_counted_by_ref(ptr->array)); // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}

if ((ref = __builtin_counted_by_ref(ptr->array))) // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
if ((ref = __builtin_counted_by_ref(ptr->array))) // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
;

for (char *p = __builtin_counted_by_ref(ptr->array); p && *p; ++p) // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
for (p = __builtin_counted_by_ref(ptr->array); p && *p; ++p) // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
;

return __builtin_counted_by_ref(ptr->array); // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
return __builtin_counted_by_ref(ptr->array); // expected-error {{value returned by '__builtin_counted_by_ref' cannot be assigned to a variable, have its address taken, or passed into or returned from a function}}
}

void test6(struct fam_struct *ptr, int size, int idx) {
*(__builtin_counted_by_ref(ptr->array) + 4) = 37; // expected-error {{value returned by '__builtin_counted_by_ref' cannot be used in a binary expression}}
__builtin_counted_by_ref(ptr->array)[3] = 37; // expected-error {{value returned by '__builtin_counted_by_ref' cannot be used in an array subscript expression}}
*(__builtin_counted_by_ref(ptr->array) + 4) = 37; // expected-error {{value returned by '__builtin_counted_by_ref' cannot be used in a binary expression}}
__builtin_counted_by_ref(ptr->array)[3] = 37; // expected-error {{value returned by '__builtin_counted_by_ref' cannot be used in an array subscript expression}}
}

struct non_fam_struct {
Expand All @@ -77,10 +78,10 @@ struct non_fam_struct {
};

void *test7(struct non_fam_struct *ptr, int size) {
*__builtin_counted_by_ref(ptr->array) = size // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
*__builtin_counted_by_ref(&ptr->array[0]) = size; // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
*__builtin_counted_by_ref(ptr->pointer) = size; // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
*__builtin_counted_by_ref(&ptr->pointer[0]) = size; // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
*__builtin_counted_by_ref(ptr->array) = size // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
*__builtin_counted_by_ref(&ptr->array[0]) = size; // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
*__builtin_counted_by_ref(ptr->pointer) = size; // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
*__builtin_counted_by_ref(&ptr->pointer[0]) = size; // expected-error {{'__builtin_counted_by_ref' argument must reference a flexible array member}}
}

struct char_count {
Expand Down
Loading