<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 22 September 2017 at 18:00, Volodymyr Sapsai via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space"><br><div><div><div class="h5"><br><blockquote type="cite"><div>On Sep 21, 2017, at 15:17, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div><br class="m_-443097055292461227Apple-interchange-newline"><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_extra"><div class="gmail_quote">On 15 September 2017 at 12:51, Volodymyr Sapsai via cfe-commits<span class="m_-443097055292461227Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@<wbr>lists.llvm.org</a>></span><span class="m_-443097055292461227Apple-converted-space"> </span>wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Author: vsapsai<br>Date: Fri Sep 15 12:51:42 2017<br>New Revision: 313386<br><br>URL:<span class="m_-443097055292461227Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project?rev=313386&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/<wbr>llvm-project?rev=313386&view=<wbr>rev</a><br>Log:<br>[Sema] Error out early for tags defined inside an enumeration.<br><br>This fixes PR28903 by avoiding access check for inner enum constant. We<br>are performing access check because one enum constant references another<br>and because enum is defined in CXXRecordDecl. But access check doesn't<br>work because FindDeclaringClass doesn't expect more than one EnumDecl<br>and because inner enum has access AS_none due to not being an immediate<br>child of a record.<br><br>The change detects an enum is defined in wrong place and allows to skip<br>parsing its body. Access check is skipped together with body parsing.<br>There was no crash in C, added test case to cover the new error.<br><br><a>rdar://problem/28530809</a><br><br>Reviewers: rnk, doug.gregor, rsmith<br><br>Reviewed By: doug.gregor<br><br>Subscribers: cfe-commits<br><br>Differential Revision:<span class="m_-443097055292461227Apple-converted-space"> </span><a href="https://reviews.llvm.org/D37089" rel="noreferrer" target="_blank">https://reviews.<wbr>llvm.org/D37089</a><br><br><br>Modified:<br> <span class="m_-443097055292461227Apple-converted-space"> </span>cfe/trunk/include/clang/<wbr>Basic/DiagnosticSemaKinds.td<br> <span class="m_-443097055292461227Apple-converted-space"> </span>cfe/trunk/lib/Sema/SemaDecl.<wbr>cpp<br> <span class="m_-443097055292461227Apple-converted-space"> </span>cfe/trunk/test/Sema/enum.c<br> <span class="m_-443097055292461227Apple-converted-space"> </span>cfe/trunk/test/SemaCXX/enum.<wbr>cpp<br><br>Modified: cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td<br>URL:<span class="m_-443097055292461227Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=313386&r1=313385&r2=313386&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/<wbr>llvm-project/cfe/trunk/<wbr>include/clang/Basic/Diagnostic<wbr>SemaKinds.td?rev=313386&r1=<wbr>313385&r2=313386&view=diff</a><br>==============================<wbr>==============================<wbr>==================<br>--- cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td (original)<br>+++ cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td Fri Sep 15 12:51:42 2017<br>@@ -1335,6 +1335,8 @@ def err_type_defined_in_alias_temp<wbr>late :<br> "%0 cannot be defined in a type alias template">;<br> def err_type_defined_in_condition : Error<<br> "%0 cannot be defined in a condition">;<br>+def err_type_defined_in_enum : Error<<br>+ "%0 cannot be defined in an enumeration">;<br><br> def note_pure_virtual_function : Note<<br> "unimplemented pure virtual method %0 in %1">;<br><br>Modified: cfe/trunk/lib/Sema/SemaDecl.cp<wbr>p<br>URL:<span class="m_-443097055292461227Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=313386&r1=313385&r2=313386&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/<wbr>llvm-project/cfe/trunk/lib/<wbr>Sema/SemaDecl.cpp?rev=313386&<wbr>r1=313385&r2=313386&view=diff</a><br>==============================<wbr>==============================<wbr>==================<br>--- cfe/trunk/lib/Sema/SemaDecl.cp<wbr>p (original)<br>+++ cfe/trunk/lib/Sema/SemaDecl.cp<wbr>p Fri Sep 15 12:51:42 2017<br>@@ -13928,6 +13928,12 @@ CreateNewDecl:<br> Invalid = true;<br> }<br><br>+ if (!Invalid && TUK == TUK_Definition && DC->getDeclKind() == Decl::Enum) {<br>+ Diag(New->getLocation(), diag::err_type_defined_in_enum<wbr>)<br>+ << Context.getTagDeclType(New);<br>+ Invalid = true;<br>+ }<br></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div></div></div></div></div></blockquote><br></div></div><div><div>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::<wbr>ParseSpecifierQualifierList. Which is called from</div><div><br></div><div>#4<span class="m_-443097055292461227Apple-tab-span" style="white-space:pre-wrap"> </span>clang::Parser::<wbr>ParseParenExpression(clang::<wbr>Parser::ParenParseOption&, bool, bool, clang::OpaquePtr<clang::<wbr>QualType>&, clang::SourceLocation&) at llvm-project/clang/lib/Parse/<wbr>ParseExpr.cpp:2375</div></div></div></div></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space"><div><div><div>#5<span class="m_-443097055292461227Apple-tab-span" style="white-space:pre-wrap"> </span>clang::Parser::<wbr>ParseCastExpression(bool, bool, bool&, clang::Parser::TypeCastState, bool) at llvm-project/clang/lib/Parse/<wbr>ParseExpr.cpp:768</div><div>#6<span class="m_-443097055292461227Apple-tab-span" style="white-space:pre-wrap"> </span>clang::Parser::<wbr>ParseCastExpression(bool, bool, clang::Parser::TypeCastState, bool) at llvm-project/clang/lib/Parse/<wbr>ParseExpr.cpp:521</div><div>#7<span class="m_-443097055292461227Apple-tab-span" style="white-space:pre-wrap"> </span>clang::Parser::<wbr>ParseConstantExpressionInExprE<wbr>valContext(clang::Parser::<wbr>TypeCastState) at llvm-project/clang/lib/Parse/<wbr>ParseExpr.cpp:201</div><div><br></div><div>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.</div><div><br></div><div>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.</div></div><div><div class="h5"><br><blockquote type="cite"><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">+<br> // Maybe add qualifier info.<br> if (SS.isNotEmpty()) {<br> if (SS.isSet()) {<br><br>Modified: cfe/trunk/test/Sema/enum.c<br>URL:<span class="m_-443097055292461227Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/enum.c?rev=313386&r1=313385&r2=313386&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/<wbr>llvm-project/cfe/trunk/test/<wbr>Sema/enum.c?rev=313386&r1=<wbr>313385&r2=313386&view=diff</a><br>==============================<wbr>==============================<wbr>==================<br>--- cfe/trunk/test/Sema/enum.c (original)<br>+++ cfe/trunk/test/Sema/enum.c Fri Sep 15 12:51:42 2017<br>@@ -123,3 +123,14 @@ int NegativeShortTest[NegativeShor<wbr>t == -<br> // PR24610<br> enum Color { Red, Green, Blue }; // expected-note{{previous use is here}}<br> typedef struct Color NewColor; // expected-error {{use of 'Color' with tag type that does not match previous declaration}}<br>+<br>+// PR28903<br>+struct PR28903 {<br>+ enum {<br>+ PR28903_A = (enum { // expected-error-re {{'enum PR28903::(anonymous at {{.*}})' cannot be defined in an enumeration}}<br>+ PR28903_B,<br>+ PR28903_C = PR28903_B<br>+ })0<br>+ };<br>+ int makeStructNonEmpty;<br>+};<br><br>Modified: cfe/trunk/test/SemaCXX/enum.cp<wbr>p<br>URL:<span class="m_-443097055292461227Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/enum.cpp?rev=313386&r1=313385&r2=313386&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/<wbr>llvm-project/cfe/trunk/test/Se<wbr>maCXX/enum.cpp?rev=313386&r1=<wbr>313385&r2=313386&view=diff</a><br>==============================<wbr>==============================<wbr>==================<br>--- cfe/trunk/test/SemaCXX/enum.cp<wbr>p (original)<br>+++ cfe/trunk/test/SemaCXX/enum.cp<wbr>p Fri Sep 15 12:51:42 2017<br>@@ -110,3 +110,13 @@ enum { overflow = 123456 * 234567 };<br> // expected-warning@-2 {{not an integral constant expression}}<br> // expected-note@-3 {{value 28958703552 is outside the range of representable values}}<br> #endif<br>+<br>+// PR28903<br>+struct PR28903 {<br>+ enum {<br>+ PR28903_A = (enum { // expected-error-re {{'PR28903::(anonymous enum at {{.*}})' cannot be defined in an enumeration}}<br>+ PR28903_B,<br>+ PR28903_C = PR28903_B<br>+ })<br>+ };<br>+};<br><br><br>______________________________<wbr>_________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a></blockquote></div></div></div></div></blockquote></div></div></div><br></div><br>______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>