[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp:114-117
+  const auto MacroIt = llvm::find_if(
+  PP.macros(), [&](const auto ) { return K.first->getName() == Macro; });
+  if (MacroIt == PP.macro_end())
+return llvm::None;

balazske wrote:
> martong wrote:
> > martong wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > martong wrote:
> > > > > > Szelethus wrote:
> > > > > > > This seems a bit clunky even for the `Preprocessor` -- how about
> > > > > > > 
> > > > > > > ```lang=c++
> > > > > > > const auto *MacroII = PP.getIdentifierInfo(Macro);
> > > > > > > if (!MacroII)
> > > > > > >   return;
> > > > > > > const MacroInfo *MI = PP.getMacroInfo(MacroII);
> > > > > > > assert(MI);
> > > > > > > ```
> > > > > > Ok, but we cannot assert on `MI`, because there may be cases when 
> > > > > > the macro is not defined in a TU. In that case we should just 
> > > > > > return with None.
> > > > > What exactly happens when the macro is `#undef`-ined and redefined? 
> > > > > We get the last redefinition that's valid at the end of the 
> > > > > translation unit, right? Can we check whether there are multiple 
> > > > > definitions and guard against that?
> > > > Ugh, now that you say it that is a valid concern. I had to deal with 
> > > > that back in the day: 
> > > > https://reviews.llvm.org/D52794?id=171962#inline-476352
> > > Solving this does not seem easy in my opinion. To handle `#undef`s we 
> > > should build an infrastructure where summaries can reference callable 
> > > objects. This is necessary, because in `evalCall` the value of `EOF` 
> > > would depend on the souce location of the call expression of the function 
> > > with the summary. Not impossible to solve, but certainly introduces 
> > > complexity. Do you think that the redefinition of EOF is so common? I 
> > > mean maybe it is too much hassle for a very rare edge case (?).
> > The standard library (libc or libc++) should define EOF consitently in 
> > stdio.h.
> > Now, if the application redefines the value of EOF then the code could be 
> > broken, or at least it would not be compatible with libc. Consider the 
> > following code that is perfectly legit if we don't redefine EOF, but if we 
> > do:
> > ```
> > #include 
> > #include 
> > #define EOF -2 // Here be dragons !!!
> > int main(void)
> > {
> > FILE* fp = fopen("test.txt", "r");
> > int c;
> > while ((c = fgetc(fp)) != EOF) { // BOOM: Infinite loop !!!
> >putchar(c);
> > }
> > fclose(fp);
> > }
> > ```
> > So, redefinition of EOF (or any standard macro) results in broken code.
> > And this is also a warning:
> > ```
> > ) clang eof-inf.c
> > eof-inf.c:3:9: warning: 'EOF' macro redefined [-Wmacro-redefined]
> > #define EOF -2 // Here be dragons !!!
> > ^
> > /usr/include/x86_64-linux-gnu/bits/libio.h:66:10: note: previous definition 
> > is here
> > # define EOF (-1)
> >  ^
> > 1 warning generated.
> > ```
> It should be possible to get the list of all macro definitions (of a macro). 
> We can select the first item, or one that is inside a system header (by the 
> SourceLocation).
Actually, redefinition of a macro in a standard library is undefined behavior:

C++11:
```
17.6.4.3.1
[macro.names]
1) A translation unit that includes a standard library header shall not #define 
or #undef names declared in any standard library header.
```
and
```
[reserved.names]
1) The C ++ standard library reserves the following kinds of names:
— macros
— global names
— names with external linkage
2) If a program declares or defines a name in a context where it is reserved, 
other than as explicitly allowed by this Clause, its behavior is undefined.
```

Also, this is stated in C99 (7.1).
Plus[[ https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html | 
 from libc manual ]]:
```
The names of all library types, macros, variables and functions that come from 
the ISO C standard are reserved unconditionally; your program may not redefine 
these names. All other library names are reserved if your program explicitly 
includes the header file that defines or declares them. 
```

Thus, I don't think we should come up with solutions to handle already broken 
code (i.e. programs with undefined behavior).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp:114-117
+  const auto MacroIt = llvm::find_if(
+  PP.macros(), [&](const auto ) { return K.first->getName() == Macro; });
+  if (MacroIt == PP.macro_end())
+return llvm::None;

martong wrote:
> martong wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > martong wrote:
> > > > > Szelethus wrote:
> > > > > > This seems a bit clunky even for the `Preprocessor` -- how about
> > > > > > 
> > > > > > ```lang=c++
> > > > > > const auto *MacroII = PP.getIdentifierInfo(Macro);
> > > > > > if (!MacroII)
> > > > > >   return;
> > > > > > const MacroInfo *MI = PP.getMacroInfo(MacroII);
> > > > > > assert(MI);
> > > > > > ```
> > > > > Ok, but we cannot assert on `MI`, because there may be cases when the 
> > > > > macro is not defined in a TU. In that case we should just return with 
> > > > > None.
> > > > What exactly happens when the macro is `#undef`-ined and redefined? We 
> > > > get the last redefinition that's valid at the end of the translation 
> > > > unit, right? Can we check whether there are multiple definitions and 
> > > > guard against that?
> > > Ugh, now that you say it that is a valid concern. I had to deal with that 
> > > back in the day: https://reviews.llvm.org/D52794?id=171962#inline-476352
> > Solving this does not seem easy in my opinion. To handle `#undef`s we 
> > should build an infrastructure where summaries can reference callable 
> > objects. This is necessary, because in `evalCall` the value of `EOF` would 
> > depend on the souce location of the call expression of the function with 
> > the summary. Not impossible to solve, but certainly introduces complexity. 
> > Do you think that the redefinition of EOF is so common? I mean maybe it is 
> > too much hassle for a very rare edge case (?).
> The standard library (libc or libc++) should define EOF consitently in 
> stdio.h.
> Now, if the application redefines the value of EOF then the code could be 
> broken, or at least it would not be compatible with libc. Consider the 
> following code that is perfectly legit if we don't redefine EOF, but if we do:
> ```
> #include 
> #include 
> #define EOF -2 // Here be dragons !!!
> int main(void)
> {
> FILE* fp = fopen("test.txt", "r");
> int c;
> while ((c = fgetc(fp)) != EOF) { // BOOM: Infinite loop !!!
>putchar(c);
> }
> fclose(fp);
> }
> ```
> So, redefinition of EOF (or any standard macro) results in broken code.
> And this is also a warning:
> ```
> ) clang eof-inf.c
> eof-inf.c:3:9: warning: 'EOF' macro redefined [-Wmacro-redefined]
> #define EOF -2 // Here be dragons !!!
> ^
> /usr/include/x86_64-linux-gnu/bits/libio.h:66:10: note: previous definition 
> is here
> # define EOF (-1)
>  ^
> 1 warning generated.
> ```
It should be possible to get the list of all macro definitions (of a macro). We 
can select the first item, or one that is inside a system header (by the 
SourceLocation).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp:114-117
+  const auto MacroIt = llvm::find_if(
+  PP.macros(), [&](const auto ) { return K.first->getName() == Macro; });
+  if (MacroIt == PP.macro_end())
+return llvm::None;

martong wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > martong wrote:
> > > > Szelethus wrote:
> > > > > This seems a bit clunky even for the `Preprocessor` -- how about
> > > > > 
> > > > > ```lang=c++
> > > > > const auto *MacroII = PP.getIdentifierInfo(Macro);
> > > > > if (!MacroII)
> > > > >   return;
> > > > > const MacroInfo *MI = PP.getMacroInfo(MacroII);
> > > > > assert(MI);
> > > > > ```
> > > > Ok, but we cannot assert on `MI`, because there may be cases when the 
> > > > macro is not defined in a TU. In that case we should just return with 
> > > > None.
> > > What exactly happens when the macro is `#undef`-ined and redefined? We 
> > > get the last redefinition that's valid at the end of the translation 
> > > unit, right? Can we check whether there are multiple definitions and 
> > > guard against that?
> > Ugh, now that you say it that is a valid concern. I had to deal with that 
> > back in the day: https://reviews.llvm.org/D52794?id=171962#inline-476352
> Solving this does not seem easy in my opinion. To handle `#undef`s we should 
> build an infrastructure where summaries can reference callable objects. This 
> is necessary, because in `evalCall` the value of `EOF` would depend on the 
> souce location of the call expression of the function with the summary. Not 
> impossible to solve, but certainly introduces complexity. Do you think that 
> the redefinition of EOF is so common? I mean maybe it is too much hassle for 
> a very rare edge case (?).
The standard library (libc or libc++) should define EOF consitently in stdio.h.
Now, if the application redefines the value of EOF then the code could be 
broken, or at least it would not be compatible with libc. Consider the 
following code that is perfectly legit if we don't redefine EOF, but if we do:
```
#include 
#include 
#define EOF -2 // Here be dragons !!!
int main(void)
{
FILE* fp = fopen("test.txt", "r");
int c;
while ((c = fgetc(fp)) != EOF) { // BOOM: Infinite loop !!!
   putchar(c);
}
fclose(fp);
}
```
So, redefinition of EOF (or any standard macro) results in broken code.
And this is also a warning:
```
) clang eof-inf.c
eof-inf.c:3:9: warning: 'EOF' macro redefined [-Wmacro-redefined]
#define EOF -2 // Here be dragons !!!
^
/usr/include/x86_64-linux-gnu/bits/libio.h:66:10: note: previous definition is 
here
# define EOF (-1)
 ^
1 warning generated.
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp:114-117
+  const auto MacroIt = llvm::find_if(
+  PP.macros(), [&](const auto ) { return K.first->getName() == Macro; });
+  if (MacroIt == PP.macro_end())
+return llvm::None;

Szelethus wrote:
> NoQ wrote:
> > martong wrote:
> > > Szelethus wrote:
> > > > This seems a bit clunky even for the `Preprocessor` -- how about
> > > > 
> > > > ```lang=c++
> > > > const auto *MacroII = PP.getIdentifierInfo(Macro);
> > > > if (!MacroII)
> > > >   return;
> > > > const MacroInfo *MI = PP.getMacroInfo(MacroII);
> > > > assert(MI);
> > > > ```
> > > Ok, but we cannot assert on `MI`, because there may be cases when the 
> > > macro is not defined in a TU. In that case we should just return with 
> > > None.
> > What exactly happens when the macro is `#undef`-ined and redefined? We get 
> > the last redefinition that's valid at the end of the translation unit, 
> > right? Can we check whether there are multiple definitions and guard 
> > against that?
> Ugh, now that you say it that is a valid concern. I had to deal with that 
> back in the day: https://reviews.llvm.org/D52794?id=171962#inline-476352
Solving this does not seem easy in my opinion. To handle `#undef`s we should 
build an infrastructure where summaries can reference callable objects. This is 
necessary, because in `evalCall` the value of `EOF` would depend on the souce 
location of the call expression of the function with the summary. Not 
impossible to solve, but certainly introduces complexity. Do you think that the 
redefinition of EOF is so common? I mean maybe it is too much hassle for a very 
rare edge case (?).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp:114-117
+  const auto MacroIt = llvm::find_if(
+  PP.macros(), [&](const auto ) { return K.first->getName() == Macro; });
+  if (MacroIt == PP.macro_end())
+return llvm::None;

NoQ wrote:
> martong wrote:
> > Szelethus wrote:
> > > This seems a bit clunky even for the `Preprocessor` -- how about
> > > 
> > > ```lang=c++
> > > const auto *MacroII = PP.getIdentifierInfo(Macro);
> > > if (!MacroII)
> > >   return;
> > > const MacroInfo *MI = PP.getMacroInfo(MacroII);
> > > assert(MI);
> > > ```
> > Ok, but we cannot assert on `MI`, because there may be cases when the macro 
> > is not defined in a TU. In that case we should just return with None.
> What exactly happens when the macro is `#undef`-ined and redefined? We get 
> the last redefinition that's valid at the end of the translation unit, right? 
> Can we check whether there are multiple definitions and guard against that?
Ugh, now that you say it that is a valid concern. I had to deal with that back 
in the day: https://reviews.llvm.org/D52794?id=171962#inline-476352


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp:114-117
+  const auto MacroIt = llvm::find_if(
+  PP.macros(), [&](const auto ) { return K.first->getName() == Macro; });
+  if (MacroIt == PP.macro_end())
+return llvm::None;

martong wrote:
> Szelethus wrote:
> > This seems a bit clunky even for the `Preprocessor` -- how about
> > 
> > ```lang=c++
> > const auto *MacroII = PP.getIdentifierInfo(Macro);
> > if (!MacroII)
> >   return;
> > const MacroInfo *MI = PP.getMacroInfo(MacroII);
> > assert(MI);
> > ```
> Ok, but we cannot assert on `MI`, because there may be cases when the macro 
> is not defined in a TU. In that case we should just return with None.
What exactly happens when the macro is `#undef`-ined and redefined? We get the 
last redefinition that's valid at the end of the translation unit, right? Can 
we check whether there are multiple definitions and guard against that?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG536456a7e93d: [analyzer] StdLibraryFunctionsChecker: Use 
platform dependent EOF and UCharMax (authored by martong).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
  clang/test/Analysis/std-c-library-functions-eof.c

Index: clang/test/Analysis/std-c-library-functions-eof.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-eof.c
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=core,apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=core,apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=core,apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=core,apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+
+void clang_analyzer_eval(int);
+
+typedef struct FILE FILE;
+// Unorthodox EOF value.
+#define EOF (-2)
+
+int getc(FILE *);
+void test_getc(FILE *fp) {
+
+  int x;
+  while ((x = getc(fp)) != EOF) {
+clang_analyzer_eval(x > 255); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= 0); // expected-warning{{TRUE}}
+  }
+
+  int y = getc(fp);
+  if (y < 0) {
+clang_analyzer_eval(y == -2); // expected-warning{{TRUE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -13,6 +13,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
+#include "clang/Lex/Preprocessor.h"
 
 namespace clang {
 
@@ -109,6 +110,45 @@
   return Nullability::Unspecified;
 }
 
+llvm::Optional tryExpandAsInteger(StringRef Macro,
+   const Preprocessor ) {
+  const auto *MacroII = PP.getIdentifierInfo(Macro);
+  if (!MacroII)
+return llvm::None;
+  const MacroInfo *MI = PP.getMacroInfo(MacroII);
+  if (!MI)
+return llvm::None;
+
+  // Filter out parens.
+  std::vector FilteredTokens;
+  FilteredTokens.reserve(MI->tokens().size());
+  for (auto  : MI->tokens())
+if (!T.isOneOf(tok::l_paren, tok::r_paren))
+  FilteredTokens.push_back(T);
+
+  if (FilteredTokens.size() > 2)
+return llvm::None;
+
+  // Parse an integer at the end of the macro definition.
+  const Token  = FilteredTokens.back();
+  if (!T.isLiteral())
+return llvm::None;
+  StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength());
+  llvm::APInt IntValue;
+  constexpr unsigned AutoSenseRadix = 0;
+  if (ValueStr.getAsInteger(AutoSenseRadix, IntValue))
+return llvm::None;
+
+  // Parse an optional minus sign.
+  if (FilteredTokens.size() == 2) {
+if (FilteredTokens.front().is(tok::minus))
+  IntValue = -IntValue;
+else
+  return llvm::None;
+  }
+
+  return IntValue.getSExtValue();
+}
 
-} // end namespace ento
-} // end namespace clang
+} // namespace ento
+} // namespace clang
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -55,6 +55,7 @@
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 using namespace clang;
 using namespace clang::ento;
@@ -241,7 +242,7 @@
 const CallExpr *CE,
 CheckerContext ) const;
 
-  void initFunctionSummaries(BasicValueFactory ) const;
+  void initFunctionSummaries(CheckerContext ) const;
 };
 } // end of anonymous namespace
 
@@ -286,6 +287,12 @@
   // "WithinRange R" is treated as "outside [T_MIN, T_MAX] \ R".
   // We cut off [T_MIN, min(R) - 1] and [max(R) + 1, T_MAX] if necessary,
   // and then cut away all 

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:313-321
 for (size_t I = 1; I != E; ++I) {
   const llvm::APSInt  = BVF.getValue(R[I - 1].second + 1ULL, T);
   const llvm::APSInt  = BVF.getValue(R[I].first - 1ULL, T);
-  assert(Min <= Max);
-  State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
-  if (!State)
-return nullptr;
+  if (Min <= Max) {
+State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
+if (!State)
+  return nullptr;

Szelethus wrote:
> I appreciated you sitting down with me with a piece of paper and a pen to 
> explain what happens here, but for the less fortunate this code snippet 
> should be decorated with some ASCII art.
> 
> I don't insist on you doing that within the scope of this patch, but if you 
> did anyways, that would be great :)
Ok, I have added that below the existing comment of the enclosing `if` block.



Comment at: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp:138
+  llvm::APInt IntValue;
+  constexpr unsigned AutoSenseRadix = 0;
+  if (ValueStr.getAsInteger(AutoSenseRadix, IntValue))

Szelethus wrote:
> TIL Despite the analyzer having a half-a-decade old checker option that was 
> defaulted to a hexadecimal value, nobody wrote tests for it, and it took 4-5 
> years to unearth that due to the incorrect parsing of integers (it was set to 
> decimal instead of auto detect) it never ever worked.
Well, I hope AutoSenseRadix will just detect the correct base.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 244376.
martong marked 6 inline comments as done.
martong added a comment.

- Enable core checkers explicitly in test
- Add ASCII art to depict a range list example


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
  clang/test/Analysis/std-c-library-functions-eof.c

Index: clang/test/Analysis/std-c-library-functions-eof.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-eof.c
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=core,apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=core,apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=core,apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=core,apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+
+void clang_analyzer_eval(int);
+
+typedef struct FILE FILE;
+// Unorthodox EOF value.
+#define EOF (-2)
+
+int getc(FILE *);
+void test_getc(FILE *fp) {
+
+  int x;
+  while ((x = getc(fp)) != EOF) {
+clang_analyzer_eval(x > 255); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= 0); // expected-warning{{TRUE}}
+  }
+
+  int y = getc(fp);
+  if (y < 0) {
+clang_analyzer_eval(y == -2); // expected-warning{{TRUE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -13,6 +13,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
+#include "clang/Lex/Preprocessor.h"
 
 namespace clang {
 
@@ -109,6 +110,45 @@
   return Nullability::Unspecified;
 }
 
+llvm::Optional tryExpandAsInteger(StringRef Macro,
+   const Preprocessor ) {
+  const auto *MacroII = PP.getIdentifierInfo(Macro);
+  if (!MacroII)
+return llvm::None;
+  const MacroInfo *MI = PP.getMacroInfo(MacroII);
+  if (!MI)
+return llvm::None;
+
+  // Filter out parens.
+  std::vector FilteredTokens;
+  FilteredTokens.reserve(MI->tokens().size());
+  for (auto  : MI->tokens())
+if (!T.isOneOf(tok::l_paren, tok::r_paren))
+  FilteredTokens.push_back(T);
+
+  if (FilteredTokens.size() > 2)
+return llvm::None;
+
+  // Parse an integer at the end of the macro definition.
+  const Token  = FilteredTokens.back();
+  if (!T.isLiteral())
+return llvm::None;
+  StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength());
+  llvm::APInt IntValue;
+  constexpr unsigned AutoSenseRadix = 0;
+  if (ValueStr.getAsInteger(AutoSenseRadix, IntValue))
+return llvm::None;
+
+  // Parse an optional minus sign.
+  if (FilteredTokens.size() == 2) {
+if (FilteredTokens.front().is(tok::minus))
+  IntValue = -IntValue;
+else
+  return llvm::None;
+  }
+
+  return IntValue.getSExtValue();
+}
 
-} // end namespace ento
-} // end namespace clang
+} // namespace ento
+} // namespace clang
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -55,6 +55,7 @@
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 using namespace clang;
 using namespace clang::ento;
@@ -241,7 +242,7 @@
 const CallExpr *CE,
 CheckerContext ) const;
 
-  void initFunctionSummaries(BasicValueFactory ) const;
+  void initFunctionSummaries(CheckerContext ) const;
 };
 } // end of anonymous namespace
 
@@ -286,6 +287,12 @@
   // "WithinRange R" is treated as "outside [T_MIN, T_MAX] \ R".
   // We cut off [T_MIN, min(R) - 1] and [max(R) + 1, T_MAX] if necessary,
   // and then cut away all holes 

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/test/Analysis/std-c-library-functions-eof.c:11
+// Unorthodox EOF value.
+#define EOF (-2)
+

Maybe you could throw in a test where the definition isn't surrounded by 
parentheses? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

LGTM! Very nice! I think you can commit as you please, granted you add core 
checkers to the test `RUN:` lines.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:313-321
 for (size_t I = 1; I != E; ++I) {
   const llvm::APSInt  = BVF.getValue(R[I - 1].second + 1ULL, T);
   const llvm::APSInt  = BVF.getValue(R[I].first - 1ULL, T);
-  assert(Min <= Max);
-  State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
-  if (!State)
-return nullptr;
+  if (Min <= Max) {
+State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
+if (!State)
+  return nullptr;

I appreciated you sitting down with me with a piece of paper and a pen to 
explain what happens here, but for the less fortunate this code snippet should 
be decorated with some ASCII art.

I don't insist on you doing that within the scope of this patch, but if you did 
anyways, that would be great :)



Comment at: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp:138
+  llvm::APInt IntValue;
+  constexpr unsigned AutoSenseRadix = 0;
+  if (ValueStr.getAsInteger(AutoSenseRadix, IntValue))

TIL Despite the analyzer having a half-a-decade old checker option that was 
defaulted to a hexadecimal value, nobody wrote tests for it, and it took 4-5 
years to unearth that due to the incorrect parsing of integers (it was set to 
decimal instead of auto detect) it never ever worked.



Comment at: clang/test/Analysis/std-c-library-functions-eof.c:1-5
+// RUN: %clang_analyze_cc1 
-analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify 
-analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux 
-analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify 
-analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux 
-analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify 
-analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux 
-analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify 
-analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux 
-analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify 
-analyzer-config eagerly-assume=false %s

Always enable `core` checkers for pathsensitive analyses.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 244173.
martong marked 6 inline comments as done.
martong added a comment.

- Move include of Preprocessor.h to CheckerHelpers.cpp
- Try -> try
- Use PP.getIdentifierInfo
- Handle parens in tryExpandAsInteger


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
  clang/test/Analysis/std-c-library-functions-eof.c

Index: clang/test/Analysis/std-c-library-functions-eof.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-eof.c
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+
+void clang_analyzer_eval(int);
+
+typedef struct FILE FILE;
+// Unorthodox EOF value.
+#define EOF (-2)
+
+int getc(FILE *);
+void test_getc(FILE *fp) {
+
+  int x;
+  while ((x = getc(fp)) != EOF) {
+clang_analyzer_eval(x > 255); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= 0); // expected-warning{{TRUE}}
+  }
+
+  int y = getc(fp);
+  if (y < 0) {
+clang_analyzer_eval(y == -2); // expected-warning{{TRUE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -13,6 +13,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
+#include "clang/Lex/Preprocessor.h"
 
 namespace clang {
 
@@ -109,6 +110,45 @@
   return Nullability::Unspecified;
 }
 
+llvm::Optional tryExpandAsInteger(StringRef Macro,
+   const Preprocessor ) {
+  const auto *MacroII = PP.getIdentifierInfo(Macro);
+  if (!MacroII)
+return llvm::None;
+  const MacroInfo *MI = PP.getMacroInfo(MacroII);
+  if (!MI)
+return llvm::None;
+
+  // Filter out parens.
+  std::vector FilteredTokens;
+  FilteredTokens.reserve(MI->tokens().size());
+  for (auto  : MI->tokens())
+if (!T.isOneOf(tok::l_paren, tok::r_paren))
+  FilteredTokens.push_back(T);
+
+  if (FilteredTokens.size() > 2)
+return llvm::None;
+
+  // Parse an integer at the end of the macro definition.
+  const Token  = FilteredTokens.back();
+  if (!T.isLiteral())
+return llvm::None;
+  StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength());
+  llvm::APInt IntValue;
+  constexpr unsigned AutoSenseRadix = 0;
+  if (ValueStr.getAsInteger(AutoSenseRadix, IntValue))
+return llvm::None;
+
+  // Parse an optional minus sign.
+  if (FilteredTokens.size() == 2) {
+if (FilteredTokens.front().is(tok::minus))
+  IntValue = -IntValue;
+else
+  return llvm::None;
+  }
+
+  return IntValue.getSExtValue();
+}
 
-} // end namespace ento
-} // end namespace clang
+} // namespace ento
+} // namespace clang
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -55,6 +55,7 @@
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 using namespace clang;
 using namespace clang::ento;
@@ -241,7 +242,7 @@
 const CallExpr *CE,
 CheckerContext ) const;
 
-  void initFunctionSummaries(BasicValueFactory ) const;
+  void initFunctionSummaries(CheckerContext ) const;
 };
 } // end of anonymous namespace
 
@@ -312,10 +313,11 @@
 for (size_t I = 1; I != E; ++I) {
   const llvm::APSInt  = BVF.getValue(R[I - 1].second + 1ULL, T);
   const llvm::APSInt  = 

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong marked 2 inline comments as done.
martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:571
+.Case(
+{ReturnValueCondition(WithinRange, {{EOFv, EOFv}, {0, 
UCharMax}})});
   };

Szelethus wrote:
> Huh, what's happening here exactly? Doesn't `Range` take 2 `RangeInt`s as 
> ctor arguments? What does `{EOFv, EOFv}, {0, UCharMax}` mean in this context?
`Range` is a convenience function that creates a `IntRangeVector` from the two 
params. This way we can easily create a list of ranges from two ints. Range(-1, 
255) results a list of ranges with one list: {{-1, 255}}.
Now, because EOF's value can be less than the minimum value of the next range 
we have to explicitly enlist the ranges.
E.g. if EOF is -2 then we will have a list of {-2,-2} and {0, 255}.



Comment at: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp:114-117
+  const auto MacroIt = llvm::find_if(
+  PP.macros(), [&](const auto ) { return K.first->getName() == Macro; });
+  if (MacroIt == PP.macro_end())
+return llvm::None;

Szelethus wrote:
> This seems a bit clunky even for the `Preprocessor` -- how about
> 
> ```lang=c++
> const auto *MacroII = PP.getIdentifierInfo(Macro);
> if (!MacroII)
>   return;
> const MacroInfo *MI = PP.getMacroInfo(MacroII);
> assert(MI);
> ```
Ok, but we cannot assert on `MI`, because there may be cases when the macro is 
not defined in a TU. In that case we should just return with None.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added inline comments.
This revision now requires changes to proceed.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:67-69
+/// Try to parse the value of a defined preprocessor macro. We can only parse
+/// simple expressions that consist of an optional minus sign token and then a
+/// token for an integer. If we cannot parse the value then None is returned.

While I agree that we shouldn't have to reinvent the `Preprocessor` every time 
we need something macro related, I doubt that this will catch anything. I 
checker my system's standard library, and this is what I found:

```lang=c
#ifndef EOF
# define EOF (-1)
#endif
```
Lets go just one step further and cover the probably majority of the cases 
where the definition is in parentheses.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:512-517
+  const auto EOFv = []() -> RangeInt {
+if (const llvm::Optional OptInt =
+TryExpandAsInteger("EOF", C.getPreprocessor()))
+  return *OptInt;
+return -1;
+  }();

Hah, would've never thought of doing this with a lambda. Me likey.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:571
+.Case(
+{ReturnValueCondition(WithinRange, {{EOFv, EOFv}, {0, 
UCharMax}})});
   };

Huh, what's happening here exactly? Doesn't `Range` take 2 `RangeInt`s as ctor 
arguments? What does `{EOFv, EOFv}, {0, UCharMax}` mean in this context?



Comment at: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp:114-117
+  const auto MacroIt = llvm::find_if(
+  PP.macros(), [&](const auto ) { return K.first->getName() == Macro; });
+  if (MacroIt == PP.macro_end())
+return llvm::None;

This seems a bit clunky even for the `Preprocessor` -- how about

```lang=c++
const auto *MacroII = PP.getIdentifierInfo(Macro);
if (!MacroII)
  return;
const MacroInfo *MI = PP.getMacroInfo(MacroII);
assert(MI);
```



Comment at: clang/test/Analysis/std-c-library-functions-eof.c:24
+  if (y < 0) {
+clang_analyzer_eval(y == EOF); // expected-warning{{TRUE}}
+  }

So the test is about checking whether the analyzer correctly assigned `-2` to 
`y`, right? Then let's check that too.
```lang=c++
clang_analyzer_eval(y == 2);
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:17
 #include "clang/AST/Stmt.h"
+#include "clang/Lex/Preprocessor.h"
+#include "llvm/ADT/Optional.h"

We do not need to include Preprocessor.h here (fwd declaration is enough).



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:70
+/// token for an integer. If we cannot parse the value then None is returned.
+llvm::Optional TryExpandAsInteger(StringRef Macro, const Preprocessor 
);
 

Name should begin with lowercase letter?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 244149.
martong added a comment.

- Fix crash in TryExpandAsInteger


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
  clang/test/Analysis/std-c-library-functions-eof.c

Index: clang/test/Analysis/std-c-library-functions-eof.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-eof.c
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+
+void clang_analyzer_eval(int);
+
+typedef struct FILE FILE;
+// Unorthodox EOF value.
+#define EOF -2
+
+int getc(FILE *);
+void test_getc(FILE *fp) {
+
+  int x;
+  while ((x = getc(fp)) != EOF) {
+clang_analyzer_eval(x > 255); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= 0); // expected-warning{{TRUE}}
+  }
+
+  int y = getc(fp);
+  if (y < 0) {
+clang_analyzer_eval(y == EOF); // expected-warning{{TRUE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -109,6 +109,31 @@
   return Nullability::Unspecified;
 }
 
+llvm::Optional TryExpandAsInteger(StringRef Macro,
+   const Preprocessor ) {
+  const auto MacroIt = llvm::find_if(
+  PP.macros(), [&](const auto ) { return K.first->getName() == Macro; });
+  if (MacroIt == PP.macro_end())
+return llvm::None;
+  const MacroInfo *MI = PP.getMacroInfo(MacroIt->first);
+
+  // Parse an integer at the end of the macro definition.
+  const Token  = MI->tokens().back();
+  if (!T.isLiteral())
+return llvm::None;
+  StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength());
+  llvm::APInt IntValue;
+  constexpr unsigned AutoSenseRadix = 0;
+  if (ValueStr.getAsInteger(AutoSenseRadix, IntValue))
+return llvm::None;
+
+  // Parse an optional minus sign.
+  if (MI->tokens().size() == 2)
+if (MI->tokens().front().is(tok::minus))
+  IntValue = -IntValue;
+
+  return IntValue.getSExtValue();
+}
 
-} // end namespace ento
-} // end namespace clang
+} // namespace ento
+} // namespace clang
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -55,6 +55,7 @@
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 using namespace clang;
 using namespace clang::ento;
@@ -241,7 +242,7 @@
 const CallExpr *CE,
 CheckerContext ) const;
 
-  void initFunctionSummaries(BasicValueFactory ) const;
+  void initFunctionSummaries(CheckerContext ) const;
 };
 } // end of anonymous namespace
 
@@ -312,10 +313,11 @@
 for (size_t I = 1; I != E; ++I) {
   const llvm::APSInt  = BVF.getValue(R[I - 1].second + 1ULL, T);
   const llvm::APSInt  = BVF.getValue(R[I].first - 1ULL, T);
-  assert(Min <= Max);
-  State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
-  if (!State)
-return nullptr;
+  if (Min <= Max) {
+State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
+if (!State)
+  return nullptr;
+  }
 }
   }
 
@@ -449,9 +451,7 @@
   if (!FD)
 return None;
 
-  SValBuilder  = C.getSValBuilder();
-  BasicValueFactory  = SVB.getBasicValueFactory();
-  initFunctionSummaries(BVF);
+  initFunctionSummaries(C);
 
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)
@@ -478,11 +478,13 @@
 }
 
 void 

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> Harbormaster failed remote builds in B46321 
> : Diff 244141!

This is actually true, I have a test that crashes! Finally, a case where remote 
builds are proved to be useful!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 244143.
martong added a comment.

- EOFMacroIt -> MacroIt


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
  clang/test/Analysis/std-c-library-functions-eof.c

Index: clang/test/Analysis/std-c-library-functions-eof.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-eof.c
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+
+void clang_analyzer_eval(int);
+
+typedef struct FILE FILE;
+// Unorthodox EOF value.
+#define EOF -2
+
+int getc(FILE *);
+void test_getc(FILE *fp) {
+
+  int x;
+  while ((x = getc(fp)) != EOF) {
+clang_analyzer_eval(x > 255); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= 0); // expected-warning{{TRUE}}
+  }
+
+  int y = getc(fp);
+  if (y < 0) {
+clang_analyzer_eval(y == EOF); // expected-warning{{TRUE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -109,6 +109,29 @@
   return Nullability::Unspecified;
 }
 
+llvm::Optional TryExpandAsInteger(StringRef Macro,
+   const Preprocessor ) {
+  const auto MacroIt = llvm::find_if(
+  PP.macros(), [&](const auto ) { return K.first->getName() == Macro; });
+  if (MacroIt == PP.macro_end())
+return llvm::None;
+  const MacroInfo *MI = PP.getMacroInfo(MacroIt->first);
+
+  // Parse an integer at the end of the macro definition.
+  const Token  = MI->tokens().back();
+  StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength());
+  llvm::APInt IntValue;
+  constexpr unsigned AutoSenseRadix = 0;
+  if (ValueStr.getAsInteger(AutoSenseRadix, IntValue))
+return llvm::None;
+
+  // Parse an optional minus sign.
+  if (MI->tokens().size() == 2)
+if (MI->tokens().front().is(tok::minus))
+  IntValue = -IntValue;
+
+  return IntValue.getSExtValue();
+}
 
-} // end namespace ento
-} // end namespace clang
+} // namespace ento
+} // namespace clang
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -55,6 +55,7 @@
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 using namespace clang;
 using namespace clang::ento;
@@ -241,7 +242,7 @@
 const CallExpr *CE,
 CheckerContext ) const;
 
-  void initFunctionSummaries(BasicValueFactory ) const;
+  void initFunctionSummaries(CheckerContext ) const;
 };
 } // end of anonymous namespace
 
@@ -312,10 +313,11 @@
 for (size_t I = 1; I != E; ++I) {
   const llvm::APSInt  = BVF.getValue(R[I - 1].second + 1ULL, T);
   const llvm::APSInt  = BVF.getValue(R[I].first - 1ULL, T);
-  assert(Min <= Max);
-  State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
-  if (!State)
-return nullptr;
+  if (Min <= Max) {
+State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
+if (!State)
+  return nullptr;
+  }
 }
   }
 
@@ -449,9 +451,7 @@
   if (!FD)
 return None;
 
-  SValBuilder  = C.getSValBuilder();
-  BasicValueFactory  = SVB.getBasicValueFactory();
-  initFunctionSummaries(BVF);
+  initFunctionSummaries(C);
 
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)
@@ -478,11 +478,13 @@
 }
 
 void StdLibraryFunctionsChecker::initFunctionSummaries(
-

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 244141.
martong added a comment.

- Remove PP declaration


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
  clang/test/Analysis/std-c-library-functions-eof.c

Index: clang/test/Analysis/std-c-library-functions-eof.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-eof.c
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+
+void clang_analyzer_eval(int);
+
+typedef struct FILE FILE;
+// Unorthodox EOF value.
+#define EOF -2
+
+int getc(FILE *);
+void test_getc(FILE *fp) {
+
+  int x;
+  while ((x = getc(fp)) != EOF) {
+clang_analyzer_eval(x > 255); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= 0); // expected-warning{{TRUE}}
+  }
+
+  int y = getc(fp);
+  if (y < 0) {
+clang_analyzer_eval(y == EOF); // expected-warning{{TRUE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -109,6 +109,29 @@
   return Nullability::Unspecified;
 }
 
+llvm::Optional TryExpandAsInteger(StringRef Macro,
+   const Preprocessor ) {
+  const auto EOFMacroIt = llvm::find_if(
+  PP.macros(), [&](const auto ) { return K.first->getName() == Macro; });
+  if (EOFMacroIt == PP.macro_end())
+return llvm::None;
+  const MacroInfo *MI = PP.getMacroInfo(EOFMacroIt->first);
+
+  // Parse an integer at the end of the macro definition.
+  const Token  = MI->tokens().back();
+  StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength());
+  llvm::APInt IntValue;
+  constexpr unsigned AutoSenseRadix = 0;
+  if (ValueStr.getAsInteger(AutoSenseRadix, IntValue))
+return llvm::None;
+
+  // Parse an optional minus sign.
+  if (MI->tokens().size() == 2)
+if (MI->tokens().front().is(tok::minus))
+  IntValue = -IntValue;
+
+  return IntValue.getSExtValue();
+}
 
-} // end namespace ento
-} // end namespace clang
+} // namespace ento
+} // namespace clang
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -55,6 +55,7 @@
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 using namespace clang;
 using namespace clang::ento;
@@ -241,7 +242,7 @@
 const CallExpr *CE,
 CheckerContext ) const;
 
-  void initFunctionSummaries(BasicValueFactory ) const;
+  void initFunctionSummaries(CheckerContext ) const;
 };
 } // end of anonymous namespace
 
@@ -312,10 +313,11 @@
 for (size_t I = 1; I != E; ++I) {
   const llvm::APSInt  = BVF.getValue(R[I - 1].second + 1ULL, T);
   const llvm::APSInt  = BVF.getValue(R[I].first - 1ULL, T);
-  assert(Min <= Max);
-  State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
-  if (!State)
-return nullptr;
+  if (Min <= Max) {
+State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
+if (!State)
+  return nullptr;
+  }
 }
   }
 
@@ -449,9 +451,7 @@
   if (!FD)
 return None;
 
-  SValBuilder  = C.getSValBuilder();
-  BasicValueFactory  = SVB.getBasicValueFactory();
-  initFunctionSummaries(BVF);
+  initFunctionSummaries(C);
 
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)
@@ -478,11 +478,13 @@
 }
 
 void StdLibraryFunctionsChecker::initFunctionSummaries(
-

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 244140.
martong added a comment.

- Remove debug dump
- Add TryExpandAsInteger to CheckerHelpers.h


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
  clang/test/Analysis/std-c-library-functions-eof.c

Index: clang/test/Analysis/std-c-library-functions-eof.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-eof.c
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+
+void clang_analyzer_eval(int);
+
+typedef struct FILE FILE;
+// Unorthodox EOF value.
+#define EOF -2
+
+int getc(FILE *);
+void test_getc(FILE *fp) {
+
+  int x;
+  while ((x = getc(fp)) != EOF) {
+clang_analyzer_eval(x > 255); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= 0); // expected-warning{{TRUE}}
+  }
+
+  int y = getc(fp);
+  if (y < 0) {
+clang_analyzer_eval(y == EOF); // expected-warning{{TRUE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -109,6 +109,29 @@
   return Nullability::Unspecified;
 }
 
+llvm::Optional TryExpandAsInteger(StringRef Macro,
+   const Preprocessor ) {
+  const auto EOFMacroIt = llvm::find_if(
+  PP.macros(), [&](const auto ) { return K.first->getName() == Macro; });
+  if (EOFMacroIt == PP.macro_end())
+return llvm::None;
+  const MacroInfo *MI = PP.getMacroInfo(EOFMacroIt->first);
+
+  // Parse an integer at the end of the macro definition.
+  const Token  = MI->tokens().back();
+  StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength());
+  llvm::APInt IntValue;
+  constexpr unsigned AutoSenseRadix = 0;
+  if (ValueStr.getAsInteger(AutoSenseRadix, IntValue))
+return llvm::None;
+
+  // Parse an optional minus sign.
+  if (MI->tokens().size() == 2)
+if (MI->tokens().front().is(tok::minus))
+  IntValue = -IntValue;
+
+  return IntValue.getSExtValue();
+}
 
-} // end namespace ento
-} // end namespace clang
+} // namespace ento
+} // namespace clang
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -55,6 +55,7 @@
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 using namespace clang;
 using namespace clang::ento;
@@ -241,7 +242,7 @@
 const CallExpr *CE,
 CheckerContext ) const;
 
-  void initFunctionSummaries(BasicValueFactory ) const;
+  void initFunctionSummaries(CheckerContext ) const;
 };
 } // end of anonymous namespace
 
@@ -312,10 +313,11 @@
 for (size_t I = 1; I != E; ++I) {
   const llvm::APSInt  = BVF.getValue(R[I - 1].second + 1ULL, T);
   const llvm::APSInt  = BVF.getValue(R[I].first - 1ULL, T);
-  assert(Min <= Max);
-  State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
-  if (!State)
-return nullptr;
+  if (Min <= Max) {
+State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
+if (!State)
+  return nullptr;
+  }
 }
   }
 
@@ -449,9 +451,7 @@
   if (!FD)
 return None;
 
-  SValBuilder  = C.getSValBuilder();
-  BasicValueFactory  = SVB.getBasicValueFactory();
-  initFunctionSummaries(BVF);
+  initFunctionSummaries(C);
 
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)
@@ -478,11 +478,13 @@
 }
 
 void 

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong planned changes to this revision.
martong marked 3 inline comments as done.
martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:533
+
+IntValue.dump();
+return IntValue.getSExtValue();

balazske wrote:
> Debug message (to be removed)?
Thanks, good catch!



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:535
+return IntValue.getSExtValue();
+  }();
 

balazske wrote:
> It would be good to have this function available generally to other checkers, 
> the same functionality is needed in https://reviews.llvm.org/D72705 too.
> It could work with any (specified) macro name, there are other special values 
> in API calls. But there can be more difficult cases if the EOF (or other) is 
> not a simple value but another macro or constructed from values some way. 
> (The `ULONG_MAX` and similar can be get in the same way.)
Ok, I am gonna put a generic version of this under `CheckerHelpers.h`, so all 
checkers can use it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:533
+
+IntValue.dump();
+return IntValue.getSExtValue();

Debug message (to be removed)?



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:535
+return IntValue.getSExtValue();
+  }();
 

It would be good to have this function available generally to other checkers, 
the same functionality is needed in https://reviews.llvm.org/D72705 too.
It could work with any (specified) macro name, there are other special values 
in API calls. But there can be more difficult cases if the EOF (or other) is 
not a simple value but another macro or constructed from values some way. (The 
`ULONG_MAX` and similar can be get in the same way.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74473/new/

https://reviews.llvm.org/D74473



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: Szelethus, NoQ.
Herald added subscribers: cfe-commits, steakhal, Charusso, gamesh411, dkrupp, 
donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, 
kristof.beyls, xazax.hun, whisperity.
Herald added a project: clang.
martong added a child revision: D73898: [analyzer] StdLibraryFunctionsChecker: 
Add argument constraints.

Both EOF and the max value of unsigned char is platform dependent. In this
patch we try our best to deduce the value of EOF from the Preprocessor,
if we can't we fall back to -1.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74473

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/std-c-library-functions-eof.c

Index: clang/test/Analysis/std-c-library-functions-eof.c
===
--- /dev/null
+++ clang/test/Analysis/std-c-library-functions-eof.c
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+
+void clang_analyzer_eval(int);
+
+typedef struct FILE FILE;
+// Unorthodox EOF value.
+#define EOF -2
+
+int getc(FILE *);
+void test_getc(FILE *fp) {
+
+  int x;
+  while ((x = getc(fp)) != EOF) {
+clang_analyzer_eval(x > 255); // expected-warning{{FALSE}}
+clang_analyzer_eval(x >= 0); // expected-warning{{TRUE}}
+  }
+
+  int y = getc(fp);
+  if (y < 0) {
+clang_analyzer_eval(y == EOF); // expected-warning{{TRUE}}
+  }
+}
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -241,7 +241,7 @@
 const CallExpr *CE,
 CheckerContext ) const;
 
-  void initFunctionSummaries(BasicValueFactory ) const;
+  void initFunctionSummaries(CheckerContext ) const;
 };
 } // end of anonymous namespace
 
@@ -312,10 +312,11 @@
 for (size_t I = 1; I != E; ++I) {
   const llvm::APSInt  = BVF.getValue(R[I - 1].second + 1ULL, T);
   const llvm::APSInt  = BVF.getValue(R[I].first - 1ULL, T);
-  assert(Min <= Max);
-  State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
-  if (!State)
-return nullptr;
+  if (Min <= Max) {
+State = CM.assumeInclusiveRange(State, *N, Min, Max, false);
+if (!State)
+  return nullptr;
+  }
 }
   }
 
@@ -449,9 +450,7 @@
   if (!FD)
 return None;
 
-  SValBuilder  = C.getSValBuilder();
-  BasicValueFactory  = SVB.getBasicValueFactory();
-  initFunctionSummaries(BVF);
+  initFunctionSummaries(C);
 
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)
@@ -478,11 +477,13 @@
 }
 
 void StdLibraryFunctionsChecker::initFunctionSummaries(
-BasicValueFactory ) const {
+CheckerContext ) const {
   if (!FunctionSummaryMap.empty())
 return;
 
-  ASTContext  = BVF.getContext();
+  SValBuilder  = C.getSValBuilder();
+  BasicValueFactory  = SVB.getBasicValueFactory();
+  const ASTContext  = BVF.getContext();
 
   // These types are useful for writing specifications quickly,
   // New specifications should probably introduce more types.
@@ -491,15 +492,47 @@
   // of function summary for common cases (eg. ssize_t could be int or long
   // or long long, so three summary variants would be enough).
   // Of course, function variants are also useful for C++ overloads.
-  QualType Irrelevant; // A placeholder, whenever we do not care about the type.
-  QualType IntTy = ACtx.IntTy;
-  QualType LongTy = ACtx.LongTy;
-  QualType LongLongTy = ACtx.LongLongTy;
-  QualType SizeTy = ACtx.getSizeType();
-
-  RangeInt IntMax = BVF.getMaxValue(IntTy).getLimitedValue();
-  RangeInt LongMax = BVF.getMaxValue(LongTy).getLimitedValue();
-  RangeInt LongLongMax = BVF.getMaxValue(LongLongTy).getLimitedValue();
+  const QualType
+  Irrelevant; // A placeholder, whenever we do not care about the type.
+  const QualType IntTy = ACtx.IntTy;
+  const QualType LongTy = ACtx.LongTy;
+  const QualType LongLongTy =