This is an archive of the discontinued LLVM Phabricator instance.

[clang analysis][thread-safety] Handle return-by-reference...
AcceptedPublic

Authored by courbet on Jun 16 2023, 6:05 AM.

Details

Summary

...of guarded variables, when the function is not marked as requiring locks:

class Return {
  Mutex mu;
  Foo foo GUARDED_BY(mu);

  Foo &returns_ref_locked() {
    MutexLock lock(&mu);
    return foo;  // BAD
  }

  Foo &returns_ref_locks_required() SHARED_LOCKS_REQUIRED(mu) {
    return foo;  // OK
  }
};

This is implemented as -Wthread-safety-return and not part of
-Wthread-safety for now.

Diff Detail

Event Timeline

courbet created this revision.Jun 16 2023, 6:05 AM
Herald added a project: Restricted Project. · View Herald Transcript
courbet requested review of this revision.Jun 16 2023, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2023, 6:05 AM
aaronpuchert added a subscriber: aaronpuchert.

Thanks for working on this! Someone recently pointed out to me that we have a gap there.

clang/include/clang/Basic/DiagnosticGroups.td
1046–1047

Why not under -Wthread-safety-reference, as it's return-by-reference that you're warning on? This seems too small for a separate flag to me.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3805

Or do you expect more warnings on return?

clang/lib/Analysis/ThreadSafety.cpp
2157–2159

Wouldn't it be more straightforward to check the actual return type? We have the FunctionDecl and could store it in ThreadSafetyAnalyzer instead of CurrentMethod.

2160

You're presumably collecting them because automatic destructor calls are after return in the CFG, right?

If that's the case, can't we immediately check against the declared exit set? It should be known before we walk the CFG, unless I'm missing something.

aaronpuchert added inline comments.Jun 16 2023, 5:08 PM
clang/lib/Analysis/ThreadSafety.cpp
2163

Also wondering why we're doing this—no other visitor function seems to bother the VisitorBase = ConstStmtVisitor<BuildLockset>. Are these not just empty fallbacks?

courbet updated this revision to Diff 532558.Jun 19 2023, 1:18 AM
courbet marked an inline comment as done.

Address review comments

Thanks.

clang/include/clang/Basic/DiagnosticGroups.td
1046–1047

The main reason it so that we provide a soft transition period for users: If we put that in -Wthread-safety-reference, we'll start breaking compile for people who use -Werror, while a separate flag allows a transition period where people opt into the new feature.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3805

We could do pointers too, but arguably pointers and references are the same.

clang/lib/Analysis/ThreadSafety.cpp
2157–2159

Good point. I've also added better checking and diagnostics for const (shared) vs mutable (exclusive) locks, with more tests.

2160

You're presumably collecting them because automatic destructor calls are after return in the CFG, right?

Exactly.

If that's the case, can't we immediately check against the declared exit set? It should be known before we walk the CFG, unless I'm missing something.

Given how the code was written I was under the impression that we only knew the entry set after walking the whole CFG (we're getting ExpectedExitSet after we walk the CFG). But now I see that we're actually adressing the entry blok beforehand. Thanks for the suggestion, this makes the code much simpler indeed !

2163

The base code is hard to read because i't full of macros, but it looks like it't probably empty indeed - done.

aaronpuchert added inline comments.Jun 25 2023, 3:30 PM
clang/include/clang/Basic/DiagnosticGroups.td
1046–1047

Transition flags can end up resulting in more churn, assuming that we eventually want to put this under -Wthread-safety-reference, because then you have two categories of users:

  • those that opted in will eventually have to remove the flag again, and
  • those that didn't will get hard errors on updating the compiler at that point.

Of course you might argue that the latter case can be prevented by carefully reading the release notes, but we know how often that happens.

I'd argue that if you're using -Wthread-safety-reference, you're already opting into warnings on escaping references, and not warning on return is a false negative.

A separate flag would make sense to me if we want to keep it, for example because this produces a substantial amount of false positives under some circumstances. Did you try this on a larger code base that's using the annotations? I could try it on our code, and maybe we can get some Googler to test it on theirs, which is also heavily using Thread Safety Analysis. (Though I'm not sure whether they use -Wthread-safety-reference.)

courbet added inline comments.Jun 26 2023, 6:54 AM
clang/include/clang/Basic/DiagnosticGroups.td
1046–1047

I don't have a strong opinion for where the warning should go. We are indeed using -Wthread-safety-reference, though we're not enabling -Werror on these, so adding more warnings is fine for us.

I've run the check on a small sample of our codebase (which I don;t claim to be representative, I can do a larger analysis if needed). The warnings are more or less evenly split between missing annotations and actual bugs. I don't think any of the things I've seen qualify as false positives.

Among the missing annotations, most of the warnings are missing ABSL_EXCLUSIVE_LOCKS_REQUIRED on the function. In a small number of cases, the pattern is that a variable is lazily initialized under a lock and then returned by reference:

struct LazyImmutableThing {
  const Thing& Get() {
    {
      MutexLock lock(&mutex_);
      thing_->Initialize();
    }
    
    return thing_;
  }
  
  Mutex mutex_;
  Thing thing_ GUARDED_BY(mutex_);
};

I consider this to be a missing annotation as the returned value is dynamically immutable, the proper fix would be return TS_UNCHECKED_READ(thing_).

Most actual bugs are along the lines of:

struct S {
  T& Get() const {
    MutexLock lock(&mutex_);
    return obj_;
  }

  Mutex mutex_;
  T obj_ GUARDED_BY(mutex_);
};

though some are missing the guard altogether (T& Get() const { return obj_; }).

There are a few possible fixes. In rough order of occurrence:

  • Return by value as the copy is not too expensive and memory ordering does not matter.
  • Let the caller take the lock and annotate with ABSL_EXCLUSIVE_LOCKS_REQUIRED when the Get method is not called too often.
  • Let Get take a callback and process the value under the lock instead of returning it (when ordering matters).
courbet updated this revision to Diff 534539.Jun 26 2023, 7:01 AM

Put the new warnings in -Wtread-safety-reference

Tried this on our code base, and the number of new warnings seems acceptable. I'll still need to look through them in more detail, but there is one suspicious warning that boils down to this:

> cat reference-bug.cpp
struct __attribute__((capability("mutex"))) Mutex {} mu;
int* p __attribute__((pt_guarded_by(mu)));

int& f() {
  return *p;
}
> clang-16 -fsyntax-only -Wthread-safety-analysis reference-bug.cpp
> clang-16-D153131 -fsyntax-only -Wthread-safety-analysis reference-bug.cpp
reference-bug.cpp:5:11: warning: writing the value pointed to by 'p' requires holding mutex 'mu' exclusively [-Wthread-safety-analysis]
  return *p;
          ^
1 warning generated.

That we're warning here is correct, but the warning message is a bit off (we're not quite writing here), and it's under -Wthread-safety-analysis instead of -Wthread-safety-reference.

clang/include/clang/Basic/DiagnosticGroups.td
1046–1047

In a small number of cases, the pattern is that a variable is lazily initialized under a lock and then returned by reference:

I wonder why that's safe, is the initialization guarded to happen only once? Some kind of double-checked locking pattern perhaps? Otherwise it seems that reads could happen in parallel to writes. If it's a checked initialization, then I think the proper way to model this is:

  • The initialization acquires a lock to exclude other initializations running in parallel. Reads cannot happen, because the reference has not yet escaped.
  • After initialization, we essentially acquire an implicit shared lock. This is not tracked as a proper lock, but it doesn't need to: there are no more writes until the end of lifetime, so nobody will acquire another exclusive lock.

One could model this by creating a mutex wrapper that can be locked once in exclusive mode, and after that hands out shared locks to everybody who wants one without keeping track. (As this is slightly off-topic, we don't need to discuss this here though.)

Other than than, this matches what I'm seeing in our code.

courbet updated this revision to Diff 535418.Jun 28 2023, 8:10 AM

Add a specific message for return-by-ref of pt_guarded_by variable.

Tried this on our code base, and the number of new warnings seems acceptable. I'll still need to look through them in more detail, but there is one suspicious warning that boils down to this:

> cat reference-bug.cpp
struct __attribute__((capability("mutex"))) Mutex {} mu;
int* p __attribute__((pt_guarded_by(mu)));

int& f() {
  return *p;
}
> clang-16 -fsyntax-only -Wthread-safety-analysis reference-bug.cpp
> clang-16-D153131 -fsyntax-only -Wthread-safety-analysis reference-bug.cpp
reference-bug.cpp:5:11: warning: writing the value pointed to by 'p' requires holding mutex 'mu' exclusively [-Wthread-safety-analysis]
  return *p;
          ^
1 warning generated.

That we're warning here is correct, but the warning message is a bit off (we're not quite writing here), and it's under -Wthread-safety-analysis instead of -Wthread-safety-reference.

Right. I was relying on the fallback "imprecise" warning for this one. I added a pt_garded value just like for pass_by_value and added a test.

courbet added inline comments.Jun 29 2023, 11:56 PM
clang/include/clang/Basic/DiagnosticGroups.td
1046–1047

I wonder why that's safe, is the initialization guarded to happen only once? Some kind of double-checked locking pattern perhaps?

Yes, it looks like this:

const T& get() const {
    if (!value_set_.load(std::memory_order_acquire)) {
      MutexLock lock(&lock_);
      if (!value_set_.load(std::memory_order_relaxed)) {
        value_ = ComputeValue();
        value_set_.store(true, std::memory_order_release);
      }
    }
    return value_;
  }

I ended up silencing the return with a comment:

// `value_` is set once an for all, it won't change after this function returns.
 return ABSL_TS_UNCHECKED_READ(value_);

I agree that this is not very principled, but it is simple :)

One could model this by creating a mutex wrapper that can be locked once in exclusive mode, and after that hands out shared locks to everybody who wants one without keeping track.

This is a cool idea. If I understand correctly, it does mean that the *caller* of get has to grab a untracked shared lock ?

Sorry for letting this collect dust. I think it's a valuable addition, and looks pretty good, except that I think we should use the expected exit set instead of the entry set. These can be legitimately different for appropriately annotated functions, i.e. with acquire_(shared_)capability or release_(shared_)capability.

Apart from the bug mentioned earlier, which should now be fixed, this looks good on our code base. I have to admit a separate flag could be nice for migration, but it should be included by default in -Wthread-safety-reference. But I'm not sure if we need it.

clang/include/clang/Basic/DiagnosticGroups.td
1046–1047

This looks like the Double-checked locking pattern. I've come to the conclusion that guarded_by annotations are not appropriate for them (just copying what I wrote to someone internally):

  • The initialization flag (here value_set_) is read before acquiring the mutex, which means it can't be protected by the mutex.
  • The content (here value_), if already initialized, is also read without acquiring the mutex, so it can't be protected by it. The content, if only readable, is "protected" by atomic ordering: the initialization must have a release barrier on (or before) writing the initialization flag, which synchronizes with an acquire barrier on (or after) the initial read of the initialization flag. If the content is writable, it will need to be synchronized on its own.
  • The mutex might protect data being used in the initialization. Other than that it's just a performance optimization: having multiple threads initialize the same object would be wasteful. Especially since the pattern makes only sense for expensive initialization. (Expensive either in time or memory consumption.)
  • Lastly, this pattern is usually used in a single function, so there is no risk of forgetting the mutex.
clang/lib/Analysis/ThreadSafety.cpp
44

I wonder where we're using that, is this the leftover of an earlier version?

1248
1546–1547

Shouldn't it be the (expected) exit set if we're talking about return? Also I'd suggest (Function)EntryFSet (or with Exit).

2298

Maybe it makes sense to keep an assertion here like assert(*SortedGraph->begin() == &CFGraph->getEntry());.

2302
2494–2512

Here we build the ExpectedExitSet. You might have to move this if we're using it earlier.

clang/test/SemaCXX/warn-thread-safety-analysis.cpp
5630

For the entry/exit set issue, can you add a function that acquires a mutex (and doesn't release it), returning something protected by the mutex? And maybe one that releases but doesn't acquire.

courbet updated this revision to Diff 556940.Sep 18 2023, 4:53 AM
courbet marked 7 inline comments as done.
  • Check return values against the exit set rather than the entry set, add unit tests.
  • Address other cosmetic review comments

I think we should use the expected exit set instead of the entry set.

Indeed, I've added a few tests to check this. Let me know if you see any other tests that might be valuable.

aaronpuchert accepted this revision.Sep 18 2023, 1:09 PM
aaronpuchert added a subscriber: rupprecht.

Looks good to me, but let's wait a few days in case @aaron.ballman has anything to add.

@rupprecht, in case you're still doing integration at Google, perhaps you can test this patch. If you're seeing too many new warnings, we might introduce a flag to turn this off (temporarily).

clang/lib/Analysis/ThreadSafety.cpp
1546

Or drop the parenthesized part.

2298–2299

You might want to do the * -> & in a separate commit.

This revision is now accepted and ready to land.Sep 18 2023, 1:09 PM
courbet updated this revision to Diff 557015.Sep 19 2023, 2:05 AM
courbet marked an inline comment as done.

Rebase on NFC changes

Thanks for the review.

clang/lib/Analysis/ThreadSafety.cpp
2298–2299
aaronpuchert accepted this revision.Sep 26 2023, 3:23 PM

Looks still good to me. As I wrote on D153132, I don't think we need it anymore, but if you disagree I think I can accept it as well.

Looks still good to me. As I wrote on D153132, I don't think we need it anymore, but if you disagree I think I can accept it as well.

Sorry, I misunderstood the last comment as an endorsement of the change. I've reverted the base commit and I'll rebase this on main without the changes.

Looks still good to me. As I wrote on D153132, I don't think we need it anymore, but if you disagree I think I can accept it as well.

Sorry, I misunderstood the last comment as an endorsement of the change. I've reverted the base commit and I'll rebase this on main without the changes.

After trying a rebase I think the code is better with the base change. See my comments in https://reviews.llvm.org/D153132.

aeubanks added a subscriber: aeubanks.EditedOct 6 2023, 4:46 PM

This is finding lots of real issues in code, which is awesome, but could I request that this be put under a separate warning flag so we can toggle off just the new functionality and turn it on as we clean our codebase? e.g. -W[no-]thread-safety-analysis-return

edit: now I see the previous discussion on a separate subflag. on Chromium's codebase this is hitting lots of times and is too much to clean up all at once since some of it is hard to fix as somebody who's not an expert in the codebase.

This is finding lots of real issues in code, which is awesome, but could I request that this be put under a separate warning flag so we can toggle off just the new functionality and turn it on as we clean our codebase? e.g. -W[no-]thread-safety-analysis-return

Fine for me, but we might want to remove it again after one or two releases. I'm not sure how to communicate that this is just a “transitory” flag.

And it should be included by default in -Wthread-safety-reference, so that users of that flag see the new warnings/errors, and can demote them to warnings while fixing them. To emphasize the subflag status, I'd suggest something like -Wthread-safety-reference-return.

This is finding lots of real issues in code, which is awesome, but could I request that this be put under a separate warning flag so we can toggle off just the new functionality and turn it on as we clean our codebase? e.g. -W[no-]thread-safety-analysis-return

Fine for me, but we might want to remove it again after one or two releases. I'm not sure how to communicate that this is just a “transitory” flag.

And it should be included by default in -Wthread-safety-reference, so that users of that flag see the new warnings/errors, and can demote them to warnings while fixing them. To emphasize the subflag status, I'd suggest something like -Wthread-safety-reference-return.

I also had some push back internally on adding this to the existing flag. I'm going to add -Wthread-safety-reference-return, can we start by not temporarily including it in -Wthread-safety-reference so that we can see how much work it it to fix those warnings ?

I also had some push back internally on adding this to the existing flag. I'm going to add -Wthread-safety-reference-return, can we start by not temporarily including it in -Wthread-safety-reference so that we can see how much work it it to fix those warnings ?

Can you elaborate on this? What's the reasoning? Here are two reasons for having it as part of -Wthread-safety-reference right from the beginning:

  • -Wthread-safety-reference is already separate from -Wthread-safety-analysis because passing a reference does not imply an access. If you have the warning you're arguably already opting into this, and I don't see much of a difference between passing via parameter versus passing by return.
  • Most users don't follow all reviews or read the release notes in detail and won't notice the new flag until it shows up in their build log. So we'd just lose time.

Since warning messages always indicate the warning flag and thus make disabling it easy, I don't see an issue with enabling it right away as part of -Wthread-safety-reference.

Lastly, this doesn't seem complicated enough to warrant extended beta testing.

If people are passing -Wthread-safety-reference, there was clearly some value in the previous checks and it would be unfortunate to turn them off while fixing the codebase.
I'm not super familiar with flag families and if what I'm proposing is easily doable, but I think ideally we would keep this new functionality turned on by default under -Wthread-safety-reference and make just this new functionality toggleable under a subflag -W[no-]thread-safety-reference-return.

I also had some push back internally on adding this to the existing flag. I'm going to add -Wthread-safety-reference-return, can we start by not temporarily including it in -Wthread-safety-reference so that we can see how much work it it to fix those warnings ?

Can you elaborate on this? What's the reasoning? Here are two reasons for having it as part of -Wthread-safety-reference right from the beginning:

  • -Wthread-safety-reference is already separate from -Wthread-safety-analysis because passing a reference does not imply an access. If you have the warning you're arguably already opting into this, and I don't see much of a difference between passing via parameter versus passing by return.
  • Most users don't follow all reviews or read the release notes in detail and won't notice the new flag until it shows up in their build log. So we'd just lose time.

Since warning messages always indicate the warning flag and thus make disabling it easy, I don't see an issue with enabling it right away as part of -Wthread-safety-reference.

Lastly, this doesn't seem complicated enough to warrant extended beta testing.

We have a large number of users of -Werror -Wthread-safety-analysis internally. When we make the new warnings part of that flag we cannot integrate because we're breaking all these users. If we don't integrate we can't run the new analysis to see what we would need to fix.

Introducing a new flag allows us to:

  • keep the current analysis running for users of -Wthread-safety-analysis.
  • progressively add -Wthread-safety-analysis-reference-return to these users across the codebase, fixing them or disabling analysis as needed.

We have a large number of users of -Werror -Wthread-safety-analysis internally. When we make the new warnings part of that flag we cannot integrate because we're breaking all these users.

The proposal was to include it in -Wthread-safety-reference, not -Wthread-safety-analysis. See https://clang.llvm.org/docs/DiagnosticsReference.html#wthread-safety for the existing flags and their relations.

If we don't integrate we can't run the new analysis to see what we would need to fix.

Can you not add -Wno-error=thread-safety-reference-return together with the integration? Or are there too many places adding it independently?

Introducing a new flag allows us to:

  • keep the current analysis running for users of -Wthread-safety-analysis.
  • progressively add -Wthread-safety-analysis-reference-return to these users across the codebase, fixing them or disabling analysis as needed.

That is true, but these advantages seem to apply to a small number of users only (those aware of the new flag). If you integrate Clang trunk, it would be Ok if you leave it off by default for a couple of weeks, but turn it on before the next release.

I'm not generally against new flags, but this is more of a "gap closing" than a new feature, so an on-by-default (under -Wthread-safety-reference, not -Wthread-safety-analysis) warning should be the right choice. Changes that result in new warnings are not uncommon, and often we don't create a new flag for them at all. Here it's Ok due to the large number of warnings, but it fits too well into -Wthread-safety-reference to not be triggered by that.

We have a large number of users of -Werror -Wthread-safety-analysis internally. When we make the new warnings part of that flag we cannot integrate because we're breaking all these users.

The proposal was to include it in -Wthread-safety-reference, not -Wthread-safety-analysis. See https://clang.llvm.org/docs/DiagnosticsReference.html#wthread-safety for the existing flags and their relations.

Sorry, I meant -Wthread-safety-reference.

If we don't integrate we can't run the new analysis to see what we would need to fix.

Can you not add -Wno-error=thread-safety-reference-return together with the integration? Or are there too many places adding it independently?

Yes, we have way too many instances. I'm going to discuss with people dealing with integrates to see whether disabling the new flag globally is a possibility.

thakis added a subscriber: thakis.Oct 13 2023, 5:03 AM

FWIW, the standard procedure for adding new functionality to existing warnings is (assuming that it makes the warning fire a lot, else no extra group is needed):

  • Add it in a subgroup with its own flag
  • Enable it by default

The reasoning is that people who aren't ready for the warning yet can then turn it off with the new flag, and everyone becomes aware of the new warning. If it's off-by-default, nobody will ever know about it.

(Of course, only warnings that are useful and high-signal should be added in the first place, so this is assuming that it's a warning with a very low false positive rate.)

I've updated https://github.com/llvm/llvm-project/pull/68572 to do as suggested.

To sum up: we have a new flag -Wthread-safety-reference-return, which is on by default under -Wthread-safety-reference.