Ownership attribute for malloc checking, revised patch

Regenerated patch against an SVN pull today.

This seems complete.

No new tests fail with this patch, and it passes all its own tests.

I have compiled large portions of an embedded Linux distribution with checking, and given annotations on commonly used container functions, this reduces the false positive rate for malloc and null pointer dereference checks enormously. Without ownership attributes, approx 10k warnings, with attributes in only a few headers approx 2k warnings (mostly glib headers plus a few in the proprietary parts of the code), and a sampling of those shows that they are mostly real problems (if not likely to occur in actual execution, or of much significance). We have a checked build in our nightly runs, and it has already found the root cause of a number of real issues.

The patch itself is a git diff since I am working with git-svn, I have options for regenerating it if that’s not suitable.

Please review.

Thanks,

Andrew McGregor
Allied Telesis Labs

clang-ownership-withtests-r3.patch (32 KB)

Hi Andrew,

This is looking good, but there are a few things that are worth addressing. A few specific comments:

+void OwnershipAttr::Destroy(ASTContext &C) {
+ if (ArgNums)
+ C.Deallocate(ArgNums);
+ AttrWithString::Destroy(C);
+}

So, here it is again. This version addresses all your comments, I think. I went a bit further in deleting unnecessary methods etc.

Passes all its tests on OS X (previous patch was tested on Linux).

Andrew

clang-ownership-withtests-r4.patch (33.4 KB)

Looking good. Here are a few comments related to your recent changes:

  • // If ptr is NULL, no operation is performed.
  • if (nullState) {
  • if (!notNullState) {
  • return;
  • }
  • }

There are some tabs in here. Also, this can be written more succinctly:

if (nullState && !notNullState)
return;

Looking good. Here are a few comments related to your recent changes:

  • // If ptr is NULL, no operation is performed.
  • if (nullState) {
  • if (!notNullState) {
  • return;
  • }
  • }

There are some tabs in here. Also, this can be written more succinctly:

if (nullState && !notNullState)
return;

Yes, of course. As for the tabs… I don’t get that, I think I have to spend some time with editor documentation to figure that one out.

  • SymbolRef Sym = val.getLocSymbolInBase();
  • if (Sym) {
  • const RefState *RS = state->get(Sym);

I think you want to use ‘notNullState’ here, since your assumption is that this transition occurs only when the value is non-null. When ‘nullState’ is not null, you’ll also probably want a transition so that path has consistent checking of the nullity of the symbol.

Not sure I understand… the RegionState is used because I’m getting at and assuming forward the semantics of the memory region being assigned away, and not touching the null or otherwise assumptions. Should I be using notNullState to assert that the assignee is not null?

Minor style nit:

state->set (S…)
isa (MS)

Please don’t write your function calls that way (with the extra space). It’s inconsistent with the rest of the codebase, and it’s not even consistent within the file.

Ok. Not sure how that happened… too much fiddling around with editors again (this has been touched by Eclipse, Emacs and TextMate… things have been a bit chaotic… I think this one is Eclipse’s fault).

  • SymbolRef Sym = val.getLocSymbolInBase();
  • if (Sym) {
  • const RefState *RS = state->get(Sym);

I think you want to use ‘notNullState’ here, since your assumption is that this transition occurs only when the value is non-null. When ‘nullState’ is not null, you’ll also probably want a transition so that path has consistent checking of the nullity of the symbol.

Not sure I understand… the RegionState is used because I’m getting at and assuming forward the semantics of the memory region being assigned away, and not touching the null or otherwise assumptions.

But the non-nullness is a precondition for the checker state to change, so this should be recorded as part of the “assumptions”.

Should I be using notNullState to assert that the assignee is not null?

Yes basically.

I’ll try and explain with an example. Suppose we were analyzing:

foo(p);
bar(p);

and ‘foo’ and ‘bar’ modified some checker-state (like your checker does) associated with p, but only if p is not null. Conceptually there are only two possible ‘nullity’ cases for ‘p’:

(1)

foo(p); // we check if ‘p’ is null, and assume it is null
bar(p); // ‘p’ is still null

(2)

foo(p); // we check if ‘p’ is null, and assume it isn’t; go modify checker state
bar(p); // ‘p’ is not null; go modify checker state

However, with what you have, we basically have 4 possibilities; I’ll list the two additional ones:

(3)

foo(p); // we check if ‘p’ is null, and assume it is null
bar(p); // we don’t record that we checked if ‘p’ is null, we check again and assume that it isn’t null; modify state

(4)

foo(p); // we check if ‘p’ is null, and assume it isn’t; go modify checker state
bar(p); // we don’t record that we checked if ‘p’ is null, we check again and assume that it is null and do nothing

What you are seeing from (3) and (4) is part of (I believe) the “frame problem” from AI. By having the following statement use ‘state’ instead of ‘notNullState’, you are forgetting the nullity check:

// We no longer own this pointer.
state = state->set(Sym, RefState::getRelinquished(StoreE));

When you use the ‘assume’ function, ‘state’ is not modified. Instead it returns two new states; one where the assumption is true and the other where it is false. If your modification to the checker state implies that the pointer was non-null, you should be using the ‘notNullState’ here instead of ‘state’ as that is a requirement for the checker state to change.

Similarly, if you add a transition where ‘notNullState’ is non-null and don’t add a transition for ‘nullState’, then you will prune out paths where ‘p’ is null. Essentially this would prune out path (1).

Does this make any more sense?

Ok, here’s a version that I think has the state transitions right.

Andrew

clang-ownership-withtests-r5.patch (32.5 KB)

Looks good! Do you have commit access, or should I go and apply it?

Actually, I think we need transitions for ‘nullState’ as well (per my comment in my other email).

Ok, null transitions done (for both FreeMemAux and PreVisitBind).

Thanks for the comments,

Andrew

clang-ownership-withtests-r6.patch (32.5 KB)

Hi Andrew,

I’ve gone ahead and committed these changes:

http://llvm.org/viewvc/llvm-project?view=rev&revision=109939

Thanks so much for working on this! I took the liberty of changing some of the transitions for PreVisitBind, because there was subtleties with adding transitions for nullState that I don’t feel I explained well. I also made the bifurcation of the paths more lazy; specifically we only bifurcate the path when the pointer has a RefState. I think it looks right (although I added a few FIXMEs); please look it over and let me know if anything looks weird or wrong.

Cheers,
Ted

Thanks :slight_smile:

I think it’ll be a couple of days before I can come back on this, since I’m typing this at Schiphol airport about to fly back to NZ via Hong Kong… I might get a chance to look at it in-flight, but chances are connectivity will be sparse.

Andrew