Page MenuHomePhabricator

[analyzer] StackAddrEscape: Delay turning on by default a little bit?
ClosedPublic

Authored by NoQ on Dec 8 2017, 4:59 PM.

Details

Summary

I'm seeing false positives on the new check added in D39438 of the following kind:

void foo() {
  T buf[16];
  for (int n = 0; n < 16; ++n) {
    T *ptr = &buf[n];
    dispatch_async(queue, ^{ use(ptr); });
  }
  dispatch_barrier_sync(queue, ^{});
}

In this case, pointer to the local variable buf is captured by a block that goes into dispatch_async, yet it is not a bug because synchronization ensures that this block quits before the variable goes out of scope.

I guess it's kinda similar to the semaphore synchronization case. We could probably add another suppression for functions that contain any dispatch_barrier_async calls. The ideal solution is to delay the warning until checkEndFunction (like the escape-to-global check does) to see if things get synced up until then (of course, once we have scopes we can make it even more fine-grained).

Alexander, do you think we should keep the checker under a flag (not enabled for regular users) until we get a better understanding of what kinds of synchronizations can get on the way? This diff splits your new check into alpha.core.StackAddressAsyncEscape. Or do you already have good quick fixes coming?

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Dec 8 2017, 4:59 PM
NoQ edited the summary of this revision. (Show Details)Dec 8 2017, 5:00 PM
NoQ updated this revision to Diff 126247.Dec 8 2017, 5:03 PM

Update the other run-line.

alexshap accepted this revision.Dec 8 2017, 9:40 PM

i see, to be honest, this is kind of unfortunate, if i am not mistaken, i've seen these false-positives, but not too many, most reports were real bugs. But if it's annoying, than i think it's fine to make it "alpha" until i figure out a good way to workaround this issue.

This revision is now accepted and ready to land.Dec 8 2017, 9:40 PM
NoQ added a comment.Dec 11 2017, 9:38 AM

Yeah, we usually try to avoid omissions of modeling in on-by-default checkers because the user may accidentally run into projects in which the unmodeled idiom is common, and then he'd get false positives all over the place. In my case it was just two new positives, both false due to this dispatch_barrier_sync idiom.

NoQ updated this revision to Diff 126390.Dec 11 2017, 9:44 AM

Add a FIXME test.

This revision was automatically updated to reflect the committed changes.