Need a hint or more likely help

Need a hint or more likely help

[Steve Kargl]

All, consider this simple code:

  module foo
    contains
      subroutine bar 
        character(len=:), allocatable :: s(:)
        call bah(s)
     end subroutine bar
  end module foo

If one compiles with -fdump-tree-original, one see (with some pruning)

  void bar ()
  {
    integer(kind=8) .s;
    struct array01_character(kind=1) s;

The above two lines seem to be ok.

    bitsizetype D.4319;
    sizetype D.4320;

    try
      {
        D.4319 = (bitsizetype) (sizetype) NON_LVALUE_EXPR <.s> * 8;
        D.4320 = (sizetype) NON_LVALUE_EXPR <.s>;
        s.data = 0B;
        s.dtype = {.elem_len=(unsigned long) .s, .version=0, .rank=1, .type=6};
        bah ((character(kind=1)[0:][1:.s] * restrict) s.data, .s);
    }

This is bad. .s is undefined.  I've trace this to trans-array.cc:11531

  if (sym->ts.type == BT_CHARACTER
      && !INTEGER_CST_P (sym->ts.u.cl->backend_decl))
    {
      gfc_conv_string_length (sym->ts.u.cl, NULL, &init);
      gfc_trans_vla_type_sizes (sym, &init);

The problem here is that sym->ts.u.cl->length == NULL.  If I change
the conditional to 

  if (sym->ts.type == BT_CHARACTER
      && sym->ts.u.cl->length
      && !INTEGER_CST_P (sym->ts.u.cl->backend_decl))

then the option -fdump-tree-original produces

  void bar ()
  {
    integer(kind=8) .s;
    struct array01_character(kind=1) s;

    try
      {
        s.data = 0B;
        s.dtype = {.elem_len=(unsigned long) .s, .version=0, .rank=1, .type=6};
        bah ((character(kind=1)[0:][1:.s] * restrict) s.data, .s);
      }

which looks good except I don't know what the reference to .s here
means.  Is this correct or should we set .s to zero by artificially
setting sym->ts.u.cl->length to, say, zero length?

-- 
Steve

Re: Need a hint or more likely help

[Steve Kargl]

On Sat, Feb 10, 2024 at 06:00:42PM -0800, Steve Kargl wrote:
> 
> The problem here is that sym->ts.u.cl->length == NULL.  If I change
> the conditional to 
> 
>   if (sym->ts.type == BT_CHARACTER
>       && sym->ts.u.cl->length
>       && !INTEGER_CST_P (sym->ts.u.cl->backend_decl))
> 
> then the option -fdump-tree-original produces
> 
>   void bar ()
>   {
>     integer(kind=8) .s;
>     struct array01_character(kind=1) s;
> 
>     try
>       {
>         s.data = 0B;
>         s.dtype = {.elem_len=(unsigned long) .s, .version=0, .rank=1, .type=6};
>         bah ((character(kind=1)[0:][1:.s] * restrict) s.data, .s);
>       }
> 

Well, that leads to a regression.  The goes removes references
to an undefined variable, and gives a regression?  I tried to
artificial set length to 0 (and -1 but -1 is 0) and that leads
to even more regressions??

-- 
Steve

Re: Need a hint or more likely help

[Mikael Morin]

Hello,

Le 11/02/2024 à 03:00, Steve Kargl a écrit :
> All, consider this simple code:
> 
>    module foo
>      contains
>        subroutine bar
>          character(len=:), allocatable :: s(:)
>          call bah(s)
>       end subroutine bar
>    end module foo
> 
> If one compiles with -fdump-tree-original, one see (with some pruning)
> 
>    void bar ()
>    {
>      integer(kind=8) .s;
>      struct array01_character(kind=1) s;
> 
> The above two lines seem to be ok.
> 
>      bitsizetype D.4319;
>      sizetype D.4320;
> 
>      try
>        {
>          D.4319 = (bitsizetype) (sizetype) NON_LVALUE_EXPR <.s> * 8;
>          D.4320 = (sizetype) NON_LVALUE_EXPR <.s>;
>          s.data = 0B;
>          s.dtype = {.elem_len=(unsigned long) .s, .version=0, .rank=1, .type=6};
>          bah ((character(kind=1)[0:][1:.s] * restrict) s.data, .s);
>      }
> 
> This is bad. .s is undefined.  I've trace this to trans-array.cc:11531
> 
>    if (sym->ts.type == BT_CHARACTER
>        && !INTEGER_CST_P (sym->ts.u.cl->backend_decl))
>      {
>        gfc_conv_string_length (sym->ts.u.cl, NULL, &init);
>        gfc_trans_vla_type_sizes (sym, &init);
> 
> The problem here is that sym->ts.u.cl->length == NULL.  If I change
> the conditional to
> 
>    if (sym->ts.type == BT_CHARACTER
>        && sym->ts.u.cl->length
>        && !INTEGER_CST_P (sym->ts.u.cl->backend_decl))
> 
No, I think sym->ts.u.cl->length == NULL is correct for deferred length.
It is set only when there is an expression defining the length:
     character(len=size(somevar)) :: s(:)

> then the option -fdump-tree-original produces
> 
>    void bar ()
>    {
>      integer(kind=8) .s;
>      struct array01_character(kind=1) s;
> 
>      try
>        {
>          s.data = 0B;
>          s.dtype = {.elem_len=(unsigned long) .s, .version=0, .rank=1, .type=6};
>          bah ((character(kind=1)[0:][1:.s] * restrict) s.data, .s);
>        }
> 
> which looks good except I don't know what the reference to .s here
> means.  Is this correct or should we set .s to zero by artificially
> setting sym->ts.u.cl->length to, say, zero length?
> 
Setting the variable .s to zero should be correct in any case, but it's 
not the same as setting the expression.
I think you can use
     if (sym->ts.deferred)
       gfc_add_modify (
         &init,
         sym->ts.u.cl->backend_decl,
         build_zero_cst (TREE_TYPE (sym->ts.u.cl->backen_decl))
       );
to set .s to zero.

Actually, setting to any value should be correct (including -1), as the 
value should not be used as long as the data pointer is NULL.

What is not clear is whether the values of D.4319 and D.4320  need 
update when memory is allocated for s.  I think those variables are 
referenced in the type of s (but it's not visible in the dump), and may 
need update when the value of .s changes.

I hope it helps.
Mikael

Re: Need a hint or more likely help

[Steve Kargl]

On Sun, Feb 11, 2024 at 11:56:52AM +0100, Mikael Morin wrote:
> Hello,
> 
> Le 11/02/2024 à 03:00, Steve Kargl a écrit :
> > All, consider this simple code:
> > 
> >    module foo
> >      contains
> >        subroutine bar
> >          character(len=:), allocatable :: s(:)
> >          call bah(s)
> >       end subroutine bar
> >    end module foo
> > 
> > If one compiles with -fdump-tree-original, one see (with some pruning)
> > 
> >    void bar ()
> >    {
> >      integer(kind=8) .s;
> >      struct array01_character(kind=1) s;
> > 
> > The above two lines seem to be ok.
> > 
> >      bitsizetype D.4319;
> >      sizetype D.4320;
> > 
> >      try
> >        {
> >          D.4319 = (bitsizetype) (sizetype) NON_LVALUE_EXPR <.s> * 8;
> >          D.4320 = (sizetype) NON_LVALUE_EXPR <.s>;
> >          s.data = 0B;
> >          s.dtype = {.elem_len=(unsigned long) .s, .version=0, .rank=1, .type=6};
> >          bah ((character(kind=1)[0:][1:.s] * restrict) s.data, .s);
> >      }
> > 
> > This is bad. .s is undefined.  I've trace this to trans-array.cc:11531
> > 
> >    if (sym->ts.type == BT_CHARACTER
> >        && !INTEGER_CST_P (sym->ts.u.cl->backend_decl))
> >      {
> >        gfc_conv_string_length (sym->ts.u.cl, NULL, &init);
> >        gfc_trans_vla_type_sizes (sym, &init);
> > 
> > The problem here is that sym->ts.u.cl->length == NULL.  If I change
> > the conditional to
> > 
> >    if (sym->ts.type == BT_CHARACTER
> >        && sym->ts.u.cl->length
> >        && !INTEGER_CST_P (sym->ts.u.cl->backend_decl))
> > 
> No, I think sym->ts.u.cl->length == NULL is correct for deferred length.
> It is set only when there is an expression defining the length:
>     character(len=size(somevar)) :: s(:)

Yes, you are correct.  I spent some time reading gfc_conv_string_length()
and does appear to address the NULL case.

> > then the option -fdump-tree-original produces
> > 
> >    void bar ()
> >    {
> >      integer(kind=8) .s;
> >      struct array01_character(kind=1) s;
> > 
> >      try
> >        {
> >          s.data = 0B;
> >          s.dtype = {.elem_len=(unsigned long) .s, .version=0, .rank=1, .type=6};
> >          bah ((character(kind=1)[0:][1:.s] * restrict) s.data, .s);
> >        }
> > 
> > which looks good except I don't know what the reference to .s here
> > means.  Is this correct or should we set .s to zero by artificially
> > setting sym->ts.u.cl->length to, say, zero length?
> > 
> Setting the variable .s to zero should be correct in any case, but it's not
> the same as setting the expression.
> I think you can use
>     if (sym->ts.deferred)
>       gfc_add_modify (
>         &init,
>         sym->ts.u.cl->backend_decl,
>         build_zero_cst (TREE_TYPE (sym->ts.u.cl->backen_decl))
>       );
> to set .s to zero.
> 
> Actually, setting to any value should be correct (including -1), as the
> value should not be used as long as the data pointer is NULL.
> 
> What is not clear is whether the values of D.4319 and D.4320  need update
> when memory is allocated for s.  I think those variables are referenced in
> the type of s (but it's not visible in the dump), and may need update when
> the value of .s changes.
> 
> I hope it helps.

Indeed, the above appears to be the solution.  I looked at 
the first 9 dumps from -fdump-tree-all, and by time we get
to a.f90.022t.ssa the temp variables D.4310 and D.4320 seem
to be optimized out/reaped.  Thanks for the help.  I tend
to forget how the trans-* files work, and sometimes need
a reminder.

-- 
Steve

Re: Need a hint or more likely help

[Steve Kargl]

On Sat, Feb 10, 2024 at 06:00:42PM -0800, Steve Kargl wrote:
> All, consider this simple code:
> 
>   module foo
>     contains
>       subroutine bar 
>         character(len=:), allocatable :: s(:)
>         call bah(s)
>      end subroutine bar
>   end module foo
> 
> If one compiles with -fdump-tree-original, one see (with some pruning)
> 
>   void bar ()
>   {
>     integer(kind=8) .s;
>     struct array01_character(kind=1) s;
> 
> The above two lines seem to be ok.
> 
>     bitsizetype D.4319;
>     sizetype D.4320;
> 
>     try
>       {
>         D.4319 = (bitsizetype) (sizetype) NON_LVALUE_EXPR <.s> * 8;
>         D.4320 = (sizetype) NON_LVALUE_EXPR <.s>;
>         s.data = 0B;
>         s.dtype = {.elem_len=(unsigned long) .s, .version=0, .rank=1, .type=6};
>         bah ((character(kind=1)[0:][1:.s] * restrict) s.data, .s);
>     }
> 
> This is bad. .s is undefined.  I've trace this to trans-array.cc:11531

I have created PR113883 with the patch suggested by Mikael and
a testcase.   I have bootstrapped and regression tested the result.
Could I ask someone to commit it?

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113883


* trans-array.cc (gfc_trans_deferred_array): Set length of an unallocated
  character entity to zero to prevent reference to undefined variable.

* testsuite/gfortran.dg/allocatable_length.f90: New test. 




-- 
Steve