Page MenuHomePhabricator

Enable "Unix" checks for Windows
ClosedPublic

Authored by ariccio on Jan 15 2016, 2:31 PM.

Details

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)

Diff Detail

Repository
rL LLVM

Event Timeline

ariccio updated this revision to Diff 45039.Jan 15 2016, 2:31 PM
ariccio retitled this revision from to Enable "Unix" checks for Windows.
ariccio updated this object.

(build seems fine)

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

dcoughlin edited edge metadata.Jan 15 2016, 3:23 PM

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 ↗(On Diff #45039)

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 ↗(On Diff #45039)

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?

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 ↗(On Diff #45039)

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

3593 ↗(On Diff #45039)

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 updated this revision to Diff 45090.Jan 16 2016, 11:19 PM
ariccio edited edge metadata.

Rewrote the comment.

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 marked 2 inline comments as done.Jan 19 2016, 10:10 PM

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.

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.

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 updated this revision to Diff 45488.Jan 20 2016, 9:04 PM
ariccio marked 2 inline comments as done.

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

This revision was automatically updated to reflect the committed changes.

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

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.

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