Patchwork [PING^2,PR46003,C++] Fix the assertion failure in build_target_expr

login
register
mail settings
Submitter Yufeng Zhang
Date March 2, 2011, 5:53 p.m.
Message ID <000001cbd902$c51c4f80$4f54ee80$@Zhang@arm.com>
Download mbox | patch
Permalink /patch/85118/
State New
Headers show

Comments

Yufeng Zhang - March 2, 2011, 5:53 p.m.
Thank you very much for the reply.

On 02/26/2011 05:38 PM, Jason Merrill wrote:
> The problem seems to be with the calls to build_cplus_new in
> convert_like_real, which seem to be redundant; we should have already
> called from build_cxx_call when we built up the call in the first place
> What happens if we just remove all those calls?

I tried to remove one of calls to build_cplus_new in convert_like_real,
the one just after the call to build_temp. For the following test code:

struct A
{
  A(int);
};

template<int> void foo(A x)
{ 0 ? x : 0; }

the compiler would complain (for both arm and x86 targets):

test.cpp: In function 'void foo(A)':
test.cpp:7:11: error: no match for ternary 'operator?:' in '(bool)0 ? x
: 0u->A::A(A(0))'


I looked into it a bit further. It seems that without the call to
build_cplus_new, a CALL_EXPR node rather than a TARGET_EXPR node is
returned to convert_like_real's caller build_conditional_expr, and
then this CALL_EXPR node is used as the converted 3rd operand for the
conditional expression. I don't think this is right, since the node is
just an overloaded constructor call.

The calls to build_cplus_new in convert_like_real don't seem to be
redundant. build_cxx_call won't be called, e.g. from build_over_call,
when processing_template_decl is non-zero. Furthermore, build_cxx_call
calls build_cplus_new only if MAYBE_CLASS_TYPE_P holds of the function
return type, but this is not the case in constructor calls.

Also the comment above build_special_member_call says:

   If NAME indicates a complete object constructor, INSTANCE may be
   NULL_TREE.  In this case, the caller will call build_cplus_new to
   store the newly constructed object into a VAR_DECL.

since build_temp calls build_special_member_call without calling
build_cplus_new, it seems reasonable to me that build_cplus_new should
be called after the call to build_temp in convert_like_real.

> We shouldn't be calling build_aggr_init_expr for such a CALL_EXPR.

Can you please provide a bit more details about the reason? I don't really
understand why, even after having spent some time further understanding
the related code.

The internal doc says: "An AGGR_INIT_EXPR represents the initialization
as the return value of a function call, or as the result of a
constructor. ..." It doesn't seem to breach the concept by calling
build_aggr_init_expr for a pre-overload-resolution CALL_EXPR.


Moreover, I'm thinking to further constrain the proposed change by also
considering processing_template_decl, since the problem only happens
when a template declaration is being processed. This should make the
patch more conservative and less possible in causing troubles.

Please let me know how you think about it. If you would like more analysis
or other experiments to be done, please do let me know as well.


Regards,
Yufeng


2011-03-02  Yufeng Zhang  <yufeng.zhang@arm.com>

        PR c++/46003
        * tree.c (build_aggr_init_expr): Also consider overloaded ctors
        when determining is_ctor.
Jason Merrill - March 3, 2011, 11:38 p.m.
On 03/02/2011 12:53 PM, Yufeng Zhang wrote:
> The calls to build_cplus_new in convert_like_real don't seem to be
> redundant.

Hmm, I guess you're right.  I think rather that the problem is in 
build_conditional_expr; it should have a template case that just 
determines the appropriate type and then builds up a COND_EXPR with that 
type from the unconverted operands, like we do for calls in 
finish_call_expr.

Jason
Yufeng Zhang - March 7, 2011, 6:27 p.m.
> On 03/03/2011 11:39 PM, Jason Merrill wrote: 
> I think rather that the problem is in build_conditional_expr; it
> should have a template case that just determines the appropriate
> type and then builds up a COND_EXPR with that type from the
> unconverted operands, like we do for calls in finish_call_expr.

Thanks for the suggestion. I spent some more time looking into the
related code. If I understand it correctly, I think this is just what
build_x_conditional_expr in ./cp/typeck.c does:

If the conditional expression is type-dependent in the template, it
builds and returns a COND_EXPR without TREE_TYPE.

If the conditional expression is not type-dependent, it calls
build_conditional_expr to determine its TREE_TYPE and then builds up
a COND_EXPR with the unconverted/original operands.

Different from CALL_EXPR, the TREE_TYPE of a COND_EXPR is determined
by the TREE_TYPEs of its latter two operands. When the types of the
two operands are different, a conversion is attempted on each operand
type to match the type of the other operand; while for a CALL_EXPR,
its TREE_TYPE is independent from the TREE_TYPE(s) of its arg(s).

So I think it is necessary for build_conditional_expr to call
convert_like_real to do the conversions. The parser should try its best
to determine the type and check for any semantic error for expressions
in a template declaration, especially for non-dependent expressions.

I still feel the proposed patch in
  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00076.html
makes sense. But if you think anything is not appropriate or have any
more question, please do let me know.


Regards,
Yufeng
Jason Merrill - March 8, 2011, 3:05 p.m.
On 03/07/2011 01:27 PM, Yufeng Zhang wrote:
>> On 03/03/2011 11:39 PM, Jason Merrill wrote:
>> I think rather that the problem is in build_conditional_expr; it
>> should have a template case that just determines the appropriate
>> type and then builds up a COND_EXPR with that type from the
>> unconverted operands, like we do for calls in finish_call_expr.
>
> Thanks for the suggestion. I spent some more time looking into the
> related code. If I understand it correctly, I think this is just what
> build_x_conditional_expr in ./cp/typeck.c does:

Yes.  The problem is that build_conditional_expr is actually trying to 
do the conversions; it should just determine what the conversions would 
be and then stop there.  That's what we do with calls--we determine all 
the desired conversions and pass them into build_over_call, which just 
ignores them.  Actually performing the conversions can wait until 
instantiation tme.

> Different from CALL_EXPR, the TREE_TYPE of a COND_EXPR is determined
> by the TREE_TYPEs of its latter two operands. When the types of the
> two operands are different, a conversion is attempted on each operand
> type to match the type of the other operand; while for a CALL_EXPR,
> its TREE_TYPE is independent from the TREE_TYPE(s) of its arg(s).

I don't think the difference is significant; a call to an overloaded 
function also depends on the types of its arguments.  We need to 
determine the conversions from argument to parameter types in order to 
select the right function, just as in a ?: expression we need to 
determine the conversions from the 2nd and 3rd operands to a result type.

Jason

Patch

diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 1a1f150..ae33e58 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -391,9 +391,12 @@  build_aggr_init_expr (tree type, tree init)
   else
     return convert (type, init);
 
-  is_ctor = (TREE_CODE (fn) == ADDR_EXPR
-	     && TREE_CODE (TREE_OPERAND (fn, 0)) == FUNCTION_DECL
-	     && DECL_CONSTRUCTOR_P (TREE_OPERAND (fn, 0)));
+  is_ctor = ((TREE_CODE (fn) == ADDR_EXPR
+	      && TREE_CODE (TREE_OPERAND (fn, 0)) == FUNCTION_DECL
+	      && DECL_CONSTRUCTOR_P (TREE_OPERAND (fn, 0)))
+	     || (processing_template_decl
+		 && is_overloaded_fn (fn)
+		 && DECL_CONSTRUCTOR_P (get_first_fn (fn))));
 
   /* We split the CALL_EXPR into its function and its arguments here.
      Then, in expand_expr, we put them back together.  The reason for