r313386 - [Sema] Error out early for tags defined inside an enumeration.

Volodymyr Sapsai via cfe-commits cfe-commits at lists.llvm.org
Fri Sep 22 18:00:47 PDT 2017



> On Sep 21, 2017, at 15:17, Richard Smith <richard at metafoo.co.uk> wrote:
> 
> On 15 September 2017 at 12:51, Volodymyr Sapsai via cfe-commits <cfe-commits at lists.llvm.org <mailto:cfe-commits at 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 at -2 {{not an integral constant expression}}
>  // expected-note at -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 at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170922/6f640c01/attachment.html>


More information about the cfe-commits mailing list