r226306 - Don't crash if a declarator in a friend decl doesn't have a name.

[Nico Weber]

On Fri, Jan 16, 2015 at 5:55 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> On Fri, Jan 16, 2015 at 11:34 AM, Nico Weber <nicolasweber at gmx.de> wrote:
> > Author: nico
> > Date: Fri Jan 16 13:34:13 2015
> > New Revision: 226306
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=226306&view=rev
> > Log:
> > Don't crash if a declarator in a friend decl doesn't have a name.
> >
> > There was already an explicit check for that for the first decl.  Move
> that
> > to a different place so that it's called for the following decls too.
> Also
> > don't randomly set the BitfieldSize ExprResult to true (this sets a
> pointer to
> > true internally).
> >
> > Found by SLi's bot.
> >
> > Modified:
> >     cfe/trunk/include/clang/Parse/Parser.h
> >     cfe/trunk/lib/Parse/ParseDeclCXX.cpp
> >     cfe/trunk/test/CXX/class/class.friend/p1.cpp
> >
> > Modified: cfe/trunk/include/clang/Parse/Parser.h
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=226306&r1=226305&r2=226306&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/include/clang/Parse/Parser.h (original)
> > +++ cfe/trunk/include/clang/Parse/Parser.h Fri Jan 16 13:34:13 2015
> > @@ -2294,7 +2294,7 @@ private:
> >                                     Decl *TagDecl);
> >    ExprResult ParseCXXMemberInitializer(Decl *D, bool IsFunction,
> >                                         SourceLocation &EqualLoc);
> > -  void ParseCXXMemberDeclaratorBeforeInitializer(Declarator
> &DeclaratorInfo,
> > +  bool ParseCXXMemberDeclaratorBeforeInitializer(Declarator
> &DeclaratorInfo,
> >                                                   VirtSpecifiers &VS,
> >                                                   ExprResult
> &BitfieldSize,
> >                                                   LateParsedAttrList
> &LateAttrs);
> >
> > Modified: cfe/trunk/lib/Parse/ParseDeclCXX.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDeclCXX.cpp?rev=226306&r1=226305&r2=226306&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/lib/Parse/ParseDeclCXX.cpp (original)
> > +++ cfe/trunk/lib/Parse/ParseDeclCXX.cpp Fri Jan 16 13:34:13 2015
> > @@ -2019,7 +2019,7 @@ bool Parser::isCXX11FinalKeyword() const
> >
> >  /// \brief Parse a C++ member-declarator up to, but not including, the
> optional
> >  /// brace-or-equal-initializer or pure-specifier.
> > -void Parser::ParseCXXMemberDeclaratorBeforeInitializer(
> > +bool Parser::ParseCXXMemberDeclaratorBeforeInitializer(
> >      Declarator &DeclaratorInfo, VirtSpecifiers &VS, ExprResult
> &BitfieldSize,
> >      LateParsedAttrList &LateParsedAttrs) {
> >    // member-declarator:
> > @@ -2073,6 +2073,15 @@ void Parser::ParseCXXMemberDeclaratorBef
> >        }
> >      }
> >    }
> > +
> > +  // If this has neither a name nor a bit width, something has gone
> seriously
> > +  // wrong. Skip until the semi-colon or }.
> > +  if (!DeclaratorInfo.hasName() && BitfieldSize.isUnset()) {
> > +    // If so, skip until the semi-colon or a }.
> > +    SkipUntil(tok::r_brace, StopAtSemi | StopBeforeMatch);
> > +    return true;
> > +  }
> > +  return false;
> >  }
> >
> >  /// ParseCXXClassMemberDeclaration - Parse a C++ class member
> declaration.
> > @@ -2298,14 +2307,8 @@ void Parser::ParseCXXClassMemberDeclarat
> >    bool ExpectSemi = true;
> >
> >    // Parse the first declarator.
> > -  ParseCXXMemberDeclaratorBeforeInitializer(DeclaratorInfo, VS,
> BitfieldSize,
> > -                                            LateParsedAttrs);
> > -
> > -  // If this has neither a name nor a bit width, something has gone
> seriously
> > -  // wrong. Skip until the semi-colon or }.
> > -  if (!DeclaratorInfo.hasName() && BitfieldSize.isUnset()) {
> > -    // If so, skip until the semi-colon or a }.
> > -    SkipUntil(tok::r_brace, StopAtSemi | StopBeforeMatch);
> > +  if (ParseCXXMemberDeclaratorBeforeInitializer(
> > +          DeclaratorInfo, VS, BitfieldSize, LateParsedAttrs)) {
> >      TryConsumeToken(tok::semi);
> >      return;
> >    }
> > @@ -2530,7 +2533,7 @@ void Parser::ParseCXXClassMemberDeclarat
> >      // Parse the next declarator.
> >      DeclaratorInfo.clear();
> >      VS.clear();
> > -    BitfieldSize = true;
>
> This was previously setting BitfieldSize to ExprError() (that is, null
> and invalid) and now sets it to ExprResult() (that is, null but
> valid). Was that your intention?
>

Yes: The if above checks if (!DeclaratorInfo.hasName() &&
BitfieldSize.isUnset()), and isUnset() is implemented as `return !Invalid
&& !Val;`. Without this change, the error path wasn't taken.


>
> > +    BitfieldSize = nullptr;
> >      Init = true;
>
> If you don't like the implicit conversion from bool to ExprResult (and
> I don't blame you...) please fix this one too for local consistency.
>

I added an explicit constructor call and flipped this from true to false
(from what I can tell, this shouldn't make a behavior difference for this
one) in r226363.

Thanks!


>
> >      HasInitializer = false;
> >      DeclaratorInfo.setCommaLoc(CommaLoc);
> > @@ -2538,8 +2541,9 @@ void Parser::ParseCXXClassMemberDeclarat
> >      // GNU attributes are allowed before the second and subsequent
> declarator.
> >      MaybeParseGNUAttributes(DeclaratorInfo);
> >
> > -    ParseCXXMemberDeclaratorBeforeInitializer(DeclaratorInfo, VS,
> BitfieldSize,
> > -                                              LateParsedAttrs);
> > +    if (ParseCXXMemberDeclaratorBeforeInitializer(
> > +            DeclaratorInfo, VS, BitfieldSize, LateParsedAttrs))
> > +      break;
> >    }
> >
> >    if (ExpectSemi &&
> >
> > Modified: cfe/trunk/test/CXX/class/class.friend/p1.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/class/class.friend/p1.cpp?rev=226306&r1=226305&r2=226306&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/test/CXX/class/class.friend/p1.cpp (original)
> > +++ cfe/trunk/test/CXX/class/class.friend/p1.cpp Fri Jan 16 13:34:13 2015
> > @@ -66,6 +66,10 @@ class A {
> >    class facet;
> >    friend class facet;  // should not assert
> >    class facet {};
> > +
> > +  friend int Unknown::thing(); // expected-error {{use of undeclared
> identifier}}
> > +  friend int friendfunc(), Unknown::thing(); // expected-error {{use of
> undeclared identifier}}
> > +  friend int friendfunc(), Unknown::thing() : 4; // expected-error
> {{use of undeclared identifier}}
> >  };
> >
> >  A::UndeclaredSoFar y; // expected-error {{no type named
> 'UndeclaredSoFar' in 'A'}}
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150116/be9e41e3/attachment.html>