refinements to definition of TREE_READONLY ?

refinements to definition of TREE_READONLY ?

[Olivier Hainque]

Hello,

According to comments in PR/35493, when DECL_INITIAL is null on a
TREE_READONLY decl, the middle-end may assume the value to be zero.

This is not explicit from the current definitions in tree.h, we'd
like to understand if this is actually the case and to suggest a doc
extension to this effect.

We'd also like to get to an agreement on what TREE_READONLY means on a
decl with non static storage, if anything at all.

The kind of issues we're seeing is ...

 Ada front-end sets TREE_RO and DECL_INITIAL on a stack decl with
 initializer but never assigned later on,

 gimplify_decl_expr turns this into an explicit assignment,
 clears DECL_INITIAL and leaves TREE_RO set (creating an
 unexpected assignment with TREE_RO on the lhs).

 later middle-end passes (e.g. tree-sra) turn this into an
 assignment from 0 (null DECL_INITIAL).

Thanks in advance for your feedback,

Olivier

Re: refinements to definition of TREE_READONLY ?

[Olivier Hainque]

Olivier Hainque wrote:
[...]

 While we're at it, it would be nice to settle similar notions
 about TREE_CONSTANT. There was an exchange along similar lines at

   http://gcc.gnu.org/ml/gcc/2005-08/msg00686.html

 

Re: refinements to definition of TREE_READONLY ?

[Richard Guenther]

On Fri, Jul 11, 2008 at 6:15 PM, Olivier Hainque <hainque@adacore.com> wrote:
> Hello,
>
> According to comments in PR/35493, when DECL_INITIAL is null on a
> TREE_READONLY decl, the middle-end may assume the value to be zero.
>
> This is not explicit from the current definitions in tree.h, we'd
> like to understand if this is actually the case and to suggest a doc
> extension to this effect.

A doc extension is ok.

> We'd also like to get to an agreement on what TREE_READONLY means on a
> decl with non static storage, if anything at all.

Good question...

> The kind of issues we're seeing is ...
>
>  Ada front-end sets TREE_RO and DECL_INITIAL on a stack decl with
>  initializer but never assigned later on,
>
>  gimplify_decl_expr turns this into an explicit assignment,
>  clears DECL_INITIAL and leaves TREE_RO set (creating an
>  unexpected assignment with TREE_RO on the lhs).

... I suggest to not clear DECL_INITIAL here.

>  later middle-end passes (e.g. tree-sra) turn this into an
>  assignment from 0 (null DECL_INITIAL).

But in the end tree-sra shouldn't try to look at DECL_INITIAL from non-static
storage.

So it looks like there are multiple issues here and a bug in tree-sra.

Richard.

> Thanks in advance for your feedback,
>
> Olivier
>
>
>
>
>
>
>
>
>
>

Re: refinements to definition of TREE_READONLY ?

[Olivier Hainque]

Richard Guenther wrote:
> A doc extension is ok.

 Understood, as soon as we agree on what it should say :)
 
> > We'd also like to get to an agreement on what TREE_READONLY means on a
> > decl with non static storage, if anything at all.
> 
> Good question...

> >  Ada front-end sets TREE_RO and DECL_INITIAL on a stack decl with
> >  initializer but never assigned later on,

> >  gimplify_decl_expr turns this into an explicit assignment,
> >  clears DECL_INITIAL and leaves TREE_RO set (creating an
> >  unexpected assignment with TREE_RO on the lhs).
> 
> ... I suggest to not clear DECL_INITIAL here.

 Leaving an assignment with TREE_RO on the lhs ?
 
 Other options we thought of were

   Don't set TREE_RO on the decl if !TREE_STATIC, in gigi (we might
   decide it doesn't make sense and document accordingly).

   Clear TREE_RO in gimplify_decl_expr. We could leave
   DECL_INITIAL there in this case as well.

> >  later middle-end passes (e.g. tree-sra) turn this into an
> >  assignment from 0 (null DECL_INITIAL).

> But in the end tree-sra shouldn't try to look at DECL_INITIAL from
> non-static storage.

 Sorry, this part was confused on my side: the issue we had with
 sra was with a static rhs (global Ada constant integer), of value 1,
 marked TREE_RO and without DECL_INITIAL.

Re: refinements to definition of TREE_READONLY ?

[Richard Guenther]

On Fri, Jul 11, 2008 at 6:49 PM, Olivier Hainque <hainque@adacore.com> wrote:
> Richard Guenther wrote:
>> A doc extension is ok.
>
>  Understood, as soon as we agree on what it should say :)
>
>> > We'd also like to get to an agreement on what TREE_READONLY means on a
>> > decl with non static storage, if anything at all.
>>
>> Good question...
>
>> >  Ada front-end sets TREE_RO and DECL_INITIAL on a stack decl with
>> >  initializer but never assigned later on,
>
>> >  gimplify_decl_expr turns this into an explicit assignment,
>> >  clears DECL_INITIAL and leaves TREE_RO set (creating an
>> >  unexpected assignment with TREE_RO on the lhs).
>>
>> ... I suggest to not clear DECL_INITIAL here.
>
>  Leaving an assignment with TREE_RO on the lhs ?
>
>  Other options we thought of were
>
>   Don't set TREE_RO on the decl if !TREE_STATIC, in gigi (we might
>   decide it doesn't make sense and document accordingly).

That would work.

>   Clear TREE_RO in gimplify_decl_expr. We could leave
>   DECL_INITIAL there in this case as well.

This as well.

>> >  later middle-end passes (e.g. tree-sra) turn this into an
>> >  assignment from 0 (null DECL_INITIAL).
>
>> But in the end tree-sra shouldn't try to look at DECL_INITIAL from
>> non-static storage.
>
>  Sorry, this part was confused on my side: the issue we had with
>  sra was with a static rhs (global Ada constant integer), of value 1,
>  marked TREE_RO and without DECL_INITIAL.

So what happened?  The code in tree-sra looking at DECL_INITIAL looks fine.

Richard.

Re: refinements to definition of TREE_READONLY ?

[Olivier Hainque]

Richard Guenther wrote:
> >   Don't set TREE_RO on the decl if !TREE_STATIC, in gigi (we might
> >   decide it doesn't make sense and document accordingly).
> 
> That would work.

 Both doing the thing in gigi and deciding that TREE_RO only makes
 sense if TREE_STATIC as well ?

 (I have no strong feeling about it, just asking to make sure I'm
 getting what you meant) 
 
> >   Clear TREE_RO in gimplify_decl_expr. We could leave
> >   DECL_INITIAL there in this case as well.
> 
> This as well.

 OK.

> >  Sorry, this part was confused on my side: the issue we had with
> >  sra was with a static rhs (global Ada constant integer), of value 1,
> >  marked TREE_RO and without DECL_INITIAL.
> 
> So what happened?

 The lhs was initialized with 0 instead of 1.
 
> The code in tree-sra looking at DECL_INITIAL looks fine.

 If it's ok to assume !DECL_INITIAL is equivalent to 0, indeed.

 Is it really ?






 
 

Re: refinements to definition of TREE_READONLY ?

[Richard Guenther]

On Fri, Jul 11, 2008 at 7:22 PM, Olivier Hainque <hainque@adacore.com> wrote:
> Richard Guenther wrote:
>> >   Don't set TREE_RO on the decl if !TREE_STATIC, in gigi (we might
>> >   decide it doesn't make sense and document accordingly).
>>
>> That would work.
>
>  Both doing the thing in gigi and deciding that TREE_RO only makes
>  sense if TREE_STATIC as well ?

Yes.

>  (I have no strong feeling about it, just asking to make sure I'm
>  getting what you meant)
>
>> >   Clear TREE_RO in gimplify_decl_expr. We could leave
>> >   DECL_INITIAL there in this case as well.
>>
>> This as well.
>
>  OK.
>
>> >  Sorry, this part was confused on my side: the issue we had with
>> >  sra was with a static rhs (global Ada constant integer), of value 1,
>> >  marked TREE_RO and without DECL_INITIAL.
>>
>> So what happened?
>
>  The lhs was initialized with 0 instead of 1.
>
>> The code in tree-sra looking at DECL_INITIAL looks fine.
>
>  If it's ok to assume !DECL_INITIAL is equivalent to 0, indeed.
>
>  Is it really ?

For static storage yes, which it seems to test:

      else if (TREE_CODE (rhs) == VAR_DECL
               && TREE_STATIC (rhs)
               && TREE_READONLY (rhs)
               && targetm.binds_local_p (rhs))
        fns->init (lhs_elt, DECL_INITIAL (rhs), bsi);

Richard.

Re: refinements to definition of TREE_READONLY ?

[Eric Botcazou]

> For static storage yes, which it seems to test:
>
>       else if (TREE_CODE (rhs) == VAR_DECL
>                && TREE_STATIC (rhs)
>                && TREE_READONLY (rhs)
>                && targetm.binds_local_p (rhs))
>         fns->init (lhs_elt, DECL_INITIAL (rhs), bsi);

I think there is a problem with binds_local_p though, which explains why PR 
tree-opt/35493 has been fixed for ELF but not for PE-COFF (PR ada/36207):
what if we have DECL_EXTERNAL, targetm.binds_local_p and no DECL_INITIAL?
In this case, the code will replace the VAR_DECL with zero, which is wrong.
tree-ssa-ccp.c:get_symbol_constant_value will do the same.

For example, expand_expr_real_1 does test the non-nullity of DECL_INITIAL:

	else if (optimize >= 1
		 && modifier != EXPAND_CONST_ADDRESS
		 && modifier != EXPAND_INITIALIZER
		 && modifier != EXPAND_MEMORY
		 && TREE_READONLY (array) && ! TREE_SIDE_EFFECTS (array)
		 && TREE_CODE (array) == VAR_DECL && DECL_INITIAL (array)
		 && TREE_CODE (DECL_INITIAL (array)) != ERROR_MARK
		 && targetm.binds_local_p (array))

Here's the hook for PE-COFF:

bool
i386_pe_binds_local_p (const_tree exp)
{
  /* PE does not do dynamic binding.  Indeed, the only kind of
     non-local reference comes from a dllimport'd symbol.  */
  if ((TREE_CODE (exp) == VAR_DECL || TREE_CODE (exp) == FUNCTION_DECL)
      && DECL_DLLIMPORT_P (exp))
    return false;

  return true;
}

-- 
Eric Botcazou

Re: refinements to definition of TREE_READONLY ?

[Richard Guenther]

On Fri, Jul 11, 2008 at 8:17 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> For static storage yes, which it seems to test:
>>
>>       else if (TREE_CODE (rhs) == VAR_DECL
>>                && TREE_STATIC (rhs)
>>                && TREE_READONLY (rhs)
>>                && targetm.binds_local_p (rhs))
>>         fns->init (lhs_elt, DECL_INITIAL (rhs), bsi);
>
> I think there is a problem with binds_local_p though, which explains why PR
> tree-opt/35493 has been fixed for ELF but not for PE-COFF (PR ada/36207):
> what if we have DECL_EXTERNAL, targetm.binds_local_p and no DECL_INITIAL?
> In this case, the code will replace the VAR_DECL with zero, which is wrong.
> tree-ssa-ccp.c:get_symbol_constant_value will do the same.

But it also tests TREE_STATIC which should not be true for automatic variables.
The binds_local_p check is only to guard against overriding the symbol with
an external definition.

Richard.

> For example, expand_expr_real_1 does test the non-nullity of DECL_INITIAL:
>
>        else if (optimize >= 1
>                 && modifier != EXPAND_CONST_ADDRESS
>                 && modifier != EXPAND_INITIALIZER
>                 && modifier != EXPAND_MEMORY
>                 && TREE_READONLY (array) && ! TREE_SIDE_EFFECTS (array)
>                 && TREE_CODE (array) == VAR_DECL && DECL_INITIAL (array)
>                 && TREE_CODE (DECL_INITIAL (array)) != ERROR_MARK
>                 && targetm.binds_local_p (array))
>
> Here's the hook for PE-COFF:
>
> bool
> i386_pe_binds_local_p (const_tree exp)
> {
>  /* PE does not do dynamic binding.  Indeed, the only kind of
>     non-local reference comes from a dllimport'd symbol.  */
>  if ((TREE_CODE (exp) == VAR_DECL || TREE_CODE (exp) == FUNCTION_DECL)
>      && DECL_DLLIMPORT_P (exp))
>    return false;
>
>  return true;
> }
>
> --
> Eric Botcazou
>

Re: refinements to definition of TREE_READONLY ?

[Eric Botcazou]

> But it also tests TREE_STATIC which should not be true for automatic
> variables. The binds_local_p check is only to guard against overriding the
> symbol with an external definition.

You're right, TREE_STATIC is the key here, in other words you're entitled to 
assume that !DECL_INITIAL => DECL_INITIAL == 0 for TREE_READONLY+TREE_STATIC 
and not TREE_READONLY alone.

-- 
Eric Botcazou

Re: refinements to definition of TREE_READONLY ?

[Olivier Hainque]

Hi Richard,

Still looking into this issue.

Our current understanding is that our initial bug was indirectly
caused by the Ada front-end setting TREE_STATIC on a DECL_EXTERNAL
constant, which it shouldn't do.

The straightforward fix to that uncovered corner issues with the
way we set DECL_CONTEXT in some special cases.

Investigating further ...

Thanks for your constructive feedback,

Olivier