> On Sep 21, 2017, at 15:17, Richard Smith <rich...@metafoo.co.uk> wrote:
>
> On 15 September 2017 at 12:51, Volodymyr Sapsai via cfe-commits
> <cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>> wrote:
> Author: vsapsai
> Date: Fri Sep 15 12:51:42 2017
> New Revision: 313386
>
> URL: http://llvm.org/viewvc/llvm-project?rev=313386&view=rev
> <http://llvm.org/viewvc/llvm-project?rev=313386&view=rev>
> Log:
> [Sema] Error out early for tags defined inside an enumeration.
>
> This fixes PR28903 by avoiding access check for inner enum constant. We
> are performing access check because one enum constant references another
> and because enum is defined in CXXRecordDecl. But access check doesn't
> work because FindDeclaringClass doesn't expect more than one EnumDecl
> and because inner enum has access AS_none due to not being an immediate
> child of a record.
>
> The change detects an enum is defined in wrong place and allows to skip
> parsing its body. Access check is skipped together with body parsing.
> There was no crash in C, added test case to cover the new error.
>
> rdar://problem/28530809
>
> Reviewers: rnk, doug.gregor, rsmith
>
> Reviewed By: doug.gregor
>
> Subscribers: cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D37089
> <https://reviews.llvm.org/D37089>
>
>
> Modified:
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> cfe/trunk/lib/Sema/SemaDecl.cpp
> cfe/trunk/test/Sema/enum.c
> cfe/trunk/test/SemaCXX/enum.cpp
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=313386&r1=313385&r2=313386&view=diff
>
> <http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=313386&r1=313385&r2=313386&view=diff>
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Sep 15 12:51:42
> 2017
> @@ -1335,6 +1335,8 @@ def err_type_defined_in_alias_template :
> "%0 cannot be defined in a type alias template">;
> def err_type_defined_in_condition : Error<
> "%0 cannot be defined in a condition">;
> +def err_type_defined_in_enum : Error<
> + "%0 cannot be defined in an enumeration">;
>
> def note_pure_virtual_function : Note<
> "unimplemented pure virtual method %0 in %1">;
>
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=313386&r1=313385&r2=313386&view=diff
>
> <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=313386&r1=313385&r2=313386&view=diff>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Sep 15 12:51:42 2017
> @@ -13928,6 +13928,12 @@ CreateNewDecl:
> Invalid = true;
> }
>
> + if (!Invalid && TUK == TUK_Definition && DC->getDeclKind() == Decl::Enum) {
> + Diag(New->getLocation(), diag::err_type_defined_in_enum)
> + << Context.getTagDeclType(New);
> + Invalid = true;
> + }
>
> This looks like the wrong fix. As noted elsewhere, this is wrong in C. And in
> C++, the relevant context is a type-specifier, which should be rejected due
> to the check 7 lines above.
>
> It looks like the actual bug is that we don't consider the type within a C99
> compound literal to be a type-specifier. The fact that the context is an
> enumeration is irrelevant.
At which point can we detect IsTypeSpecifier should be true? Which in turn
boils down to DeclSpecContext should be DSC_type_specifier. Currently we have
DeclSpecContext DSC_normal because it is a default argument in
Parser::ParseSpecifierQualifierList. Which is called from
#4 clang::Parser::ParseParenExpression(clang::Parser::ParenParseOption&,
bool, bool, clang::OpaquePtr<clang::QualType>&, clang::SourceLocation&) at
llvm-project/clang/lib/Parse/ParseExpr.cpp:2375
#5 clang::Parser::ParseCastExpression(bool, bool, bool&,
clang::Parser::TypeCastState, bool) at
llvm-project/clang/lib/Parse/ParseExpr.cpp:768
#6 clang::Parser::ParseCastExpression(bool, bool,
clang::Parser::TypeCastState, bool) at
llvm-project/clang/lib/Parse/ParseExpr.cpp:521
#7
clang::Parser::ParseConstantExpressionInExprEvalContext(clang::Parser::TypeCastState)
at llvm-project/clang/lib/Parse/ParseExpr.cpp:201
I have considered using TypeCastState for setting DeclSpecContext but its value
is NotTypeCast because Parser::ParseEnumBody calls ParseConstantExpression with
default argument. And it looks correct as parsing enum body doesn't imply
presence of a type cast.
I was struggling to find a good indication we are parsing type specifier and
the best option seems to be ParseCastExpression because it expects a type. But
it is too broad and likely to cause false positives. In quick prototype it
didn't work so I didn't pursue it further. Do you think it is possible to tell
we are in type specifier based on tokens we parsed? Specifically "(" seems to
be significant as without it parsing goes in different direction.
>
> +
> // Maybe add qualifier info.
> if (SS.isNotEmpty()) {
> if (SS.isSet()) {
>
> Modified: cfe/trunk/test/Sema/enum.c
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/enum.c?rev=313386&r1=313385&r2=313386&view=diff
>
> <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/enum.c?rev=313386&r1=313385&r2=313386&view=diff>
> ==============================================================================
> --- cfe/trunk/test/Sema/enum.c (original)
> +++ cfe/trunk/test/Sema/enum.c Fri Sep 15 12:51:42 2017
> @@ -123,3 +123,14 @@ int NegativeShortTest[NegativeShort == -
> // PR24610
> enum Color { Red, Green, Blue }; // expected-note{{previous use is here}}
> typedef struct Color NewColor; // expected-error {{use of 'Color' with tag
> type that does not match previous declaration}}
> +
> +// PR28903
> +struct PR28903 {
> + enum {
> + PR28903_A = (enum { // expected-error-re {{'enum PR28903::(anonymous at
> {{.*}})' cannot be defined in an enumeration}}
> + PR28903_B,
> + PR28903_C = PR28903_B
> + })0
> + };
> + int makeStructNonEmpty;
> +};
>
> Modified: cfe/trunk/test/SemaCXX/enum.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/enum.cpp?rev=313386&r1=313385&r2=313386&view=diff
>
> <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/enum.cpp?rev=313386&r1=313385&r2=313386&view=diff>
> ==============================================================================
> --- cfe/trunk/test/SemaCXX/enum.cpp (original)
> +++ cfe/trunk/test/SemaCXX/enum.cpp Fri Sep 15 12:51:42 2017
> @@ -110,3 +110,13 @@ enum { overflow = 123456 * 234567 };
> // expected-warning@-2 {{not an integral constant expression}}
> // expected-note@-3 {{value 28958703552 is outside the range of
> representable values}}
> #endif
> +
> +// PR28903
> +struct PR28903 {
> + enum {
> + PR28903_A = (enum { // expected-error-re {{'PR28903::(anonymous enum at
> {{.*}})' cannot be defined in an enumeration}}
> + PR28903_B,
> + PR28903_C = PR28903_B
> + })
> + };
> +};
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
