fixed some problems

update

added comments. added note tag, added test, improved the code

Use of addDeclToContexts, added new test.

@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".

Using one isa statement for all class names.
Changed the documentation.

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).

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?

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.

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.

Applied the review comments.

Rebase to newer 'main' branch.

Created a common place for the DeclContext child error handling code.

Removed a left comment and improved another comment.

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

I found only small issues.

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).

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.

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".

Additional documentation related fixes.

Applied the review proposals.
Changed "system call" to "standard function".
Put the string lists into constant std::initializer_list.

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.

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).

In D120824#3369407, @martong wrote:

So, my understanding is that the issue stems from the fact that hasInClassInitializer() gave inconsistent results with getInClassInitializer() for previously imported nodes.

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.

Improved the test to fix failure on Windows.

The current build errors are probably not related, the Windows test failure shows up in other differential objects.

Removed the static variable.

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.

Changed name of the checker to ErrnoModeling, other small cleanup.

Improved the tests.

Applied review comments, fixed the failing tests.

Addressed a part of the review comments.
The test checker is now a separate checker.
Initialization is simplified, checkASTDecl is used.

Newer revision: D118996

Documentation not added yet.

ping

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.

(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.