This is an archive of the discontinued LLVM Phabricator instance.

[clang][analyzer] Add stream related functions to StdLibraryFunctionsChecker.
ClosedPublic

Authored by balazske on Dec 20 2022, 4:11 AM.

Details

Summary

Additional stream handling functions are added.
These are partially evaluated by StreamChecker, result of the addition is
check for more preconditions and construction of success and failure branches
with specific errno handling.

Diff Detail

Event Timeline

balazske created this revision.Dec 20 2022, 4:11 AM
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
balazske requested review of this revision.Dec 20 2022, 4:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2022, 4:11 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Some of the changes are also present in D135247. I suppose you're in the middle of splitting those patches apart and remaking the patch stack?

This patch and D140395 is (almost) the same code as D135360 and D135247. The changes are separated for the different checkers. Tests are added at the second patch.

This patch and D140395 is (almost) the same code as D135360 and D135247. The changes are separated for the different checkers. Tests are added at the second patch.

Well, that is confusing. Which patches should I review? Can you please only have a single opened differential per change, and a single patch stack? If you decided that this patch is the ONE for the ErrnoModeling changes, please abandon the other patch with the very same change.

Would be possible to test the errno specific changes as well?

clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
310

When can this occur? Maybe we can turn this early return to an assert -- when ModelPOSIX is true, this mustn't be null (if what I'm saying is correct).

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
800

Just thinking aloud, no changes required here necesserily -- in most places, we use a combination of SmallString and raw_svector_ostream so construct strings, but I can't find anything set in stone about the practice (mostly looking at LLVM's Programmers Manual). Isn't this just good enough? Is llvm::Twine worth the hassle?

1636–1648

If I undetstand correctly, these 3 cases will cause the current path of execution to split into 3. I vaguely recall some arguments against this, but that's been a while, did the stance on this change? I see a number of already commited summaries with 3 cases, so this could be fine for all I know.

1650–1652

Maybe StreamChecker could take over the handling of that case? Seems like it also warns about the nullness of the first fread parameter.

1793–1794

Sure, but what should be fixed? We should check whether the the last argument is a SEEK_* value?

balazske added inline comments.Jan 2 2023, 7:30 AM
clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
310

I am not totally sure that errno is found always (definition of errno is encountered) if a summary for a standard function is added. The standard function in itself must be there, otherwise no summary is added and this function should not be called. But definition of errno may reside in other header (errno.h) that is probably not included in the translation unit. If the errno definition is not found the ErrnoRegion is not set (the checker may still turned be on but does nothing, but these functions are allowed to be called). Existence of errno is not even dependent on ModelPOSIX, errno is defined in the (non POSIX) C standard.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
800

The Twine may work is all partial strings are constant pointers, this is not true here. The getArgDesc does not return a constant string, the returned value must be copied. Still it may work if one single expression is used to construct the string.

1636–1648

The problematic case is if the function is not evaluated by StreamChecker, otherwise StreamChecker already sets the constraints on the return value and argument and some of the cases here should become unsatisfied. It may be known about the argument 1 that it can not be zero (or is only zero), then it is only 2 possible branches. (Still it can happen often that all 3 branches are possible.) This split is needed to make the summary correct and not dependent on if StreamChecker evaluates the function or not.

1650–1652

The current approach was that the NULL argument warning is generated by one checker only, StreamChecker or this checker. The NULL check is already built into these summaries so this is the better place for the warning. The NULL warning is removed from StreamChecker in D137790. Additionally the same problem can happen with BufferSizeConstraint at other function summaries, we can say generally (or more often) that if a buffer has size of 0 it is allowed to be NULL pointer.

Would be possible to test the errno specific changes as well?

I suppose the tests are done in the followup patch mostly?

clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
310

Very well, I'm sold.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
1650–1652

Aha, okay, so we should extend the existing infrastructure to express this as well. Please do that in another patch though! Thanks!

Szelethus added inline comments.Jan 3 2023, 7:30 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
799

I don't mean to make you test every single Case or ArgumentConstraint you added, but NotZeroConstraint is brand new, and is not tested anywhere.

1056–1064

Can you name an example for that? fgetpos?

balazske marked 5 inline comments as done.Jan 3 2023, 9:04 AM

I suppose the tests are done in the followup patch mostly?

Yes the tests in the next patch should cover the cases.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
799

This new class is probably not needed at all. It is always possible to use ReturnValueCondition(WithinRange, SingleValue(0)) or similar for nonzero cases.

1056–1064

fgetpos is a good example, the return value is already bound and a state split for success and failure was performed in the evalCall function (in StreamChecker) and the previous line Case.getErrnoConstraint().apply did not create a new state (this is true for fgetpos because it has a ErrnoUnchanged condition in the summary).

1793–1794

It could be an improvement to detect possible values for the argument 2 that are the SEEK_* values. Now the range [0,2] is used (the SEEK_* constants are usually 0,1,2).

balazske updated this revision to Diff 486252.Jan 4 2023, 6:01 AM
balazske marked an inline comment as done.

Removed class NotZeroConstraint.

LGTM, granted you add that test in the followup commit. If possible, I'd prefer to have features tested in the patch that added them (but this is fine for now).

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
799

Convenience is a good enough reason. Please test this before commiting.

1793–1794

Could you please more-or-less copy-paste this inline into the code?

Szelethus accepted this revision.Jan 4 2023, 6:36 AM
This revision is now accepted and ready to land.Jan 4 2023, 6:36 AM
balazske added inline comments.Jan 4 2023, 7:21 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
799

There was no exact test for the return value condition. A following test function can be added:

addToFunctionSummaryMap(
    "__test_return_note",
    Signature(ArgTypes{}, RetType{IntTy}),
    Summary(EvalCallAsPure)
        .Case({ReturnValueCondition(WithinRange, SingleValue(0))}, ErrnoIrrelevant, "Test return 0")
        .Case({ReturnValueCondition(WithinRange, SingleValue(1))}, ErrnoIrrelevant, "Test return 1")
        );

And add this to std-c-library-functions-arg-constraints-note-tags.cpp:

int __test_return_note();
int test_return_constraint_note(int y) {
  int x = __test_return_note(); // expected-note{{Test return 0}} \
                                // expected-note{{'x' initialized here}}
  return y / x;       // expected-warning{{Division by zero}} \
                      // expected-note{{Division by zero}}
}

LGTM! I think I prefer this solution anyways. Please commit (the entire patchstack).

balazske updated this revision to Diff 486558.Jan 5 2023, 7:03 AM

Adding test for summary case, change of comment.