Target a single assignment in bounds checking error messages #893

dopelsunce · 2020-08-03T18:24:25Z

In bounds checking related diagnostic messages, highlight the assignment within a top-level statement that causes the error or warning. This PR considers two types of bounds checking error messages for a variable V:
1. The compiler cannot prove or can disprove the declared bounds for V are valid after an assignment to a variable in the bounds of V; and
2. The inferred bounds of V become unknown after an assignment to a variable used in the declared bounds for V.
Print 'assignment', 'increment', 'decrement' or 'initialization' in the error message depending on the "type" of the target expression.

An example

array_ptr<int> p : count(len) = ...;
other = 0, len = len * 2;

Error message before the change (highlight the whole statement):

error: inferred bounds for 'p' are unknown after statement
  other = 0, len = len * 2;
  ^~~~~~~~~~~~~~~~~~~~~~~~~

After the change (blame the embedded assignment that caused the error; print 'assignment' instead of 'statement'):

error: inferred bounds for 'p' are unknown after assignment
  other = 0, len = len * 2;
             ^~~~~~~~~~~~~

… bounds

kkjeer

Looks good so far! I think there is a missing case where the compiler cannot prove (or can disprove) that the updated observed bounds imply the declared bounds of a variable v after an assignment to a variable i that is used in the declared bounds of v. Also, please remove extraneous whitespace changes from the PR.

clang/lib/Sema/SemaBounds.cpp

kkjeer · 2020-08-03T21:15:00Z

clang/lib/Sema/SemaBounds.cpp

+        BDCType = Sema::BoundsDeclarationCheck::BDC_Initialization;
+      }
+
+      // Find the last assignment to V to blame in the diagnostic message if it


The expression in BlameAssignments (if it exists) might not be an assignment to V. It could be an assignment to a variable used in the bounds of V.

kkjeer · 2020-08-03T21:21:42Z

clang/lib/Sema/SemaBounds.cpp

@@ -4069,8 +4134,10 @@ namespace {
        const VarDecl *W = Pair.first;
        BoundsExpr *Bounds = Pair.second;
        BoundsExpr *AdjustedBounds = ReplaceVariableInBounds(Bounds, V, OriginalValue, CSS);
-        if (!Pair.second->isUnknown() && AdjustedBounds->isUnknown())
+        if (!Pair.second->isUnknown() && AdjustedBounds->isUnknown()) {


This seems to only update BlameAssignments if the updated observed bounds of W are unknown. For a case like:

unsigned int i; array_ptr<int> p : count(i) = 0; // Original value of i in i++ is i - 1 // i++ causes the updated observed bounds of p to be (p, p + i - 1) i++, unrelated_variable = 0;

We want BlameAssignments[p] => i++.

kkjeer · 2020-08-03T21:26:23Z

clang/lib/Sema/SemaBounds.cpp

+      //
+      // BlameAssignments is used to provide more context for two types of
+      // diagnostic messages:
+      //   1. The compiler cannot prove or can disprove the declared bounds for


It is also possible that the compiler cannot prove or can disprove the declared bounds for V are valid after an assignment to a variable used in the declared bounds of V. For example:

unsigned int len; array_ptr<int> p : count(len) = 0; // Original value of len is len + 2 // Updated observed bounds of p are (p, p + len - 2) // Cannot prove that bounds(p, p + len - 2) imply (p, p + len) len = len + 2;

I've changed the comment to this general case.

clang/lib/Sema/SemaBounds.cpp

* Simplify logic and improve comments

kkjeer · 2020-08-05T19:09:24Z

clang/lib/Sema/SemaBounds.cpp

+      }
+
+      // Find the assignment (if it exists) to blame for the error or warning.
+      auto It = State.BlameAssignments.find(V);


Can this logic move into a helper method that sets a SourceLocation, SourceRange, and BDCType based on St and BlameAssignments?

kkjeer · 2020-08-05T19:12:45Z

clang/lib/Sema/SemaBounds.cpp

+      // the whole St in the error message.
+      SourceLocation Loc = St->getBeginLoc();
+      SourceRange SrcRange = St->getSourceRange();
+      auto BDCType = Sema::BoundsDeclarationCheck::BDC_Statement;


This could miss the case where the bounds of V are unknown after an initialization, e.g.

array_ptr<int> unknown_src : bounds(unknown); // Bounds of arr are unknown after initialization array_ptr<int> arr : count(1) = unknown_src;

kkjeer · 2020-08-05T23:25:03Z

clang/lib/Sema/SemaBounds.cpp

+    // BlameAssignmentWithinStmt returns the source location of the blamed
+    // assignment.
+    SourceLocation BlameAssignmentWithinStmt(Stmt *St, 
+                 const VarDecl *V, CheckingState State, unsigned DiagId) const {


Please align const VarDecl *V underneath Stmt *St (and ensure the remaining parameters are on separate lines if necessary in order to prevent lines longer than 80 characters).

kkjeer · 2020-08-05T23:26:39Z

clang/lib/Sema/SemaBounds.cpp

@@ -4017,11 +4023,55 @@ namespace {
        << ObservedBounds << ObservedBounds->getSourceRange();
    }

+    // BlameAssignmentWithinStmt prints a diagnostic message that highlights the
+    // assignment expression in St that causes V's observed bounds to be unknown
+    // or invalid.  If St is a DeclStmt, St itself and V are highlighted.


Nit: BlameAssignmentWithinStmt can also be used to print a diagnostic message if V's bounds are not necessarily invalid, but are not provably valid. I suggest updating the comment to something like "not provably valid" instead of "invalid".

Thanks for the suggestion!

kkjeer · 2020-08-05T23:28:49Z

clang/lib/Sema/SemaBounds.cpp

+      // starts at the beginning of a declaration T v = e, then extra
+      // diagnostics may be emitted for T.
+      SourceLocation Loc = St->getBeginLoc();
+      if (isa<DeclStmt>(St)) {


Is is possible for a DeclStmt to have an entry in State.BlameAssignments? If not, you can early return here rather than searching for V in BlameAssignments below.

DeclStmt does not have entries in State.BlameAssignments. I made the function return early inside the if block.

kkjeer

Looks good! @dtarditi @mgrang this is ready for a quick further review.

mgrang

Overall looks fine to me. Just a couple of clarifying comments.

clang/include/clang/Sema/Sema.h

clang/lib/Sema/SemaBounds.cpp

mgrang · 2020-08-07T21:33:32Z

clang/lib/Sema/SemaBounds.cpp

+        if (UnaryOperator *UO = dyn_cast<UnaryOperator>(BlameExpr)) {
+          if (UO->isIncrementOp())
+            BDCType = Sema::BoundsDeclarationCheck::BDC_Increment;
+          else if (UO->isDecrementOp())


Are there plans to handle other unary operators like unary minus, deref, address-of, etc?

No. Here UO must be a self-modifying unary operator (because UO causes the bounds to be updated), and I can only think of ++ and --.

I can also put an assert here.

dtarditi

Looks great. Thank you! I have one suggestion for improving a comment.

dtarditi · 2020-08-27T17:18:13Z

clang/lib/Sema/SemaBounds.cpp

+
+        // Choose the type of assignment E to show in the diagnostic messages
+        // from: assignment (=), decrement (--) or increment (++). These are all
+        // modifying operators that can update observed bounds.


Could you add a comment that says "If none of these cases match, the diagnostic message reports that the error is for a statement."? When I first read the code, I noticed there were several ways we could fall off the end of an if-statement chain. It doesn't matter because the code just reports a diagnostic message for a statement type.

dopelsunce added 5 commits July 29, 2020 15:13

Target error message at assignments

5722d2d

Target subexpressions to blame on error messages for unknown inferred…

021e928

… bounds

Update tests to reflect fine-grained error messages

354a859

Update comments

2a82843

Remove unused variable

af5f9c3

kkjeer reviewed Aug 3, 2020

View reviewed changes

dopelsunce added 2 commits August 3, 2020 16:25

* Also add (V, E) to BlameAssignments if E modifies the bounds of V

f6f3af5

* Simplify logic and improve comments

Update tests to reflect the missing case

7296d8b

dopelsunce mentioned this pull request Aug 3, 2020

Update tests to reflect improved bounds checking error messages checkedc/checkedc#412

Merged

dopelsunce added 4 commits August 3, 2020 17:02

Restore whitespace

70aa057

Restore whitespace

3abacfd

Restore whitespace

b22bd1a

Restore whitespace

29926f2

dopelsunce marked this pull request as ready for review August 4, 2020 00:14

dopelsunce requested review from dtarditi, emoham and mgrang August 4, 2020 00:14

kkjeer reviewed Aug 5, 2020

View reviewed changes

Refactor and address the missing case; fix tests

3317bce

kkjeer reviewed Aug 5, 2020

View reviewed changes

Clarify the logic in BlameAssignmentWithinStmt

ff01d32

kkjeer approved these changes Aug 7, 2020

View reviewed changes

mgrang approved these changes Aug 7, 2020

View reviewed changes

kkjeer mentioned this pull request Aug 8, 2020

Improve error messages for bounds checking increment/decrement operators #844

Closed

Reorder BoundsDeclarationCheck enum and update comments

d321731

dtarditi approved these changes Aug 27, 2020

View reviewed changes

Add comments and resolve conflict

85c6b72

kkjeer merged commit 4b47004 into master Aug 27, 2020

kkjeer mentioned this pull request Aug 28, 2020

Change expected error to 'increment' rather than 'statement' #907

Merged

kkjeer deleted the error_message_assignment branch September 11, 2021 00:00

This was referenced Jan 15, 2022

Update tests to reflect improved bounds checking error messages checkedc/checkedc-fork#411

Closed

Target a single assignment in bounds checking error messages checkedc/checkedc-llvm-project#889

Closed

Target a single assignment in bounds checking error messages #893

Target a single assignment in bounds checking error messages #893

Uh oh!

Conversation

dopelsunce commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

An example

Uh oh!

kkjeer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkjeer left a comment

Choose a reason for hiding this comment

Uh oh!

mgrang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dopelsunce Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtarditi left a comment

Choose a reason for hiding this comment

Uh oh!

dtarditi Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dopelsunce commented Aug 3, 2020 •

edited

Loading

dopelsunce Aug 7, 2020 •

edited

Loading

dtarditi Aug 27, 2020 •

edited

Loading