Fixes for test case failures after the merge to LLVM/Clang 12.x#1136
Closed
sulekhark wants to merge 146 commits intoupdated_baseline_master_12_checkptfrom
Closed
Fixes for test case failures after the merge to LLVM/Clang 12.x#1136sulekhark wants to merge 146 commits intoupdated_baseline_master_12_checkptfrom
sulekhark wants to merge 146 commits intoupdated_baseline_master_12_checkptfrom
Conversation
* split variable adder and constraint adder passes * populate typedef map during variable adder phase * avoid numparam crash * Add ReasonFailed to brainTransplant; Refactor insertNewFVConstraint * another assert * add tests proto vs body * formatting, clarity * remove xfail from tests, add grep * wording: 'new'->'seen' * more mergefailure reasons * remove calls to braintransplant (regression fail: 80) * split add variable phase at higher level * only save merged FVC (regression fail: 24) * make special case match description * remove code for braintransplant * restore the safety of PragramVariableAdder * first pass comments; assert for backwards merge * second comment pass - usage and defn * add test for #427, now solved
* Adding stats * Fixing
* Fix #373 * Use a single constraint variable to represent each typedef * Fix rewriting for array typedefs * Fix rewriting for function typedefs Co-authored-by: Matt McCutchen (Correct Computation) <matt@correctcomputation.com> Co-authored-by: John Kastner <john@correctcomputation.com>
Some other cleanup to CMake files while I'm here: - Formatting - target_include_directories with no include directories is a no-op.
* remove the last of deferred params code * add failed tests for merging funtion pointers
They were already also printed to TotalConstraintStats.json when -dump-stats is passed. Also add a regression test that 3C's stderr is empty by default on a successful run. Fixes #478.
* change assert to consistency fail(macro) * remove checked regions around inconsistent function calls; fix logic error * add macro check * don't add type params if they're all inconsistant * add test * remove commented code * allow existing macro type params to be valid * minor efficientcy * use more appropriate rewrite check
* Fixing minor and adding stats * Changing return value as a pair * Adding context sensitivity for struct-field access * Correct stats * Adding insrc flag * Adding flags for greatest and least solutions * Adding stats * Adding * Bounds not needed ntarrays * Minor * Handling comments and testcase * Fixing cmakelists * Minor fix
- Add `--build_dir` option to `convert_project` to specify a build directory (containing `compile_commands.json`) different from the source directory (which is scanned for source files to update includes). This is needed by LibArchive and ZLib, which currently set `--project_path` to the build directory, which breaks include updating. - Switch from `-output-postfix` to `-output-dir` to reduce the amount of code needed to move converted source files into place for the post-conversion build. Theoretically, this would break any caller that relies on the current location of the output files, but the current benchmark test workflow doesn't use the output files at all. - To make the output directory location more predictable, always set the 3C base directory equal to the `--project_path` specified to `convert_project` rather than the common ancestor directory of the source files in the compilation database. This will temporarily break LibArchive and ZLib even further until they are migrated to the new usage of the `--project_path` and `--build_dir` options. - Add `--extra-3c-arg` to `convert_project` that can be used to pass the `-alltypes` flag through to 3C. mwhicks1/3c-actions#7 will use the new features to improve the benchmark test workflow.
We've found it more distracting than helpful.
The constraint created between a function parameter and a corresponding argument is updated to be an equality constraint when the function call is inside a macro. Doing this forces the checked type of the parameter to the same as that of the argument. Casts are only inserted when parameter and argument types are not equal, so this changes ensures that 3C will not try (and subsequently fail) to insert casts on expressions in macros. After the previous change there were still issues with cast insertion due to source location collision in the ExprConstraintVars map. To avoid these problems the map has been updated to use the AST node ID instead of the source location. AST node IDs are only unique within a translation unit, so they are paired with the name of the main file for the translation unit. AST nodes IDs are consistent between multiple clang tool invocations,but they can change when run on different computers. http://clang-developers.42468.n3.nabble.com/Question-about-hashing-AST-nodes-td4063810.html#a4063828
Use of VarArg parameters are assumed to be unsafe even though CheckedC will accept them with checked pointer types. If we want to support VarArgs with checked pointer types, we can remove the constraint to WILD here. We would then need to update TypeExprRewriter to rewrite the type in these expression.
Rewriting K&R style functions now produces correct code (Fixes #93), but does not preserve the K&R style. This change also contains a reorganization of FunctionDeclReplacement::getSourceRange.
Before this change, types for function pointer parameters were generated from a QualType object even when the specific parameter was not changed by 3c. Now parameters that 3c does not convert will be rewritten using their original source representation. This fixes #484 by using va_list for the parameter (how it's written in the source) instead of expanding it to struct __va_list_tag *. As a side effect, this preserves formatting in function pointer types slightly better than before. This shows up mostly as spaces being deleted from our generated tests.
The code for deciding if a function should get an itype was duplicated for function declarations and function pointer types. The function pointer version of the code had a bug in it that caused issue #498. The duplicated code has been extracted into a pair of functions that are reused for functions and function pointers. A lot of the lines changed in this PR are caused by an EnvironmentMap& parameter being changed to Constraints&. This lets the function pointer code call solutionEqualTo which is needed for correct itype insertion.
* prototype without Diagnostics * enableSourceFileDiagnostics * only enable diags once per ast * add fail case * FileEntry path before CanonicalFilePath * Diagnostic errors cause non-zero exit code * setup multipass verifier * remove unused prefixes * delete unused code; create ASTs in seperate function * rework canonical file path * add test refactoring TODOs * :disable diag_verifier tests * Wording Co-authored-by: John Kastner <john@correctcomputation.com> * Slight improvements and comments for file path canonicalization (for possible addition to #488) (#508) * Slight improvements and comments for file path canonicalization. In particular, getCanonicalFilePath (the version that asserts) is no longer used. * Add comments about blocks around diagnostic generation. Co-authored-by: John Kastner <john@correctcomputation.com> Co-authored-by: Matt McCutchen (Correct Computation) <matt@correctcomputation.com>
I'm first merging with CCI's main branch before #494 because #494 made whitespace changes to a huge number of tests, and it will be easier to review if I deal with #494 separately. This is just a textual merge, and fortunately, the textually merged tests pass. (Not only that, but there are no diffs in any of the test-updating programs!) I'll leave all the formatting of tests that were added or changed on CCI's main branch until the end.
This consists mostly of re-running testgenerator.py to fix massive whitespace conflicts, plus straightforward textual merges of the remaining files. I had to manually fix two CHECK comments in fptr.c and hash.c where the first parent had the same good formatting as the common ancestor (because that version of 3C didn't have #494 and thus didn't preserve formatting) and #494 changed to the same bad formatting as the source, so the textual merge kept the bad formatting, but instead we need to match the source formatting that is now good. test_updater.py and processor.py have no diffs except in valist.c, which I'll address later.
- Run clang-format and apply exclusions as appropriate. - Manually fix an over-length comment.
This affected valist.c.
This was the only diff in the test-updating programs.
…20210330 Merge from Microsoft 2021-03-30 + format regression tests
PR #488 made `3c -verify` cover only the compiler diagnostics, but none of our regression tests actually use that functionality. Instead, one regression test (macro_function_call) used `3c -verify` to try to test the absence of 3C warnings, and we were unaware that the test wasn't testing what it was supposed to. I think it's best to make `3c -verify` an error for now so we don't make that mistake again.
* Allow addition and subtraction operators to be invertible for unchecked pointer types * Add tests for checked and unchecked pointer invertibility * Update equivalent expression tests to account for invertible unchecked pointer arithmetic
* Add NormalizeUtils.h and NormalizeUtils.cpp * Add NormalizeUtil::AddExprs helper method * Add NormalizeUtil::TransformAdditiveOp method * Fix typos * Add ExprCreatorUtil::CreateUnaryOperator method * Add NormalizeUtil::GetAdditionOperands helper method * Rename variable in NormalizeUtil::TransformSingleAdditiveOp * Add ExprUtil::EnsureEqualBitWidths method * Add NormalizeUtil::GetRHSConstant helper method * Add NormalizeUtil::TransformAssocLeft method * Add NormalizeUtil::ConstantFold method * Remove ConstantFoldUpperOffsets, GetRHSConstant, and EnsureEqualBitWidths methods from BaseRange and call utility methods instead * Fix typos * Avoid creating an unnecessary binary operator in TransformAdditiveOp * Return argument expression from TransformAssocLeft if the argument is already in the output form * Add NormalizeUpperBound method to CheckBoundsDeclarations * Add CompareNormalizeBounds method to CheckBoundsDeclarations * Remove expected warning from bounds widening test in bounds-context.c * Add tests for comparing normalized bounds to bounds-decl-checking.c * Move declaration of PointerAndConst * Add comment explaining why we don't check for B - P
…lang-tidy files in an optimized manner by caching previously read values. Valgrind detects the access of an uninitialized variable during this process, which causes this test case to fail. As this is a low priority issue, a temporary fix is made (described in the test case) and an issue will be filed.
To make it pass also on 32-bit Windows, see PR48920.
…1133) * Update TransformAssocLeft to take expressions of the form E1 + (E2 +/- E3) and output (E1 + E2) +/- E3 * Don't transform X - Y to X + -Y in NormalizeUpperBound
…ored into isIntegerConstantExpression (with a different signature) and getIntegerConstantExpression in LLVM/Clang 12.x.
We get rid of UnionGen and UnionKill sets to save memory consumed by the compiler. We also do not need to compute the Gen and Kill sets for blocks thus resulting in further space savings. Instead we now compute and use (but don't store) the Out sets of each statement in the fixpoint loop. For each block we only store the Out set of the last statement of the block which is used to comoute the Out set of the block.
Make add_clang_tool add a dependency on checkedc-headers.
* Add NormalizeUtils.h and NormalizeUtils.cpp * Add NormalizeUtil::AddExprs helper method * Add NormalizeUtil::TransformAdditiveOp method * Fix typos * Add ExprCreatorUtil::CreateUnaryOperator method * Add NormalizeUtil::GetAdditionOperands helper method * Rename variable in NormalizeUtil::TransformSingleAdditiveOp * Add ExprUtil::EnsureEqualBitWidths method * Add NormalizeUtil::GetRHSConstant helper method * Add NormalizeUtil::TransformAssocLeft method * Add NormalizeUtil::ConstantFold method * Remove ConstantFoldUpperOffsets, GetRHSConstant, and EnsureEqualBitWidths methods from BaseRange and call utility methods instead * Fix typos * Avoid creating an unnecessary binary operator in TransformAdditiveOp * Return argument expression from TransformAssocLeft if the argument is already in the output form * Add NormalizeUpperBound method to CheckBoundsDeclarations * Add CompareNormalizeBounds method to CheckBoundsDeclarations * Remove expected warning from bounds widening test in bounds-context.c * Add tests for comparing normalized bounds to bounds-decl-checking.c * Move declaration of PointerAndConst * Add comment explaining why we don't check for B - P * Add NormalizeUtil::QueryPointerAdditiveConstant helper method * Add NormalizeUtil::AddConstants helper method * Add NormalizeUtil::GetVariableAndConstant method * Call NormalizeUtil::GetVariableAndConstant from CheckBoundsDeclarations::CompareNormalizedBounds * Remove CheckBoundsDeclarations::NormalizeUpperBound method * Add and update bounds checking tests * Don't call TransformAdditiveOp from GetVariableAndConstant * Remove unused NormalizeUtil::GetAdditionOperands method * Fix formatting
#1137) An expression that modifies an LValue is said to be invertible w.r.t. the LValue if we can write an expression in terms of the original value of the LValue before the modification. For example, the expression x + 1 is invertible w.r.t x because we can write this expression in terms of the original value of x which is (x - 1) + 1. In this PR, we use invertibility of statements to support bounds widening in loops. More specifically, if a statement modifies a variable that occurs in the bounds expression of a null-terminated array then instead of killing its bounds at that statement we use invertibility of the statement to try to write the widened bounds in terms of the original value of the variable.
|
@sulekhark I reviewed the commits made by you to fix various issues with the merge of LLVM 12.x. The changes look good. Thanks. |
mgrang
approved these changes
Jul 27, 2021
Member
|
Nice work on the workaround for I assume you've already run the 3C regression tests, but I'll run some additional 3C and clangd tests so that if I find problems, we can discuss whether and how to fix them before the upcoming Checked C release. |
Contributor
Author
|
@mattmccutchen-cci , Thanks and yes, I have run
|
Member
|
Thanks for the information. I'll be happy to take your recommendation on which branch(es) to test. However, I noticed some issues with the branches that you might want to address first, since you'll probably want to address them before the final merge anyway to avoid surprises:
It seems this isn't quite true: Also, there are a few commits in Aside: I'd suggest that a more meaningful name for |
clang/lib/AST/NormalizeUtils.cpp
Outdated
Comment on lines
+197
to
+203
| Optional<llvm::APSInt> OptConstant = | ||
| E->getRHS()->getIntegerConstantExpr(S.Context); | ||
| if (!OptConstant) | ||
| return false; | ||
|
|
||
| bool Overflow; | ||
| Constant = ExprUtil::ConvertToSignedPointerWidth(S.Context, Constant, Overflow); |
Member
There was a problem hiding this comment.
When I fixed the build error I got elsewhere in this file during my test merge, I noticed a potential mistake here: I would expect this code to update Constant from OptConstant rather than using whatever value it contained before NormalizeUtil::GetRHSConstant was called. If this is indeed a mistake and the Checked C tests didn't catch it, it might make sense to add another test.
Contributor
Author
There was a problem hiding this comment.
Thanks for catching this! I will fix this locally and also add a test case.
And, yes, both updated_baseline_master_12 and updated_baseline_master_12_temp are a bit behind master (updated_baseline_master_12_temp more so). I plan merge master into these branches before proceeding further.
We use the CheckedCAnalysesPrepass.cpp to gather the checked scopes for statements. We store a map of statements to their checked scope specifiers. An entry in this map is only made for the following statements: 1. For the first non-compound statement of a compound statement. 2. For the first statement that follows a compound statement. We then use this info in the bounds widening analysis to determine the checked scope specifiers for each statement.
…APSInt> value that has to be dereferenced to extract the integer constant. Added two negative test cases to capture the impact of failing to extract the integer constant.
Comment on lines
+275
to
+282
| if (BO->getLHS()->getType()->isPointerType() && | ||
| BO->getRHS()->isIntegerConstantExpr(S.Context)) | ||
| PointerExpr = BO->getLHS(); | ||
| else if (BO->getOpcode() == BinaryOperatorKind::BO_Add && | ||
| BO->getRHS()->getType()->isPointerType() && | ||
| BO->getLHS()->isIntegerConstantExpr(S.Context)) | ||
| PointerExpr = BO->getRHS(); | ||
| else |
Member
There was a problem hiding this comment.
I see you fixed the same build error that I encountered when I tried to merge master and updated_baseline_master_12. But doesn't this need a getIntegerConstantExpr now that isIntegerConstantExpr no longer does that as a side effect?
Suggested change
| if (BO->getLHS()->getType()->isPointerType() && | |
| BO->getRHS()->isIntegerConstantExpr(S.Context)) | |
| PointerExpr = BO->getLHS(); | |
| else if (BO->getOpcode() == BinaryOperatorKind::BO_Add && | |
| BO->getRHS()->getType()->isPointerType() && | |
| BO->getLHS()->isIntegerConstantExpr(S.Context)) | |
| PointerExpr = BO->getRHS(); | |
| else | |
| if (BO->getLHS()->getType()->isPointerType() && | |
| BO->getRHS()->isIntegerConstantExpr(S.Context)) { | |
| PointerExpr = BO->getLHS(); | |
| Constant = *BO->getRHS()->getIntegerConstantExpr(S.Context); | |
| } else if (BO->getOpcode() == BinaryOperatorKind::BO_Add && | |
| BO->getRHS()->getType()->isPointerType() && | |
| BO->getLHS()->isIntegerConstantExpr(S.Context)) { | |
| PointerExpr = BO->getRHS(); | |
| Constant = *BO->getLHS()->getIntegerConstantExpr(S.Context); | |
| } else |
As with the NormalizeUtil::GetRHSConstant bug, if this wasn't caught by the existing Checked C tests, you might want to add more tests.
Contributor
Author
There was a problem hiding this comment.
@mattmccutchen-cci , thanks! There were test case failures and I was looking into those.
At this point, all features that were planned for the current release are on master. So, now I will work on finalizing the merge and the release (the released Checked C compiler will be based on LLVM 12.0.1). If you are still having merge issues/test failures with updated_baseline_master_12 or updated_baseline_master_12_temp, I propose that you wait until the release, so that you don't have to work with branches that are behind master.
Member
|
I ran CCI's additional 3C and clangd tests on the current |
…d initialize the out parameter Constant using OptConstant.
Contributor
Author
|
Closing this PR as the changes related to LLVM/Clang 12 upgrade have been merged into the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR shows the diffs of the branch
updated_baseline_master_12with respect to the branchupdated_baseline_master_12_checkpt. It is created only to enable the review of the fixes for the test failures due to merge, without cluttering the PR with LLVM 12.x changes. After the review, the branchupdated_baseline_master_12will be merged tomasterof thecheckedc-clangrepo. (Note: Certain commits like the commits related to the 3C merge and other commits on themasterbranch will have to be ignored during the review.)The
updated_baseline_master_12_checkptof thecheckedc-clangrepository is at:mainbranch of the upstream LLVM/clang repository withmasterbranch (as of mid-June) ofcheckedc-clangrepo merged into it, plusThe
updated_baseline_master_12of thecheckedc-clangrepository is at:mainbranch of the upstream LLVM/clang repository withmasterbranch (current) ofcheckedc-clangrepo merged into it, plus