Introduce temporary bindings to BoundsCastExprs #694

kkjeer · 2019-10-11T01:32:07Z

This PR adds a temporary binding expression when building a BoundsCastExpr. This helps enable dynamic and assume bounds casts expressions to be treated as non-value-preserving in CanonBounds::CompareExpr (see issue #691).

Changes

Introduce temporary binding expression to bounds casts.
Adjust bounds inference and visiting cast expressions to reflect the temporary bindings.
Account for BoundsCastExprs when getting the temporary binding of an expression.
Account for whether the expression is in a checked scope or not in PruneTemporaryBindings. (A follow-up issue Differentiate between Memory and Bounds checked scopes in SemaBounds #698 is opened to use the CheckedScopeSpecifier enum rather than a boolean here.)
~~Consider BoundsCastExprs to be value-preserving in CheckBoundsDeclarations::EqualValue. Note: this assumes that the expressions E1 and E2 have been successfully evaluated.~~ This change has been removed. This affects the Checked C static_check_bounds_cast.c test (modified in Add tests for bounds cast expression temporary bindings checkedc#385 - opened Improve memory access detection for bounds casts #695 to track remaining work).
Treat bounds cast expressions as non-value-preserving.
Consider an expression e to be lexicographically equal to the usage of a temporary binding of e.
Remove the llvm_unreachable call if a dynamic cast fails in CanonBounds::Compare. Since bounds casts are no longer considered value-preserving, it is possible to compare, e.g. a BoundsCastExpr and an ImplicitCastExpr. An ImplicitCastExpr cannot be dynamically cast to a BoundsCastExpr.
Account for the cast kind when profiling a BoundsCastExpr.

Tests

Remove the function call test from bounds-decl-challenges. With the introduction of temporaries to bounds cast expressions, this test no longer produces an error.
Add AST tests to verify that bounds cast expressions include a temporary binding expression.
Other tests are added to checked-c (Add tests for bounds cast expression temporary bindings checkedc#385).

…ounds::CompareImpl

…t compare types on bounds expressions

…nfer bounds

…alValue (introducing the assumption that E1 and E2 have been evaluated)

dtarditi

I have a few formatting suggestions. I think that the check in EqualValue needs to be pushed into the lexicographic comparison class.

dtarditi · 2019-10-11T19:16:31Z

lib/AST/CanonBounds.cpp

-  if (BoundsValueExpr *BV1 = dyn_cast<BoundsValueExpr>(E1))
-    if (BV1->getTemporaryBinding() == E2)
+  if (BoundsValueExpr *BV1 = dyn_cast<BoundsValueExpr>(E1)) {
+	CHKCBindTemporaryExpr *Binding = BV1->getTemporaryBinding();


Stylisitc issues: I believe the indentation of the declaration of Binding is off by one-space.

Clang/LLVM developers don't put single statements in blocks. So:

if (Binding == E2) { return Result::Equal; }

should be

if (Binding == E2) return Result::Equal #Resolved

dtarditi · 2019-10-11T19:17:03Z

lib/AST/CanonBounds.cpp

      return Result::Equal;
+	}
+	if (Binding) {


The indentation seems off here.
#Resolved

dtarditi · 2019-10-11T19:20:21Z

lib/Sema/SemaBounds.cpp

@@ -2471,6 +2470,12 @@ namespace {
    }

    CHKCBindTemporaryExpr *GetTempBinding(Expr *E) {
+	  // Bounds casts should always have a temporary binding.
+	  if (BoundsCastExpr *BCE = dyn_cast<BoundsCastExpr>(E)) {
+		CHKCBindTemporaryExpr *Result = dyn_cast<CHKCBindTemporaryExpr>(BCE->getSubExpr());


The indentation should be 2 characters.
#Resolved

dtarditi · 2019-10-11T19:30:30Z

lib/Sema/SemaBounds.cpp

@@ -2234,7 +2222,18 @@ namespace {
    }

    static bool EqualValue(ASTContext &Ctx, Expr *E1, Expr *E2, EquivExprSets *EquivExprs) {
-      Lexicographic::Result R = Lexicographic(Ctx, EquivExprs).CompareExpr(E1, E2);
+	  // It is assumed that E1 and E2 have been successfully evaluated, so that dynamic and assume bounds casts can be treated as value preserving here.


I think you need to push this check into Lexicographic expression comparison. There should be a special mode that answer the questions: if e1 and e2 evaluate to values, are the values guaranteed to be equal? We can assume there that any evaluations of dynamic_bounds_cast in subexpressions of e1 (or 32) will succeed and any bounds checks that depend on bounds changed by *_cast operation will also succeed.

#Resolved

dtarditi

Looks great - nice to see code be deleted and become simpler! I have one suggested comment update. Please run automated testing before committing this,.

dtarditi · 2019-10-11T23:20:11Z

lib/AST/CanonBounds.cpp

-       Cmp = CompareTypeIgnoreCheckedness(E1ChildExpr->getType(),
-                                          E2ChildExpr->getType());
+
+       if (!isa<BoundsExpr>(E1ChildExpr))


It'd be helpful to add a comment "Bounds expressions don't have types".
#Resolved

kkjeer · 2019-10-18T22:45:12Z

Looks great - nice to see code be deleted and become simpler! I have one suggested comment update. Please run automated testing before committing this,.

The automated testing has passed.

dtarditi

For the methods in the BoundsInference class, can you make InCheckedScope a field, instead of passing it as an additional argument? An expression is always in the same checked scope, I believe.

dtarditi · 2019-10-18T23:38:59Z

If we were to add support for statement expressions in GCC (https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html) , an expression might not always be in the same checked scope.

Cherry-picked from commit 82f665e Author: Katherine Kjeer <[email protected]> Commit: GitHub <[email protected]> Introduce temporary bindings to BoundsCastExprs (#694) * Account for cast kind in StmtProfiler::VisitBoundsCastExpr and CanonBounds::CompareImpl * Don't treat dynamic and assume bounds casts as value preserving; don't compare types on bounds expressions * Introduce a temporary binding expression for all bounds cast expressions * Use the temporary bindings introduced in bounds cast expressions to infer bounds * Remove unused PruneTemporaryBindings * Treat e and the usage of a temporary binding of e as equal * Remove llvm_unreachable from CanonBounds::Compare * Account for bounds cast expressions in GetTempBinding * Treat bounds casts as value preserving in CheckBoundsDelarations::EqualValue (introducing the assumption that E1 and E2 have been evaluated) * Remove function call test from bounds-decl-challenges * Add AST dump tests for bounds cast expression temporaries * Fix indentation * More formatting fixes * Revert treating bounds casts as value preserving in EqualValue * Add comment about lack of bounds expression types * Prune temporary bindings from member expression bounds * Account for checked scope information in PruneTemporaryBindings

kakje added 11 commits October 7, 2019 17:02

Account for cast kind in StmtProfiler::VisitBoundsCastExpr and CanonB…

b8e0a0a

…ounds::CompareImpl

Don't treat dynamic and assume bounds casts as value preserving; don'…

78797c9

…t compare types on bounds expressions

Introduce a temporary binding expression for all bounds cast expressions

bf1e89e

Use the temporary bindings introduced in bounds cast expressions to i…

80609a9

…nfer bounds

Remove unused PruneTemporaryBindings

7afb068

Treat e and the usage of a temporary binding of e as equal

1141af2

Remove llvm_unreachable from CanonBounds::Compare

80cbecc

Account for bounds cast expressions in GetTempBinding

469ec62

Treat bounds casts as value preserving in CheckBoundsDelarations::Equ…

0164e42

…alValue (introducing the assumption that E1 and E2 have been evaluated)

Remove function call test from bounds-decl-challenges

a80d3f5

Add AST dump tests for bounds cast expression temporaries

5061058

kkjeer requested review from dtarditi, mgrang and sunnychatterjee October 11, 2019 01:32

kkjeer mentioned this pull request Oct 11, 2019

Add tests for bounds cast expression temporary bindings checkedc/checkedc#385

Merged

dtarditi reviewed Oct 11, 2019

View reviewed changes

kakje added 3 commits October 11, 2019 13:23

Fix indentation

8539cfa

More formatting fixes

974548a

Revert treating bounds casts as value preserving in EqualValue

a1879be

kkjeer mentioned this pull request Oct 11, 2019

Improve memory access detection for bounds casts #695

Closed

dtarditi approved these changes Oct 11, 2019

View reviewed changes

Add comment about lack of bounds expression types

be6a9e0

kkjeer mentioned this pull request Oct 17, 2019

PruneTemporaryBindings results in an array index warning being issued twice #696

Closed

kakje added 2 commits October 16, 2019 19:11

Prune temporary bindings from member expression bounds

54c60b7

Account for checked scope information in PruneTemporaryBindings

5934a06

dtarditi reviewed Oct 18, 2019

View reviewed changes

dtarditi approved these changes Oct 18, 2019

View reviewed changes

kkjeer merged commit 82f665e into master Oct 19, 2019

kkjeer deleted the issue691-bounds-casts-temp-binding branch October 19, 2019 00:16

kkjeer mentioned this pull request Nov 5, 2019

Use temporary binding for memory access base of bounds cast expressions #715

Merged

This was referenced Jan 15, 2022

Add tests for bounds cast expression temporary bindings checkedc/checkedc-fork#384

Closed

Introduce temporary bindings to BoundsCastExprs checkedc/checkedc-llvm-project#690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Introduce temporary bindings to BoundsCastExprs #694

Introduce temporary bindings to BoundsCastExprs #694

Uh oh!

kkjeer commented Oct 11, 2019 •

edited

Loading

Uh oh!

dtarditi left a comment •

edited

Loading

Uh oh!

dtarditi Oct 11, 2019 •

edited by kkjeer

Loading

Uh oh!

dtarditi Oct 11, 2019 •

edited by kkjeer

Loading

Uh oh!

dtarditi Oct 11, 2019 •

edited by kkjeer

Loading

Uh oh!

dtarditi Oct 11, 2019 •

edited by kkjeer

Loading

Uh oh!

dtarditi left a comment

Uh oh!

dtarditi Oct 11, 2019 •

edited by kkjeer

Loading

Uh oh!

kkjeer commented Oct 18, 2019

Uh oh!

dtarditi left a comment

Uh oh!

dtarditi commented Oct 18, 2019

Uh oh!

Uh oh!

Introduce temporary bindings to BoundsCastExprs #694

Introduce temporary bindings to BoundsCastExprs #694

Uh oh!

Conversation

kkjeer commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

dtarditi left a comment • 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 Oct 11, 2019 • edited by kkjeer 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 Oct 11, 2019 • edited by kkjeer 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 Oct 11, 2019 • edited by kkjeer 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 Oct 11, 2019 • edited by kkjeer 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 Oct 11, 2019 • edited by kkjeer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkjeer commented Oct 18, 2019

Uh oh!

dtarditi left a comment

Choose a reason for hiding this comment

Uh oh!

dtarditi commented Oct 18, 2019

Uh oh!

Uh oh!

kkjeer commented Oct 11, 2019 •

edited

Loading

dtarditi left a comment •

edited

Loading

dtarditi Oct 11, 2019 •

edited by kkjeer

Loading

dtarditi Oct 11, 2019 •

edited by kkjeer

Loading

dtarditi Oct 11, 2019 •

edited by kkjeer

Loading

dtarditi Oct 11, 2019 •

edited by kkjeer

Loading

dtarditi Oct 11, 2019 •

edited by kkjeer

Loading