Page MenuHomePhabricator

Thread safety analysis: Use access specifiers to decide about scope
Changes PlannedPublic

Authored by aaronpuchert on Sep 5 2020, 8:25 AM.

Details

Reviewers
aaron.ballman
Summary

Public members are always in scope, protected members in derived classes
with sufficient access.

Diff Detail

Unit TestsFailed

TimeTest
400 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

aaronpuchert created this revision.Sep 5 2020, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2020, 8:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaronpuchert requested review of this revision.Sep 5 2020, 8:25 AM
aaronpuchert planned changes to this revision.Oct 30 2020, 8:03 AM

Need to rebase this on top of D84604 plus a subsequent fix.

aaron.ballman added inline comments.Nov 2 2020, 8:47 AM
clang/lib/Analysis/ThreadSafety.cpp
1317

This should probably have a FIXME?

1323–1325

llvm::any_of()?

clang/test/SemaCXX/warn-thread-safety-negative.cpp
168

Given that this is touching on declaration contexts, it would also be good to have some tests checking for differences between declaration contexts (like out-of-line method definitions)

aaronpuchert added inline comments.Nov 2 2020, 1:29 PM
clang/test/SemaCXX/warn-thread-safety-negative.cpp
168

Isn't there a semantic and a lexical declaration context, and aren't we consistently using the semantic context?

aaronpuchert marked 2 inline comments as done.

Rebased on D84604, included static members, and addressed review comments.

@rupprecht, maybe you can try it again?

@rupprecht, maybe you can try it again?

Some more interesting errors this time :)

The ones I originally saw look correct now (i.e. it's flagging the things that are valid, but not the things out of visibility). I tried building the rest of this package, and I guess scoping isn't considered in this case though?

class Foo {
 public:
  static void Bar();
 private:
  struct Params {
    Mutex mu_;
  }
  static Params* GetParams();
};

// In the .cc file:
void Foo::Bar() {
  Params& params = *GetParams();
  MutexLock lock(params.mu_);  // error: acquiring mutex 'params.mu_' requires negative capability '!params.mu_'
}

/* static */ Params* Foo::GetParams() {
  static Params params;
  return &params;
}

On one hand, it's totally valid. On the other hand, annotating the method like static void Bar() REQUIRES(!params.mu_); isn't possible because params is a local variable.

(I'm new to threading analysis, so maybe I'm just using the wrong annotations)

aaronpuchert planned changes to this revision.Nov 3 2020, 12:32 PM

That's a good point, while mu_ is public, params is a local variable.

I need to take into account the left-hand side of a til::Project, which we're currently ignoring.