User Details
- User Since
- May 23 2018, 7:08 AM (209 w, 2 d)
Wed, May 25
fixed some problems
update
Tue, May 24
added comments. added note tag, added test, improved the code
Fri, May 20
Use of addDeclToContexts, added new test.
Thu, May 19
Mon, May 16
@martong Do you mean that a "describe" function is needed for the return value constraint of the function and for the errno constraint type? Then a note tag can contain what the function is assumed to return on success and what is allowed (or not) to do with errno. For example: "Assuming that 'mkdir' is successful, it returns zero in this case and value of 'errno' is unspecified after the call".
Sun, May 15
Fri, May 13
Using one isa statement for all class names.
Changed the documentation.
Thu, May 12
Function mkdir is modeled incorrectly by the checker. According to the man page it can return 0 or -1 only (-1 is error) but the checker allows non-negative value at success. So the shown bug report is incorrect (it can be only -1 if not 0 and then check of errno is allowed). Anyway the note tags should be added to every function.
LLDB should be at least compilable after the change. If it has an internal ImportError we must not change it. If it uses the renamed class we should rename it, or make an "using" alias and no rename (but not this is the best solution).
Wed, May 11
Is it problem if markNotInteresting is used on a symbol that was marked interesting and has subregions?
I found one other place in LibASTImporter.rst where ImportError is used. LLDB should be checked too.
Initially the ImportError was very related to ASTImporter but now the class is used at other less related places and files. ImportError is a too generic name for this unless it can be used at any other "import" in clang which is not the case (the class can be extended with other special data about the import error). Maybe the whole ASTImporter should go into a new namespace, specially if ASTImporter.cpp would be split into parts?
Thu, May 5
The code looks correct but the diff is not OK. I think that you have separate commits for every change (instead of changing the first with git commit --amend when a new change is made). In this case arc diff --update <ID> main may work (if you have a separate branch for the changes). (I did not found a place in LLVM docs where this part of the process is exactly described.)
It looks mostly good now, apart from the line 1 comment problem. Usually in a review all phabricator diffs are created against the original commit or at least against an existing (in the current github repository) commit, here the last diff seems to be against an other commit. The next diff should be based on an existing commit (can be same as the original base). Otherwise the arc patch command may not work (I do not know if you use it) and it is difficult to compare changes related to the original code.
Wed, May 4
Mon, May 2
A better title: "Move clang::ImportError into own header and rename it (NFC)."
The ASTImporterLookupTable was not dependent on the ImportError so the header should not be included in that file. It is better in ASTImporterSharedState.h. Alternative solution is to add class ASTImportError into ASTImporterSharedState.h (no new header is needed) and use this file from ASTImporter.h.
Thu, Apr 28
Applied the review comments.
Apr 13 2022
Rebase to newer 'main' branch.
Apr 12 2022
Created a common place for the DeclContext child error handling code.
Apr 11 2022
Apr 8 2022
Apr 7 2022
Apr 6 2022
Removed a left comment and improved another comment.
Mar 29 2022
Code is updated, documentation (rst) not yet. I want to use \ format of tags in the code comments. Many review comments are now out of place (because the list of functions was moved around in the file.)
rebase, applied the review comments
Mar 28 2022
I found only small issues.
Mar 26 2022
I wanted to remove this one instance of consumeError because the change in D122525, to ensure that consumeError is called really only from ImportDeclContext (and other places where it is not relevant).
Mar 25 2022
This errno check will work if a state split is added at every standard function that may set errno at failure. (The success and failure branches have different errno check state, and probably different return value for the function.) Probably this is too many state splits. But a correct program should have anyway checks for the return value that make the same state splits.
Address review comments.
- Removed caching of errno-related values from the checker.
- Added note tags.
- Added documentation comments.
Mar 22 2022
Ping
I had a question about if the lit-test is needed with the new unit tests (that contain essentially similar code). But if there is no answer I will commit it with all tests, more test is always better.
This checker is made to add a partial support for CERT rule ERR30-C . One part of the rule is "check errno only after the function returns a value indicating failure".
Mar 21 2022
Mar 18 2022
Additional documentation related fixes.
Applied the review proposals.
Changed "system call" to "standard function".
Put the string lists into constant std::initializer_list.
Mar 11 2022
The last unit test is not much different from the lit test, I could not find more simple code to get the wanted result. Is it still good to have the lit test too?
Added unit tests.
Mar 9 2022
I think that the problem may be related to the fact that the in-class initializer is not used by the code in the "To" AST (in ctu-cxxdefaultinitexpr.cpp the problem is with QMultiHash::d field). Probably the expression node in the field is set only if it is used by any CXXDefaultInitExpr, but the hasInClassInitializer value is set always if the code contains in-class initializer. I can try to make a unit test for this case (but the lit test is good to have because it is more complex and can reveal other problems).
Not really the API is the problem. The real problem was that to create a CXXDefaultInitExpr the field should have a "in-class initializer". CXXDefaultInitExpr has a pointer to the field that is initialized at that place. The field has an "in-class initializer", this is the used expression to initialize the field (CXXDefaultInitExpr is a separate object that is replicated for every initialization in constructors and initializer-list). The in-class initializer expression is not always stored in the AST, in the ToTU it is missing initially. The field has the flag set that it contains in-class initializer but the actual expression is not set yet (probably because the code parser works this way). This expression should be imported before a CXXDefaultInitExpr can be created.
Mar 8 2022
Mar 3 2022
Improved the test to fix failure on Windows.
Mar 2 2022
Feb 28 2022
The current build errors are probably not related, the Windows test failure shows up in other differential objects.
Removed the static variable.
Feb 25 2022
Another try to fix the test failures, rebased to current main.
The "hip-link-bundle-archive" test looks really unrelated, the others are fixed if we go back to -DERRNO_VAR (no " characters in command line, and probably / does not work too). There are Debian build errors but these look unrelated(?).
Rename of "Errno.h", maybe fixes Windows build problem.
Fixed remaining problems.
Removed isErrnoAvailable, added test for getErrnoValue.
Feb 24 2022
Changed name of the checker to ErrnoModeling, other small cleanup.
Improved the tests.
Applied review comments, fixed the failing tests.
Feb 23 2022
Addressed a part of the review comments.
The test checker is now a separate checker.
Initialization is simplified, checkASTDecl is used.
Feb 22 2022
Newer revision: D118996
Documentation not added yet.
Feb 15 2022
ping
Feb 13 2022
Feb 11 2022
Rebase.
Now it would be better to have a checker called UnsafeFunctionsCheck (probably in bugprone) and add the cert checkers "msc24-c" and "msc33-c" as aliases. This makes the check extendable if more (CERT rule related or not) cases for unsafe functions are added.
Fix of test failures.
Feb 8 2022
(The current code does not work, the mentioned fixes must be applied first.)
@martong Ping
We do not have possibility currently to check all LLDB tests. I think we can commit this and observe the buildbots for failure.