-
Notifications
You must be signed in to change notification settings - Fork 72
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
Available facts analysis #1152
Available facts analysis #1152
Conversation
We introduce a 2-bit field called CheckedScopeSpecifier in the Stmt class. During parsing when a compound statement is created we iterate the elements (statements) of the compound statement and set the checked scope specifier for each element to the checked scope specifier of the compound statement. We can get the checked scope specifier for a statement by calling the getCheckedScopeSpecifier method on the statement.
* Updated the instructions for upgrade of LLVM/Clang. Also added a new file LLVM-Upgrade-Notes.md to track important information related to upgrades. * Fixed typos. * Addressed review comments. * Fixed an inadvertent deletion. * Addressed review comments. * Incorporated review comments. * Fixed minor typos. * Fixed typos.
Add InferredFact and adjust WhereClauseFact to be a subclass of AbstractFact
`IsSwitchCaseBlock`: use `dyn_cast_or_null` to cover the null pointer case. `ConditionOnEdge`: do not test if there is no edge between pred to curr since it will only be called if there is an edge. `GetModifiedVars`: use `TranspareCasts` to bypass some casting. The feature to deal with membership access and the array indexing is still TODO.
from the previous available facts analysis)
It also fixes a bug to visit dead blocks.
The community has introduced a new annotation called "contains-errors" on AST nodes that contain semantic errors. As a result, after the upgrade of Checked C sources to LLVM 12 we need to check if an expr contains errors before operating on the expr. One such place is in InverseUtil::IsInvertible where we need to check if the input modifying expr contains errors. * Added containsErrors checks to InverUtil::Inverse
Support bounds widening in presence of complex conditionals like: "if (*p != 0)", "if ((c = *p) == 'a')", etc.
SemaRef(SemaRef), Cfg(Cfg), Ctx(Ctx), Lex(Lex), OS(llvm::outs()), | ||
FactComparisionMap(FactComparisionMapTy()) {} | ||
|
||
// Pretty print a expr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested comment change: Pretty print a expr
=> Pretty print an expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't mind my snooping on PRs in your repository unrelated to 3C: The GitHub "suggested change" feature (the "±" icon in the toolbar when writing a review comment) might provide a more streamlined process for changes like this. (GitHub's documentation is in item 6 here.) An example of what it looks like:
// Pretty print a expr | |
// Pretty print an expression. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you all. I will reword this and I learn the new GitHub primitive!
// Pretty print a expr | ||
void Print(const Expr *) const; | ||
|
||
// Pretty print a Stmt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested comment change: Pretty print a Stmt
=> Pretty print a statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
// Pretty print a Stmt | ||
void Print(const Stmt *) const; | ||
|
||
// Pretty print an abstract fact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested comment change Pretty print an abstract fact
=> Pretty print an abstract fact.
// Pretty print an abstract fact | ||
void DumpAbstractFact(const AbstractFact *Fact) const; | ||
|
||
// Pretty print a list of abstract facts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested comment change: Pretty print a list of abstract facts
=> Pretty print a list of abstract facts.
// Pretty print a set of variables. | ||
void PrintKillVarSet(KillVarSetTy VarSet) const; | ||
|
||
// Determine if the edge from PredBlock to CurrBlock is a fallthrough. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: add a space before // Determine if...
// Determine if a variable is used in a fact. | ||
bool IsVarInFact(const AbstractFact *Fact, const VarDecl *Var) const; | ||
|
||
// Determine if two facts equal. First check if the comparision is check before, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: two facts equal
=> two facts are equal
, comparision
=> comparison
, is check before
=> is checked before
for (auto *Fact : WC->getFacts()) { | ||
if (auto *BF = dyn_cast<BoundsDeclFact>(Fact)) { | ||
// ignore the result value here, | ||
// just take the side-effect to save the normalzied bounds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
normalzied
=> normalized
} | ||
} | ||
|
||
// Initiazlize Gen and Out of a block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initiazlize
=> Initialize
using OrderedBlocksTy = std::vector<const CFGBlock *>; | ||
|
||
// AvailableFactsKillKind denotes two kinds of kill variables. | ||
// KillExpr denotes a variable to kill a EqualityOpFact or a InferredFact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe these could be rephrased as:
// KillExpr denotes a variable that kills an EqualityOpFact or an InferredFact
// KillBounds denotes a variable that kills a BoundsDeclFact.
// AvailableFactsKillKind denotes two kinds of kill variables. | ||
// KillExpr denotes a variable to kill a EqualityOpFact or a InferredFact | ||
// KillBounds denotes a variable to kill a BoundsDeclFact. | ||
enum AvailableFactsKillKind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to declare this as an enum class
to possibly avoid future conflicts of KillExpr
and KillBounds
with variables of the same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see!
using StmtKillVarSetTy = llvm::DenseMap<const Stmt *, KillVarSetTy>; | ||
|
||
// FactComparisionMapTy denotes the comparision result of two facts. | ||
using FactComparision = std::pair<const AbstractFact *, const AbstractFact *>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typos in comment: FactComparisionMapTy denotes the comparision result of two facts.
--> FactComparisonMapTy denotes the comparison result of two facts.
Typo in variable name: FactComparision
--> FactComparison
bool IsSwitchCaseBlock(const CFGBlock *PredBlock, | ||
const CFGBlock *CurrBlock) const; | ||
|
||
// Determine the boolean state of an edge when the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems incomplete.
|
||
const CFGBlock *Block; | ||
|
||
// Block-wise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Period (.)
needed after each comment.
void DumpAvailableFacts(FunctionDecl *FD); | ||
|
||
private: | ||
void AddSuccsToWorkList(const CFGBlock *CurrBlock, WorkListTy &WorkList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments needed for the function.
void ComputeStmtGenKillSets(ElevatedCFGBlock *EB, const Stmt *CurrStmt, | ||
StmtSetTy NestedStmts); | ||
|
||
void CollectFactsInDecl(AbstractFactListTy &Gen, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments needed for function.
// This file implements a dataflow analysis for available facts analysis. | ||
//===---------------------------------------------------------------------===// | ||
|
||
#include "clang/AST/ExprUtils.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please order the includes alphabetically.
EB->Kill = EB->StmtKill[CurrStmt]; | ||
EB->GenAllSucc = EB->StmtGen[CurrStmt]; | ||
} else { | ||
EB->Kill = AFUtil.Union(EB->Kill, EB->StmtKill[CurrStmt]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be costly (in terms of space) to persist the accumulated Kill and StmtGen sets. Is it possible to locally compute them when needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you are right. The accumulated Kill is one per block which may not be large, but definitely StmtGem
is costly. That is for debugging originally.
… 1 in TargetSrcEquality (#1162) * Add AllowTempEquality parameter to RecordEqualityWithTarget * Use a ModifiedSameValue variable to determine the return value for UpdateSameValueAfterAssignment * Rename ModifiedSameValue to RemovedAnyExprs and clean up comments
…nces (#1163) * In CheckAddressOfOperand, add case for address-of array subscripts to C99-specific logic * Move address-of array subscript check after other checks such as taking the address of an lvalue * Adjust expected AST output to account for different types of address-of array subscripts * Restore deleted comment about checking for array subscript expressions * Add comment explaining the placement of the address-of array subscript logic * Put &e1[e2] typing rules under a Checked C flag
I have fixed the issues you mentioned (sorry for so many typos). |
5bae193
to
4f8aa5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you very much for this work!
I have fixed the issues in our previous code review.