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.

Ah, I guess Ianded on an older version of this through a link and didn't notice. Nevermind.

In D86207#2251802, @riccibruno wrote:

Is my comment on the deserialization of BindingDecls in https://reviews.llvm.org/D85613?id=284364 related to this change?

Do I understand correctly that this means we should have a template parameter, but haven't read it yet? In that case it would seem strange that we consider them different.

Rebase on top of D84603.

Remove "holding" and moved to a separate message because the overlap was getting smaller.

In D67113#2244463, @aaron.ballman wrote:

There may be a deeper issue here in that there are four standard conversion sequences, not three: http://eel.is/c++draft/conv#1, but otherwise this change seems correct to me (function conversion is certainly not in the first group).

Ping. Maybe this change is too silly, but I think it's better to be precise and not muddle qualification conversions together with casting away noexcept.

Maybe this is to trivial for a review. The comment on StandardConversionSequence::Third in clang/Sema/Overload.h says

@aaron.ballman, can you have a look again? I think this change is consistent with what we're already doing.

In D85691#2232079, @cuviper wrote:

FWIW, I also tagged bug 47123 for release-11.0.0.

@hans, could we bring this to the release-11.x branch?

Ping.

Thanks, this looks good to me.

First of all thanks for doing this, I was observing the same issue on arvm6l and armv7l.

In D81922#2095834, @MaskRay wrote:

Can you please add some messages about duplicate symbols?

Test failure is expected because I based this on D84603 locally, but I'm not sure how to tell that to Phabricator.

@anarazel, that should fix the issue you reported on IRC.

In D83549#2144625, @MaskRay wrote:

@aaronpuchert Your comment https://bugzilla.opensuse.org/show_bug.cgi?id=1155108#c10 "However, it's actually a bug in all three linkers. It has been fixed in lld just today [1]" made me wonder whether https://bugs.llvm.org/show_bug.cgi?id=43927 was really a linker bug.

The linkers were behaving differently though: lld and bfd dropped the FINI entirely whereas gold had FINI=0, which lead to a crash. Which of these behaviors would be expected?

Yup, c-index-test crashing was one of the motivators behind this.

@arsenm, just FYI: you probably just overlooked this when removing the header.

In D81332#2079489, @aaron.ballman wrote:

LGTM! Should we update the public docs for this change as well? Specifically, I am wondering if we should update https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#mutexheader

Could you explain why we are doing this? Clang has its own static analyzer, which can find null dereferences as well but also tracks constraints to prevent false positives like this.

@aaron.ballman, this is a follow-up to D72635#1911495.

Another ping.

@rsmith, what do you think about this?