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

Hi,

This patch fixes g++.dg/template/cond5.C failure with arm-eabi; it's a
fix in the C++ frontend.

http://gcc.gnu.org/ml/gcc-patches/2011-02/msg00689.html
http://gcc.gnu.org/ml/gcc-patches/2011-02/msg01168.html

Any comment will be appreciated.

Thanks,
Yufeng

-----Original Message-----

The test ./g++.dg/template/cond5.C fails for testing with arm-eabi with
an assertion failure, which occurs in cp/tree.c:build_target_expr(). 
(Please see the bugzilla page for more information on the analysis:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46003)

Note that the assertion does not fail in x86-targeted (or any of many
other targets targeted) compiler, because of the difference in the ARM C
++ ABI on constructor return values. The ARM C++ ABI requires C1 and C2
constructors to return this (instead of being void functions).

The patch tries to patch cp/tree.c:build_aggr_init_expr().
build_aggr_init_expr() wraps a ctor-calling CALL_EXPR tree node with an
AGGR_INIT_EXPR tree node, whose expression type is always VOID_TYPE. When
it determines whether the routine being called is a ctor or not, it only
looks into simple cases, but not the case of overloaded ctors in a tree
of COMPONENT_REF (INDIRECT_REF (something, BASELINK (OVERLOAD))). This
patch expands the is_ctor determining logic.
          
The test with arm-eabi before and after the patch shows that the
previously failing test (cond5.C) now passes, and all the other tests
are unaffected. There is no change in the x86 test result.

OK for the trunk?


Thanks,
Yufeng


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

        PR c++/46003
        * tree.c (build_aggr_init_expr): Also consider overloaded ctors
        when determining is_ctor.

On 2/21/2011 12:56 PM, Yufeng Zhang wrote:

> http://gcc.gnu.org/ml/gcc-patches/2011-02/msg00689.html

> Any comment will be appreciated.

I would think that when we call the constructor on ARM we should cast
the result to "void".  Then we would presumably not have to change
build_aggr_init_expr.  Does that work?

On 02/21/2011 03:56 PM, Yufeng Zhang wrote:
> The patch tries to patch cp/tree.c:build_aggr_init_expr().
> build_aggr_init_expr() wraps a ctor-calling CALL_EXPR tree node with an
> AGGR_INIT_EXPR tree node, whose expression type is always VOID_TYPE. When
> it determines whether the routine being called is a ctor or not, it only
> looks into simple cases, but not the case of overloaded ctors in a tree
> of COMPONENT_REF (INDIRECT_REF (something, BASELINK (OVERLOAD))). This
> patch expands the is_ctor determining logic.

I don't understand how we can get here without having already done 
overload resolution.  At this point we should have a simple function 
pointer.

Jason

On 02/23/2011 00:14 AM, Jason Merrill wrote:
>> build_aggr_init_expr() wraps a ctor-calling CALL_EXPR tree node
>> with an AGGR_INIT_EXPR tree node, whose expression type is always
>> VOID_TYPE. When it determines whether the routine being called is
>> a ctor or not, it only looks into simple cases, but not the case
>> of overloaded ctors in a tree of
>> COMPONENT_REF (INDIRECT_REF (something, BASELINK (OVERLOAD))).
>> This patch expands the is_ctor determining logic.
> 
> I don't understand how we can get here without having already done
> overload resolution.  At this point we should have a simple function
> pointer.

Good point. I looked into the issue a bit further and here is some more
observation.

Given the following code:

struct A
{
  A(int);
};

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

for the conditional expression in the function template, the following
user-defined conversion sequence is determined in order to get the type
of the 3rd operand converted to that of the 2nd one:

    ck_identity        ck_user              ck_rvalue
int -----------> int -----------> struct A -----------> struct A
                      A::A(int)           A::A(const A&)

Note that above the arrows are annotated with conversion_kind and below
the arrows are the functions involved in the conversions.

When cp/call.c:convert_like_real() performs the conversion sequence,
something interesting happens during the last step, i.e. ck_rvalue. Here
is the backtrace at that point:

#0 build_new_method_call
#1 build_special_member_call
#2 build_temp
#3 convert_like_real
#4 build_conditional_expr
#5 build_x_conditional_expr
...

I can see that build_new_method_call does the overload resolution
correctly, selects the compiler-generated copy constructor A::A(const
A&), and builds up a CALL_EXPR node with a simple function pointer to
A::A(const A&), i.e.

  CALL_EXPR (ADDR_EXPR (FUNCTION_DECL (A::A(const A&))))

_However_, just before returning this CALL_EXPR node,
build_new_method_call has some special handling of situations if the
parser is in the middle of template declaration processing, i.e. when
processing_template_dec != 0

I don't fully understand that piece of code, but it seems related
with the handling of non-dependent expressions (in order to make
template instantiation more like ordinary parsing?). If the parser
is still processing a template declaration, just as in our case,
build_new_method_call reverts to use the original set of candidate
functions to construct and return the following node:

  CALL_EXPR (CONPONENT_REF (INDIRECT_REF ((A*)0), BASELINK (OVERLOAD)))

Eventually this CALL_EXPR node is returned to convert_like_real, which
calls build_cplus_new to declare and initialize a temporary variable
using this node. When build_target_expr is further called inside
build_cplus_new, the assertion failure occurs due to the 'incompatible'
types, as the temporary variable is a RECORD_TYPE of A and the CALL_EXPR
has a type of POINTER_TYPE to A.

So this is how we get into build_target_expr without the overload
resolution performed. Is it an expected behaviour?


Thanks,
Yufeng

On 2/22/2011 03:59 AM, Mark Mitchell wrote:
> 
>> http://gcc.gnu.org/ml/gcc-patches/2011-02/msg00689.html
> 
>> Any comment will be appreciated.
> 
> I would think that when we call the constructor on ARM we should cast
> the result to "void".  Then we would presumably not have to change
> build_aggr_init_expr.  Does that work?

Do you mean casting the result of _all_ constructor calls on ARM to
"void" or just for that particular case? I'm not sure whether this
works or whether this will introduce any inconsistency.

I wonder if there is any particular concern about changing
build_aggr_init_expr, or any reason that it's not necessary for the
'is_ctor' to consider overloaded constructor calls.


Many thanks,
Yufeng

On 02/25/2011 11:12 AM, Yufeng Zhang wrote:
> So this is how we get into build_target_expr without the overload
> resolution performed. Is it an expected behaviour?

Thanks for the analysis.  We shouldn't be calling build_aggr_init_expr 
for such a CALL_EXPR.  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?

Jason