Enable "Unix" checks for Windows

[ariccio]

Summary

As discussed on the mailing list in "Clang on Windows fails to detect trivial double free in static analysis", the "unix" checkers are also applicable on Windows, and enables Malloc/MallocSizeof/cstring.BadSizeArg/cstring.NullArg/API checking. This patch enables the unix checkers by removing the "IsWindowsMSVC" check.

(I haven't actually compiled this yet, but will do so immediately)

[ariccio]

(build seems fine)

Do I need to run the tests, since this is the driver, and it works on unix as is?

[dcoughlin]

Thanks for doing this!

Do I need to run the tests, since this is the driver, and it works on unix as is?

It's probably a good idea to run all the clang tests just to make sure the driver still works on Windows after your change.

C:/LLVM/llvm/tools/clang/lib/Driver/Tools.cpp

3592

The LLVM coding standards call for prose and punctuation here. (See http://llvm.org/docs/CodingStandards.html#commenting). The convention is also for a space after '//'.

3593

I think it might be better to explicitly enable the checkers we know make sense on Windows (e.g., unix.Malloc) rather than all checkers in the unix package. For example, it seems to me that the Vfork checker might produce confusing false positives on Windows codebases. This would also prevent future Unix-only checkers from being accidentally enabled on Windows.

What do you think?

[ariccio]

In D16245#328347, @dcoughlin wrote:

It's probably a good idea to run all the clang tests just to make sure the driver still works on Windows after your change.

Everything seems fine - there are two tests that are failing for unrelated reasons (find.exe on windows is not compatible), and in those two tests, I've manually verified that the tests are working - so that's all good.

C:/LLVM/llvm/tools/clang/lib/Driver/Tools.cpp

3592

Whoops! I'm still an LLVM newbie. Will fix in soon-to-be-uploaded patch.

3593

For example, it seems to me that the Vfork checker might produce confusing false positives on Windows codebases.

At first, I started answering this question, but it doesn't look like vfork is part of the unix checks?

//There are two issues here:

1. Can we disable Vfork while still enabling the other unix.API checkers (open, pthread_once, calloc, malloc, realloc, alloca)?
2. Do Windows codebases ever use vfork?

//

[ariccio]

Rewrote the comment.

[dcoughlin]

The VForkChecker is in the Unix package (see lib/StaticAnalyzer/Checkers/Checkers.td).

Sorry if I wasn't clear before: I think the right approach is to continue enabling all unix checkers in non-Windows environments and selectively enable the unix checkers that make sense for Windows when analyzing for a Windows environment.

E.g,:

if (!IsWindowsMSVC) {
  CmdArgs.push_back("-analyzer-checker=unix");
} else {
  // Enable checkers in the "unix" package that make sense on Windows.
  CmdArgs.push_back("-analyzer-checker=unix.Malloc");
  CmdArgs.push_back("-analyzer-checker=unix.MallocSizeof");
  ...
}

This way we don't change behavior when targeting non-Windows platforms, we get improved checking on Windows, and we won't get any surprises on Windows when someone adds a new unix-only checker to the unix package in Checkers.td.

[ariccio]

In D16245#329529, @dcoughlin wrote:

The VForkChecker is in the Unix package (see lib/StaticAnalyzer/Checkers/Checkers.td).

Sorry if I wasn't clear before: I think the right approach is to continue enabling all unix checkers in non-Windows environments and selectively enable the unix checkers that make sense for Windows when analyzing for a Windows environment.

Actually, I agree with that. The issue is that the unix.API checkers aren't just unix-only APIs (of course they aren't), and APIs like open, calloc, realloc, and alloca, are most certainly used on Windows.

(I'm assuming the documentation for the unix.API checkers is accurate)

So I'm concerned that ignoring the unix.API checkers on Windows will miss quite a large number of issues. I'm weighing that against the possibility of false positive vfork diagnostics, which I don't yet understand.

My question is then, do Windows codebases actually run the risk of causing a vfork false positive? Since Windows doesn't have a vfork call, I can't think of a way to trigger a vfork false positive. By extension, I'm asking a bit about how the vfork checker identifies calls to vfork: does it do a (textual) string comparison?

E.g,:

[...]

This way we don't change behavior when targeting non-Windows platforms, we get improved checking on Windows, and we won't get any surprises on Windows when someone adds a new unix-only checker to the unix package in Checkers.td.

That's always a good idea.

Anyways, I've created a patch that enables all except the unix.API checkers, and can update this diff if we decide on that.

[dcoughlin]

I'm fine with explicitly turning on the unix.API checker on Windows. I'm not worried about warning about 'pthread_once()' on Windows, although 'valloc()' and 'alloca()' could be problematic, as these are POSIX APIs with names that could easily be used for other things on Windows codebases (same with 'vfork()'). If we do get reports about false positives for malloc()/alloca() on Windows, we could alter the checker to suppress alarms for only these APIs on Windows. We shouldn't enable unix.Vfork on Windows though -- that checker's functionality only makes sense on POSIX.

[ariccio]

In D16245#331319, @dcoughlin wrote:

I'm fine with explicitly turning on the unix.API checker on Windows. I'm not worried about warning about 'pthread_once()' on Windows, although 'valloc()' and 'alloca()' could be problematic, as these are POSIX APIs with names that could easily be used for other things on Windows codebases (same with 'vfork()'). If we do get reports about false positives for malloc()/alloca() on Windows, we could alter the checker to suppress alarms for only these APIs on Windows. We shouldn't enable unix.Vfork on Windows though -- that checker's functionality only makes sense on POSIX.

Here's the problem: I don't think that unix.Vfork is not documented as a checker!

I assumed (I can't remember why?) that vfork was part of the unix.API checkers, à la `unix.API.Vfork, but it's apparently it's own checker.

It makes much more sense now. I'll manually enable all but unix.Vfork in a soon-to-be-uploaded patch.

[ariccio]

Ok, I think that's what we're looking for?

[ariccio]

Hang on a second... where did 45581 come from?

[dcoughlin]

Because I committed with 'Differential Revision: http://reviews.llvm.org/D16245' in the commit message, Phabricator automatically updated the diff with the committed diff from SVN and closed the revision.

[ariccio]

Well, yeah... but were my comments too much? They're gone!

C:/LLVM/llvm/tools/clang/lib/Driver/Tools.cpp

if (isa<AnalyzeJobAction>(JA)) {
     CmdArgs.push_back("-analyzer-opt-analyze-nested-blocks");
 
     CmdArgs.push_back("-analyzer-eagerly-assume");
 
     // Add default argument set.
     if (!Args.hasArg(options::OPT__analyzer_no_default_checks)) {
       CmdArgs.push_back("-analyzer-checker=core");
 
-      if (!IsWindowsMSVC)
-        CmdArgs.push_back("-analyzer-checker=unix");
+      //Malloc & cstring checkers (also work on Windows)
 
       if (getToolChain().getTriple().getVendor() == llvm::Triple::Apple)
         CmdArgs.push_back("-analyzer-checker=osx");
 
       CmdArgs.push_back("-analyzer-checker=deadcode");
 
       if (types::isCXX(Input.getType()))
         CmdArgs.push_back("-analyzer-checker=cplusplus");