Page MenuHomePhabricator

Add Attribute NoThrow as an Exception Specifier Type
ClosedPublic

Authored by erichkeane on May 24 2019, 4:13 PM.

Details

Summary

In response to https://bugs.llvm.org/show_bug.cgi?id=33235, it became
clear that the current mechanism of hacking through checks for the
exception specification of a function gets confused really quickly when
there are alternate exception specifiers.

This patch introduces EST_NoThrow, which is the equivilent of
EST_noexcept when caused by EST_noThrow. The existing implementation is
left in place to cover functions with no FunctionProtoType.

Diff Detail

Repository
rL LLVM

Event Timeline

erichkeane created this revision.May 24 2019, 4:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2019, 4:13 PM
rsmith requested changes to this revision.May 24 2019, 4:21 PM
rsmith added a subscriber: rsmith.

Seems like a nice approach to the problem, but we need to not break the libclang C ABI :)

clang/include/clang-c/Index.h
202–204 ↗(On Diff #201358)

This would renumber the later enumerators, resulting in an ABI break for our stable C ABI.

clang/lib/Sema/SemaExprCXX.cpp
6067–6068 ↗(On Diff #201358)

I think this should be checked earlier, in the spirit of allowing "real" syntax to be preferred to attributes. (Eg, given a declaration that's __declspec(nothrow) and one that's noexcept, we should keep the noexcept in the merged type.)

This revision now requires changes to proceed.May 24 2019, 4:21 PM
erichkeane marked 2 inline comments as done.May 25 2019, 6:46 AM
erichkeane added inline comments.
clang/include/clang-c/Index.h
202–204 ↗(On Diff #201358)

Is it alright to just add it to the end then? Or do I need to translate it into one of the others?

clang/lib/Sema/SemaExprCXX.cpp
6067–6068 ↗(On Diff #201358)

Ah, right, good point.

Looking above, I'm torn between above or below MSAny. Do you have a thought?

erichkeane marked 2 inline comments as done.

Reread @rsmith's comments and surrounding code and found what I believe is the correct answer to these comments :)

aaron.ballman added inline comments.May 27 2019, 7:58 AM
clang/include/clang-c/Index.h
227 ↗(On Diff #201404)

__declspec(nothrow)

clang/include/clang/Basic/ExceptionSpecificationType.h
25 ↗(On Diff #201404)

__declspec(nothrow)

clang/lib/Sema/SemaType.cpp
6968 ↗(On Diff #201404)

Ignores -> ignores

6970 ↗(On Diff #201404)

I think we should diagnose that the nothrow is ignored in this case to let users know they've done something in conflict and what wins.

erichkeane marked an inline comment as done.May 27 2019, 9:48 AM
erichkeane added inline comments.
clang/lib/Sema/SemaType.cpp
6970 ↗(On Diff #201404)

I've been considering that over the weekend, and agree it is a good idea. I am on the fence about the when however. For example, should we warn with throw() or noexcept or noexcept(true)? In these cases, ignoring the attribute is the same as enforcing it.

My concern is that noexcept (expression-evaluating-to-true) gets ignored with a diagnostic, but has the same effect.

Did you have an opinion here?

aaron.ballman added inline comments.May 27 2019, 10:45 AM
clang/lib/Sema/SemaType.cpp
6970 ↗(On Diff #201404)

I think the metric should be: if there are conflicting attributes where we need to ignore one of them, we should always diagnose when the attributes specify conflicting semantics, but we don't have to always diagnose when the attributes specify the same semantics. In this case, e.g.,

__declspec(nothrow) void foo1() noexcept; // Don't bother warning
__declspec(nothrow) void foo2() noexcept(true); // Don't bother warning
__declspec(nothrow) void foo3() noexcept(false); // Warn
__declspec(nothrow) void foo4() noexcept(foo1()); // Warning is debatable
__declspec(nothrow) void foo5() noexcept(foo3()); // Warn
erichkeane marked 5 inline comments as done.

Added warning + other comments from @aaron.ballman

The exception state was further along than I expected, so I was able to make the warning better than I thought!

rsmith accepted this revision.May 28 2019, 4:28 PM

Seems fine to me; please wait for @aaron.ballman's review to conclude as well.

This revision is now accepted and ready to land.May 28 2019, 4:28 PM
aaron.ballman accepted this revision.May 30 2019, 9:14 AM

LGTM aside from some minor nits.

clang/include/clang/Basic/DiagnosticSemaKinds.td
2789 ↗(On Diff #201673)

Add single quotes around the attribute name: 'nothrow' attribute conflicts...

clang/include/clang/Sema/DeclSpec.h
1547 ↗(On Diff #201673)

Not that I dislike this, but is this function being used? It seems to be the only hasAttr in the review.

clang/lib/Sema/SemaType.cpp
6968 ↗(On Diff #201673)

How about: MSVC ignores nothrow if it is in conflict with an explicit exception specification.

6971 ↗(On Diff #201673)

Will this need a fallthrough attribute because of the statement between the labels?

erichkeane marked 6 inline comments as done.May 30 2019, 10:06 AM
erichkeane added inline comments.
clang/include/clang/Sema/DeclSpec.h
1547 ↗(On Diff #201673)

Woops, likely a leftover from a previous iteration, removing it!

clang/lib/Sema/SemaType.cpp
6971 ↗(On Diff #201673)

Ah, I think it depends on what llvm_unreachable ends up expanding to (actually, LLVM_ATTRIBUTE_NORETURN).

Basically, here: https://llvm.org/doxygen/Support_2ErrorHandling_8h_source.html

I'm not sure of what compilers has that expand to nothing don't support it (would need !def GNUC and !def _MSC_VER), but an LLVM_FALLTHROUGH is easy enough to add, thanks!

This revision was automatically updated to reflect the committed changes.
erichkeane marked 2 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 11:13 AM

This change broke compiling Qt on MinGW, see https://bugs.llvm.org/show_bug.cgi?id=42089. Trivially reproducible by trying to compile a snippet that looks like this:

class Foo {
public:
        __attribute__((nothrow)) void __attribute__((__stdcall__)) Bar();
};

void Foo::Bar() {
}

Reproducible by compiling the same snippet for a windows-msvc target as well.

This change broke compiling Qt on MinGW, see https://bugs.llvm.org/show_bug.cgi?id=42089. Trivially reproducible by trying to compile a snippet that looks like this:

class Foo {
public:
        __attribute__((nothrow)) void __attribute__((__stdcall__)) Bar();
};

void Foo::Bar() {
}

Reproducible by compiling the same snippet for a windows-msvc target as well.

Yikes, thanks for the report! I'll look at it this morning.

This change had another breaking effect as well, for the MinGW target. Code that implements a COM interface easily ends up overriding a __declspec(nothrow) function with a method that in most cases (at least in cases that follow the microsoft sample code) lacks the same attribute. For code with -fms-extensions, this is a warning (as all exception spec mismatches are non-fatal there), but MinGW code doesn't necessarily use this option. See https://bugs.llvm.org/show_bug.cgi?id=42100 for full issue report and discussion.