In D92886#2443176, @aaronpuchert wrote:

Non-static inline functions in C have confusing semantics.

I'm not terribly familiar with C, but judging from C11 6.7.4 it seems that C is a bit more permissive, but then says that certain things are implementation-defined. Whereas in C++ it's forbidden by the one-definition rule that an inline function has another external definition, it's allowed in C but implementation-defined which variant is called.

So if you do non-static inline functions like you'd do them in C++, I think you should be fine. But perhaps I'm missing something, in that case feel free to give me a pointer.

In D92886#2441290, @Quuxplusone wrote:

I agree with your reasoning, but in practice I still see a lot of people using static inline for functions (especially function templates) in .h files.

That's also what I was seeing, and then I wondered why our warnings don't catch this.

In D86308#2421237, @tambre wrote:

This has already been backported to 11.0.1: https://github.com/llvm/llvm-project/commit/03565ffd5da8370f5b89b69cd9868f32e2d75403

@tstellar, can we have this in 11.0.1? I've already ported it back in openSUSE because we were having problems with the CMake 3.19 update.

Thread safety attributes want callers of a function to have the same attribute, while this change wants callees to have the same attribute. So the attributes propagate in different directions.

In D67112#2398577, @rsmith wrote:

Looks fine as far as it goes, but it looks like we're also missing a cast in function pointer initialization via function conversion:
[...]

|-VarDecl 0x105143e8 <line:2:1, col:15> col:8 p 'void (*)()' cinit
| `-ImplicitCastExpr 0x10514498 <col:15> 'void (*)() noexcept' <FunctionToPointerDecay>
|   `-DeclRefExpr 0x10514450 <col:15> 'void () noexcept' lvalue Function 0x10514240 'f' 'void () noexcept'

It seems to depend on the standard, with -std=c++17:

Ping.

In D91494#2397260, @hans wrote:

I kind of liked that the script always downloads from github and doesn't depend on the state of my local repro.

Fixed the spelling issue in rGdea31f135ceae6e860e6301f9bb66d3b3adb1357.

Rebase and add a comment about the limitations of this implementation.

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

@rupprecht, maybe you can try it again?

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

In D88295#2365474, @rsmith wrote:

... where X has a volatile copy constructor and a volatile move constructor, I think we should produce the warning suggesting use of std::move.

If I read this correctly, we'd have a false negative with this patch, which is probably Ok given that this is an odd case.

In D88295#2365474, @rsmith wrote:

The other is whether implicitly move in a co_return statement. In that case, I think the use of CES_AsIfByStdMove is a mistake, and we should be using CES_Default there.

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

In D84604#2363768, @rupprecht wrote:

I applied D87194 locally and rebuilt the original source, and not only am I seeing the original issue (also firing on DoThings() when it should only be on DoStuff()), I'm also seeing: error: acquiring mutex 'lock' requires negative capability '!lock' [-Werror,-Wthread-safety-negative], where lock is a local variable, defined as MutexLock lock(mutex_).

Pushed a fix in rGbbed8cfe80cd27d3a47d877c7608d9be4e487d97. For now we just consider all static members as inaccessible, so we'll treat them as we did before this change.

In D84604#2363134, @rupprecht wrote:

I'm seeing failures which I think are due to this patch -- I don't have a nice godbolt repro yet, but it's something like:

Added the assertions in a follow-up change rGebfc427bbe08f0c36af9721d5a4e6d3ffe2e4bf5.

Can't really add anything to the discussion between @Quuxplusone and the author, just a few comments about the test.

Collect location of a trailing return type in the parser, use that for the warning.

LiteralPtrs aren't always globals, local variables are also translated that way. So we ask the stored ValueDecl if it isDefinedOutsideFunctionOrMethod. That seems like the right thing.

Almost forgot about that. I think I've figured it out.

Use FindPython3 and remove shebang. The script didn't have an executable bit anyway.

Maybe we'll have to clarify this further in the future, but for now this is an improvement.

In D87629#2280475, @ajtowns wrote:

Sure, it makes perfect sense that it's too hard.

It's not really too hard, there is an existing parameter somewhere in the CFG generation that controls these exception handling edges, and we'd probably have to change some other diagnostics plus improve the performance/memory usage.

Looks pretty good, thanks! I think this clears up the misunderstandings.

In D87629#2278383, @ajtowns wrote:

Not sure the try { AssertHeld } catch (...) { a = 0; } example reveals very much: it seems like thread safety annotations aren't checked within a catch block at all?

Mutex m;
int i GUARDED_BY(m);

static void thrower() { throw std::exception(); }
void f() { i = 1; }
void g() {
    try {
        thrower();
    } catch (...) {
        i = 5;
    }
    i = 6;
}

gives me warnings for i=1 and i=6 but not i=5 ?

In D87629#2275639, @ryanofsky wrote:

The mistakes about exceptions came from me taking "(no return)" in the previous documentation too literally thinking it was referring to https://en.cppreference.com/w/cpp/language/attributes/noreturn.

The key here is the word "assumed". We treat the function as if it looks like this:

You can view it this way: there is a dynamic set and a static set of capabilities. The static set is always the same at any particular point in a function, regardless of the circumstances we're called from. It's what we determine in the analysis. The dynamic set depends on who called us and could be greater. To illustrate the difference:

We don't really have a good understanding of ASSERT_CAPABILITY ourselves. For example, there is this loophole:

Add some prose, not just code. Otherwise our list of attributes would be incomplete.

Add tests with qualified names, let tests rely on shadowing.

• More detailed description how scoped capabilities work.
• Make the comment wording more consistent with existing comments and the previous explanation.
• Properly implement the destructor in the presence of an Unlock() function: we need to keep track of the status then.
• Add try-acquire functions on scoped lock.
• Fix compiler errors.

Not sure about the added comments, I can also remove them if they're not helpful.

Fix as suggested by @rsmith instead: set InvalidDecl directly.

Everything compiles with ValueDecl* Decomp instead of a LazyDeclPtr, but I'll leave it until we figure out whether we might actually need it.

I'll go with what @rsmith proposed to fix the bug. If the entire deserialization process doesn't rely on invariants, the order should indeed be irrelevant.