From ba383c70f0a105d348ce07c13c93cb53a24dfb45 Mon Sep 17 00:00:00 2001 From: Mandeep Singh Grang Date: Tue, 17 Nov 2020 14:28:45 -0800 Subject: [PATCH] Compare Expressions: Fix normalizing exprs by adding zero Previously, in order to normalize exprs we added zero only to exprs not containing any other constants. Also, for binary nodes with + operator we added +0 and for nodes with * operator we added *1. This resulted in exprs like "p - i" which do not contain constants but also do not have + or * operator to not get normalized. So we simply the logic to add zero. Now we simply add a binary node with the + operator and a 0 child at the root of the AST. This makes the logic simpler and handles a wider variety of cases. This fixes issue #933 --- clang/include/clang/AST/PreorderAST.h | 4 - clang/lib/AST/PreorderAST.cpp | 80 +++++-------------- .../widened-bounds-multiple-derefs.c | 2 +- 3 files changed, 20 insertions(+), 66 deletions(-) diff --git a/clang/include/clang/AST/PreorderAST.h b/clang/include/clang/AST/PreorderAST.h index c8c90a6d4027..364251fb3335 100644 --- a/clang/include/clang/AST/PreorderAST.h +++ b/clang/include/clang/AST/PreorderAST.h @@ -118,10 +118,6 @@ namespace clang { // this to control when to stop recursive constant folding. void ConstantFold(Node *N, bool &Changed); - // Normalize expressions which do not have any integer constants. - // @param[in] N is current node of the AST. Initial value is Root. - void NormalizeExprsWithoutConst(Node *N); - // Get the deref offset from the DerefExpr. The offset represents the // possible amount by which the bounds of an ntptr could be widened. // @param[in] UpperExpr is the upper bounds expr for the ntptr. diff --git a/clang/lib/AST/PreorderAST.cpp b/clang/lib/AST/PreorderAST.cpp index 83c6fecbd1df..4703e1e6444e 100644 --- a/clang/lib/AST/PreorderAST.cpp +++ b/clang/lib/AST/PreorderAST.cpp @@ -82,7 +82,25 @@ void PreorderAST::Create(Expr *E, Node *Parent) { E = Lex.IgnoreValuePreservingOperations(Ctx, E->IgnoreParens()); - if (const auto *BO = dyn_cast(E)) { + if (!Parent) { + // The invariant is that the root node must be a BinaryNode with an + // addition operator. So for expressions like "if (*p)", we don't have a + // BinaryOperator. So when we enter this function there is no root and the + // parent is null. So we create a new BinaryNode with + as the operator and + // add 0 as a LeafNodeExpr child of this BinaryNode. This helps us compare + // expressions like "p" and "p + 1" by normalizing "p" to "p + 0". + + auto *N = new BinaryNode(BO_Add, Parent); + AddNode(N, Parent); + + llvm::APInt Zero(Ctx.getTargetInfo().getIntWidth(), 0); + auto *ZeroLiteral = new (Ctx) IntegerLiteral(Ctx, Zero, Ctx.IntTy, + SourceLocation()); + auto *L = new LeafExprNode(ZeroLiteral, N); + AddNode(L, /*Parent*/ N); + Create(E, /*Parent*/ N); + + } else if (const auto *BO = dyn_cast(E)) { BinaryOperator::Opcode BinOp = BO->getOpcode(); Expr *LHS = BO->getLHS(); Expr *RHS = BO->getRHS(); @@ -130,19 +148,6 @@ void PreorderAST::Create(Expr *E, Node *Parent) { Create(LHS, /*Parent*/ N); Create(RHS, /*Parent*/ N); - } else if (!Parent) { - // The invariant is that the root node must be a BinaryNode. So for - // expressions like "if (*p)", we don't have a BinaryOperator. So when we - // enter this function there is no root and the parent is null. So we - // create a new BinaryNode with + as the operator and add "p" as a - // LeafNodeExpr child of this BinaryNode. Later, in the function - // NormalizeExprsWithoutConst we normalize "p" to "p + 0" by adding 0 as a - // sibling of "p". - - auto *N = new BinaryNode(BO_Add, Parent); - AddNode(N, Parent); - Create(E, /*Parent*/ N); - } else { auto *N = new LeafExprNode(E, Parent); AddNode(N, Parent); @@ -331,52 +336,6 @@ void PreorderAST::ConstantFold(Node *N, bool &Changed) { Changed = true; } -void PreorderAST::NormalizeExprsWithoutConst(Node *N) { - // Consider the following case: - // Upper bound expr: p - // Deref expr: p + 1 - // In this case, we would not able able to extract the offset from the deref - // expression because the upper bound expression does not contain a constant. - // This is because the node-by-node comparison of the two expressions would - // fail. So we require that expressions be of the form "(variable + constant)". - // So, we normalize expressions by adding an integer constant to expressions - // which do not have one. For example: - // p ==> (p + 0) - // (p + i) ==> (p + i + 0) - // (p * i) ==> (p * i * 1) - - auto *B = dyn_cast_or_null(N); - if (!B) - return; - - for (auto *Child : B->Children) { - // Recursively normalize constants in the children of a BinaryNode. - if (isa(Child)) - NormalizeExprsWithoutConst(Child); - - else if (auto *ChildLeafNode = dyn_cast(Child)) { - if (ChildLeafNode->E->isIntegerConstantExpr(Ctx)) - return; - } - } - - llvm::APInt IntConst; - switch(B->Opc) { - default: return; - case BO_Add: - IntConst = llvm::APInt(Ctx.getTargetInfo().getIntWidth(), 0); - break; - case BO_Mul: - IntConst = llvm::APInt(Ctx.getTargetInfo().getIntWidth(), 1); - break; - } - - auto *IntLiteral = new (Ctx) IntegerLiteral(Ctx, IntConst, Ctx.IntTy, - SourceLocation()); - auto *L = new LeafExprNode(IntLiteral, B); - AddNode(L, B); -} - bool PreorderAST::GetDerefOffset(Node *UpperNode, Node *DerefNode, llvm::APSInt &Offset) { // Extract the offset by which a pointer is dereferenced. For the pointer we @@ -514,7 +473,6 @@ void PreorderAST::Normalize() { ConstantFold(Root, Changed); if (Error) break; - NormalizeExprsWithoutConst(Root); } if (Ctx.getLangOpts().DumpPreorderAST) { diff --git a/clang/test/CheckedC/inferred-bounds/widened-bounds-multiple-derefs.c b/clang/test/CheckedC/inferred-bounds/widened-bounds-multiple-derefs.c index b931abbfc0ce..05d219903dda 100644 --- a/clang/test/CheckedC/inferred-bounds/widened-bounds-multiple-derefs.c +++ b/clang/test/CheckedC/inferred-bounds/widened-bounds-multiple-derefs.c @@ -328,7 +328,7 @@ void f15(int i) { // CHECK: 1: *(p - i + 1) // CHECK: upper_bound(p) = 1 // CHECK: [B11] -// CHECK: upper_bound(p) = 1 +// CHECK: upper_bound(p) = 2 _Nt_array_ptr q : count(0) = "a"; if (*q && *(q - 1))