[PATCH] Fix for Bug 15290 - Segmentation fault during template instantiation

[Richard Smith]

On Fri, Feb 22, 2013 at 5:31 AM, Ismail Pazarbasi <
ismail.pazarbasi at gmail.com> wrote:

> On Fri, Feb 22, 2013 at 12:42 AM, Ismail Pazarbasi
> <ismail.pazarbasi at gmail.com> wrote:
> > On Fri, Feb 22, 2013 at 12:18 AM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> >> On Thu, Feb 21, 2013 at 3:11 PM, Ismail Pazarbasi
> >> <ismail.pazarbasi at gmail.com> wrote:
> >>>
> >>> On Thu, Feb 21, 2013 at 11:34 PM, Richard Smith <richard at metafoo.co.uk
> >
> >>> wrote:
> >>> > Hi!
> >>> >
> >>> > On Thu, Feb 21, 2013 at 1:53 PM, Ismail Pazarbasi
> >>> > <ismail.pazarbasi at gmail.com> wrote:
> >>> >>
> >>> >> Hi,
> >>> >>
> >>> >> when operand of a noexcept operator in a friend function definition
> >>> >> involves a member of a template class enclosing the friend
> definition,
> >>> >> Clang casts current context to either a CXXMethodDecl or a
> >>> >> CXXRecordDecl. Since friend function is neither of these, cast
> fails.
> >>> >> The same test in a non-template class works fine.
> >>> >>
> >>> >> The first patch tries to retrieve the lexical parent of the function
> >>> >> definition (per class.friend/p6-7), which is expected to be a record
> >>> >> type. A test is also provided (also, pasting below for reference).
> >>> >>
> >>> >> template<typename T>
> >>> >> class noexcept_member_operand_at_friend_definition {
> >>> >>   T v_;
> >>> >>   friend int add_to_v(noexcept_member_operand_at_friend_definition
> &t)
> >>> >> noexcept(noexcept(v_ + 42))
> >>> >>   {
> >>> >>     return t.v_ + 42;
> >>> >>   }
> >>> >> };
> >>> >>
> >>> >> void this_expr()
> >>> >> {
> >>> >>   noexcept_member_operand_at_friend_definition<int> t;
> >>> >>   add_to_v(t);
> >>> >> }
> >>> >
> >>> >
> >>> > Thanks for looking into this. There is a more fundamental problem
> here,
> >>> > which this patch is masking rather than fixing. Per 5.1.1/3, 'this'
> is
> >>> > not
> >>> > permitted to appear in the declaration of a friend function, so per
> >>> > 9.3.1/3,
> >>> > we should not transform the mention of 'v_' into a class member
> access.
> >>> > Therefore, we should build a DeclRefExpr for the member, not a
> >>> > CXXMemberExpr, in this case. It looks like the bug is that
> >>> > Parser::ParseFunctionDeclarator's IsCXX11MemberFunction is not taking
> >>> > 'friend' into account when determining whether a function in a class
> is
> >>> > a
> >>> > member function.
> >>> >
> >>> > To demonstrate the difference, here's a testcase which we should
> reject,
> >>> > but
> >>> > we currently accept (and still accept with your patch):
> >>> >
> >>> > struct S {
> >>> >   friend auto f() -> decltype(this);
> >>> > };
> >>> Thank you for reviewing these.
> >>>
> >>> I first thought we should have rejected this and the case with a
> >>> non-template class as well. But both GCC and Clang accepted, and I
> >>> honestly started to look what am I missing. I found class.friend/p6-7,
> >>> which has nothing to do with 'this' pointer, but only lexical scope;
> >>> so I thought name lookup can find identifier in the class' scope, and
> >>> because 'this' is not used inside the function definition itself, it
> >>> might be legal to use 'this' inside noexcept operator (which is
> >>> clearly not permitted by 5.1.1/3).
> >>
> >>
> >> Your example is valid. While you can't use 'this' in a friend
> declaration,
> >> you *can* use 'v_', because noexcept is an unevaluated context; see
> >> 5.1.1/13.
> >
> > OK, thank you very much, I think I understood it. I will check what
> > can I do about IsCXX11MemberFunction function first.
> >
> >>>
> >>> >
> >>> >> The second patch is a minor one; it merges two if statements that
> have
> >>> >> the same condition.
> >>> >
> >>> >
> >>> > I don't think this patch is correct. Note that the body of the first
> >>> > 'if'
> >>> > statement can cause the condition of the second to no longer be
> 'true'.
> >>> Yes, you are right!
> >>> _______________________________________________
> >>> cfe-commits mailing list
> >>> cfe-commits at cs.uiuc.edu
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >>
> >>
>
> Hi again,
>
> As you said, IsCXX11MemberFunction was not taking 'friend' specifier
> into account. I have added the check, and expression is not
> transformed to a class member access expression.


Great! Two things:

1) It would be a bit clearer to only check isFriendSpecified in the
MemberContext case.
2) Please include some tests (the test from your original testcase plus the
test from my reply would be fine).

Then I'll check this fix in for you. Thanks!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130222/3a7467ae/attachment.html>