On 22 September 2017 at 18:00, Volodymyr Sapsai via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> > > 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> 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 >> 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 >> >> >> 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 >> ============================================================ >> ================== >> --- 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 >> ============================================================ >> ================== >> --- 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 > The call to ParseSpecifierQualfiierList here should always pass DSC_type_specifier. We're parsing a type within parentheses (which we've already disambiguated as being a type cast / compound literal rather than an expression), which is the DSC_type_specifier case. > #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 >> ============================================================ >> ================== >> --- 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/Se >> maCXX/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 >> 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 > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
