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

Richard Smith via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 11 16:02:37 PDT 2017


On 22 September 2017 at 18:00, Volodymyr Sapsai via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

>
>
> 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> 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 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
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> 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/20171011/d4ff8dfd/attachment-0001.html>


More information about the cfe-commits mailing list