Add a checker to maintain the system-defined value 'errno'.
The value is supposed to be set in the future by existing or
new checkers that evaluate errno-modifying function calls.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Good start!
However, I'm not a big fan of coupling the testing checker with the actual modeling checker.
IMO we should have a distinct checker, similarly to TaintTester. That way you could do even fancier things like:
define mylib_may_fail(), bifurcate and return true for the success case, on which it wouldn't touch errno; and on the failure case return false and set the errno to some concrete value that we could check against.
But I'm also fine with the current approach, providing the set_errno(SVal) introspection function.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h | ||
---|---|---|
767–769 | At this point, I'm not even sure if we should assert any of these. | |
1378 | ||
clang/lib/StaticAnalyzer/Checkers/Errno.h | ||
22 ↗ | (On Diff #410471) | I think we can settle on something better. What about calling it simply errno? |
24–26 ↗ | (On Diff #410471) | I don't think we need this. THat being said, only a top-level Check::BeginFunction callback could see an 'errno' uninitialized, which I don't think is a real issue. Having to check isErrnoAvailable() would be really an anti-pattern. |
clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp | ||
32 ↗ | (On Diff #410471) | Why don't you use StringRef here as well? |
35 ↗ | (On Diff #410471) | compiler-rt/lib/sanitizer_common/sanitizer_errno.h has a list of the possible names for this API.: That being said, you should eval::Call all of these, and bind the return value to the memorized errno region. |
60 ↗ | (On Diff #410471) | For function definitions, we should use static instead of anonymous namespaces. |
66 ↗ | (On Diff #410471) | I would expect a comment stating that:
|
88 ↗ | (On Diff #410471) | I think you can hoist these lambdas into static functions. |
99 ↗ | (On Diff #410471) | I don't think any of these actually return the canonic versions. |
118 ↗ | (On Diff #410471) | Ah, I've seen and done these reinterpret casts. |
134 ↗ | (On Diff #410471) | |
146–149 ↗ | (On Diff #410471) | You should sink these lines to the corresponding branches. |
180–181 ↗ | (On Diff #410471) | Fuse these two lines. |
clang/test/Analysis/Inputs/errno_var.h | ||
4 | ||
clang/test/Analysis/errno.c | ||
23 | Please call a different function, which is definitely not from glibc. | |
clang/test/Analysis/global-region-invalidation.c | ||
12 | If you were eval::Calling the errno_location functions, and used the corresponding header, would the tests pass? |
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h | ||
---|---|---|
767–769 | I mean, ideally this should be just UnknownSpaceRegion. For everything else we should have made a fresh MemRegion class. It is absurd that the same numeric address value (represented the symbol s) can correspond to two (now three) different locations in memory. | |
clang/lib/StaticAnalyzer/Checkers/Errno.h | ||
24–26 ↗ | (On Diff #410471) | UnknownVal implies that it's an actual value but we don't know which one. If the value doesn't exist we shouldn't use it. And if the user doesn't include the appropriate header then the value really doesn't exist in the translation unit. So flavor-wise I think we shouldn't use UnknownVal here; I'd rather have an Optional<SVal>. Other than that, yeah, I think this is a good suggestion. |
clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp | ||
32 ↗ | (On Diff #410471) | There's that rule against static constructors/destructors. We should use a lot of constexpr in these situations and/or try to turn these structures into plain C arrays as much as possible. |
66 ↗ | (On Diff #410471) | Ok so the first part of the function (identifying how the system headers represent errno) depends entirely on the AST right? In this case you can probably perform the AST scan only once during checkASTDecl<TranslationUnitDecl> and stash the result in a global variable. It shouldn't be re-run on every analysis. So whatever you do with isErrnoAvailable(), it shouldn't accept ProgramStateRef at all, it can simply query that global variable. You'll still need the state trait because SVal objects only make sense within a single analysis but the AST scan can be made only once. |
134 ↗ | (On Diff #410471) | This is literally the first step of the analysis. The block count is known. What I really want to see here is a non-trivial custom tag (i.e., the const void *symbolTag parameter on some of the overloads of conjureSymbolVal()) to make sure this symbol isn't treated as a duplicate of any other symbol some other checker conjures initially. Like all tags, you can initialize it with some address of some checker-static variable. |
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h | ||
---|---|---|
767–769 | From my perspective, a symbolic region is just a region that we don't know what its parent region is - let it be part of an object or an entire memory space.
IMO a given s should indeed refer to a single location in memory, but that's not a property we can enforce here in the constructor. | |
clang/lib/StaticAnalyzer/Checkers/Errno.h | ||
24–26 ↗ | (On Diff #410471) | Yeah, probably the Optional is a better alternative. However, I'd like to explain my reasoning behind my previous suggestion: My point is, that we shouldn't regress our API for a marginal problem. And according to my reasoning, this seems to be a marginal issue. |
clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp | ||
32 ↗ | (On Diff #410471) | Ah, I see, thanks! |
134 ↗ | (On Diff #410471) | I would still use the C.blockCount() to make it more in-line with the rest of the uses of conjureSymbol(). |
Addressed a part of the review comments.
The test checker is now a separate checker.
Initialization is simplified, checkASTDecl is used.
clang/lib/StaticAnalyzer/Checkers/Errno.h | ||
---|---|---|
22 ↗ | (On Diff #410471) | Just errno may not work because it collides with the "real" errno (that is a macro). |
24–26 ↗ | (On Diff #410471) | These work now if no ErrnoRegion is available. Returning Optional seems not better than a required check before the call. I think the current version is not the best: It can be possible to make assumptions using assume on the returned SVal value, but this must not be done on a non-existing errno value. For this case probably isErrnoAvailable is required to be used. The isErrnoAvailable can be useful for a checker that does special things only if there is a working errno modeling. |
clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp | ||
35 ↗ | (On Diff #410471) | The list is needed to build CallDescriptions and to lookup the errno location function. A CallDescriptionSet can not be used to get the function names from it. Is there a way to build a initializer_list of CallDescription objects in compile time (from a string array)? |
99 ↗ | (On Diff #410471) | ACtx.IntTy and value from getPointerType should be canonical (it is a CanQualType). |
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td | ||
---|---|---|
351–353 | I think ErrnoModeling would be more appropriate. | |
1564–1567 | If this is the case, it should have a Dependency<Errno> | |
clang/lib/StaticAnalyzer/Checkers/Errno.h | ||
22 ↗ | (On Diff #410471) | Ah, I see. Ugly macros! |
clang/lib/StaticAnalyzer/Core/MemRegion.cpp | ||
1155–1156 | I would recommend removing this comment alltogether. | |
clang/test/Analysis/errno.c | ||
16–22 | Fun fact, I think you can #include ERRNO_HEADER, if you pass the -DERRNO_HEADER=\"Inputs/errno_var.h\" :D |
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h | ||
---|---|---|
767–769 |
I agree that it's a constraint. And as such, I believe it shouldn't be made part of the region's identity. Just like "$x < 5" is not a field inside symbol $x but a separate state trait. Similarly we can turn SymbolicRegion's memory space constraint into a state trait. Then we can express more sophisticated constraints ("this symbolic region is either on the heap or a global variable but definitely not a local variable"), or we can have constraints become more precise over time ("this symbolic region was compared as "less than" another heap region, therefore it itself must be on the heap").
This could be a nice temporary solution. |
clang/test/Analysis/global-region-invalidation.c | ||
---|---|---|
12 | It is now executed wit both versions. |
The patch looks great to me now. As soon as you address comments about isErrnoAvailable() (at least, the parameter can now be removed), I think you can commit.
clang/lib/StaticAnalyzer/Checkers/Errno.h | ||
---|---|---|
22 ↗ | (On Diff #410471) | It is better to have separate namespaces for the different inter-checker API's. Probably more functions will be added later to the errno_modeling. |
24–26 ↗ | (On Diff #410471) | I have removed now isErrnoAvailable and the get function returns Optional. I am not sure if this is the best option, if more functions will be added (to get the errno location, and get and set an "errno state"). |
Hello! I had to revert in f9e8e92cf586d9cf077139a338f062a91b8ce2d2 because this broke all of the builds on Windows. Here are some sample failures:
https://lab.llvm.org/buildbot/#/builders/86/builds/30183
https://lab.llvm.org/buildbot/#/builders/216/builds/488
according to the pre-merge checks it still fails on windows and Debian.
https://buildkite.com/llvm-project/premerge-checks/builds/81130#ce8f3062-699e-4043-aed9-b891ec8ebee6
https://buildkite.com/llvm-project/premerge-checks/builds/81130#53beb4a2-2c53-4ff0-b60f-6130ed5d25cd
******************** Failed Tests (3): Clang :: Analysis/errno.c Clang :: Analysis/global-region-invalidation.c Clang :: Driver/hip-link-bundle-archive.hip Testing Time: 836.49s Skipped : 3 Unsupported : 201 Passed : 29682 Expectedly Failed: 31 Failed : 3
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(?).
I think its the forward slashes.
We could workaround the issue by adding this line to the test files: // REQUIRES: system-linux
The current build errors are probably not related, the Windows test failure shows up in other differential objects.
I think ErrnoModeling would be more appropriate.