Fix PR lto/50492

On Fri, Oct 7, 2011 at 12:37 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> this is another LTO bootstrap failure with Ada enabled.  There are 2 problems:
>
>  1. dwarf2out.c thinks that it is processing pure C++ code for gnat1 so it is
> invoking some C++-specific code paths that aren't compatible with Ada.  Fixed
> by building TRANSLATION_UNIT_DECL nodes in Ada as well.
>
>  2. the recent change:
>
>        * gimple.c (canonicalize_cond_expr_cond): Handle cast from
>        boolean-type.
>
> breaks cross-language inlining.  More specifically, it removes a necessary cast
> between an Ada boolean type and a C boolean type (hence a GIMPLE verification
> failure later) when __gnat_is_absolute_path is inlined into Is_Absolute_Path:
>
>   extern int __gnat_is_absolute_path (char *, int);
>
>   function Is_Absolute_Path (Name : String) return Boolean is
>      function Is_Absolute_Path
>        (Name   : Address;
>         Length : Integer) return Integer;
>      pragma Import (C, Is_Absolute_Path, "__gnat_is_absolute_path");
>   begin
>      return Is_Absolute_Path (Name'Address, Name'Length) /= 0;
>   end Is_Absolute_Path;
>
> We start with:
>
>  Ada_Boolean_Var = (Integer_Var != 0);
>
> and forwprop figures out that (Integer_Var != 0) is equivalent to iftmp.xxx.
> In combine_cond_expr_cond, the constant folder correctly turns this into:
>
>  Ada_Boolean_Var = (Ada_Boolean_Type) iftmp.xxx;
>
> but the call to canonicalize_cond_expr_cond wrongly removes the cast.
>
> Note that changing Ada's boolean_type_node to a C-compatible type doesn't fix
> the problem as the boolean return type of Is_Absolute_Path has to be an Ada
> boolean type in any case, so a cast will be always necessary here.

Hm, but canonicalize_cond_expr_cond is supposed to produce a
tree that is suitable for a condition in a GIMPLE_COND, or a
COND_EXPR.  So the issue is that it is used for a tcc_comparison
on a assignment RHS?  Wouldn't the "proper" fix then be to verify
that the result from forward_propagate_into_comparison_1 in forwprop
is a suitable
replacement of the assign rhs in forward_propagate_into_comparison?

Thus,


?

> Since reverting the canonicalize_cond_expr_cond change apparently has no effect
> (no regressions detected on x86 and x86-64), the proposed fix is just that.
>
> Bootstrapped/regtested on x86_64-suse-linux, OK for mainline?
>
> 2011-10-07  Eric Botcazou  <ebotcazou@adacore.com>
>
>        PR lto/50492
>        * gimple.c (canonicalize_cond_expr_cond): Revert 2011-08-02 change.
> ada/
>        * gcc-interface/gigi.h (gnat_pushdecl): Adjust comment.
>        * gcc-interface/utils.c (global_context): New variable.
>        (gnat_pushdecl): Initialize it and set it as the DECL_CONTEXT of DECLs
>        that are either public external or at top level.  Use "No" macro.
>        (end_subprog_body): Call decl_function_context.
>        (rest_of_subprog_body_compilation): Likewise.
>
>
> --
> Eric Botcazou
>

> Hm, but canonicalize_cond_expr_cond is supposed to produce a
> tree that is suitable for a condition in a GIMPLE_COND, or a
> COND_EXPR.  So the issue is that it is used for a tcc_comparison
> on a assignment RHS?

It is called on (Ada_Boolean_Type) iftmp.xxx, which comes from the RHS of

  Ada_Boolean_Var = (Integer_Var != 0);

> Wouldn't the "proper" fix then be to verify that the result from
> forward_propagate_into_comparison_1 in forwprop is a suitable
> replacement of the assign rhs in forward_propagate_into_comparison?

Not clear whether it is the proper fix, as you explicitly pass the type in the 
call to forward_propagate_into_comparison_1 just above:

  tmp = forward_propagate_into_comparison_1 (stmt,
					     gimple_assign_rhs_code (stmt),
					     TREE_TYPE
					       (gimple_assign_lhs (stmt)),
					     rhs1, rhs2);

and then to combine_cond_expr_cond, so you would expect that both functions 
return a tree with the specified type.  But this would probably work, yes.

On Fri, Oct 7, 2011 at 1:37 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Hm, but canonicalize_cond_expr_cond is supposed to produce a
>> tree that is suitable for a condition in a GIMPLE_COND, or a
>> COND_EXPR.  So the issue is that it is used for a tcc_comparison
>> on a assignment RHS?
>
> It is called on (Ada_Boolean_Type) iftmp.xxx, which comes from the RHS of
>
>  Ada_Boolean_Var = (Integer_Var != 0);
>
>> Wouldn't the "proper" fix then be to verify that the result from
>> forward_propagate_into_comparison_1 in forwprop is a suitable
>> replacement of the assign rhs in forward_propagate_into_comparison?
>
> Not clear whether it is the proper fix, as you explicitly pass the type in the
> call to forward_propagate_into_comparison_1 just above:
>
>  tmp = forward_propagate_into_comparison_1 (stmt,
>                                             gimple_assign_rhs_code (stmt),
>                                             TREE_TYPE
>                                               (gimple_assign_lhs (stmt)),
>                                             rhs1, rhs2);
>
> and then to combine_cond_expr_cond, so you would expect that both functions
> return a tree with the specified type.  But this would probably work, yes.

Yeah, it uses the type to do

  t = fold_binary_loc (gimple_location (stmt), code, type, op0, op1);

and using the "original type" is surely the best idea for that.  It could
also simply use boolean_type_node nowadays, not sure if that
would cause more fails on the type checking that is needed in
forward_propagate_into_comparison.  I suppose at most for Ada,
so maybe you can check that.  If it doesn't make a difference
then stripping the type argument from the call chain and using
boolean_type_node in combine_cond_expr_cond would be a good
cleanup.

But the original suggested patch is ok if it bootstraps & tests if
you just want to go forward.

Thanks,
Richard.

> --
> Eric Botcazou
>