Re: [Patch, fortran] PR29786 - [4.1/4.2/4.3 Regression] Initialization of overlapping variables: Not implemented

[Paul Thomas <paulthomas2 at wanadoo dot fr>]

Brooks,

Tobias had already taken a look at this and found a flaw involving 
derived types that I was intending to fix this weekend.
> I think this is OK to commit, modulo a few questions/comments 
> mentioned below, and one PR that will need to be filed against it.
I was aware of some of your points and was hoping to address them before 
resubmission.  However, I will take your review to give me the green 
light to commit, once I have sorted them out.
> This seems to rely on the s->offset values being in ascending order; 
> otherwise there will be false positives.  Can we rely on this?
I concluded so, although I will check the logic once more.
> > +  /* Now absorb all the initializer data into a single vector,
> > +     whilst checking for overlapping, unequal values.  */
> > +  data = (unsigned char*)alloca ((size_t)length);
>
> For large arrays, the data array could be quite large.  Do we really 
> want to use alloca to put it on the stack?
I did wonder about this when I changed from get_mem to alloca - I was of 
the opinion that this equivalencing could/should only be done for small 
arrays.  I'll revert it.
> > +  memset (data, '\0', (size_t)length);
>
> I think this line should have a TODO comment above it, noting that it 
> may need to be changed (or at least looked at) when -finit-local-zero 
> sorts of options are implemented to provide values other than zero.
Will do.
> [...]
> > +  known_align = 0;
> > +  if (known_align == 0 || known_align > BIGGEST_ALIGNMENT)
> > +    known_align = BIGGEST_ALIGNMENT;
>
> This if-statement is always true, because of the preceeding line.  
> What did you really mean here?  :)

This is a residue from the code that I copied to do this - thanks for 
spotting it.
> [...]
> > +  for (i = 0; i < (int)len; i++)
> > +    {
> > +      if (data[i] && (check[i] != data[i]))
> > +    {
> > +      gfc_error ("Overlapping unequal initializers in EQUIVALENCE "
> > +             "at %L", &e->where);
> > +      return 0;
> > +    }
> > +    }
>
> This will, of course, fail to detect the overlapping unequal 
> initializers if the first one is initializing things to zero.

And this is at the root of the problem with derived types.  A previous 
version had another array, matching 'data', that was filled with OxFF's 
for every initialised element of the equivalence.  I might have to 
revert to that.
> IMO, it's probably okay to go ahead and commit this with this flaw, 
> but if you do, please file a PR about it and add a TODO comment to the 
> code. Also, please assign the PR to me unless you think you'll have 
> time to fix it.  :)
>
> > +      len = len + gfc_merge_initializers (ts, c->expr, &data[len],
> > +                          length - len);
> > +
> > +      gcc_assert (len <= length);
>
> This assert is IMO unnecessary, since it can be seen solely from 
> analysis of this function that it will always be true.

This arose from the original version that did not use the target-memory 
machinery - I will remove it.
> > +  subroutine int4_int4
> > +      integer(4)         NUNITS(4)
> > +      integer(4)         o
> > +      equivalence (o,nunits(3))
>
> "o" seems like a singularly poor choice for a variable name, and 
> "NUNITS" looks like it should mean something but it doesn't.  Could 
> these perhaps be A and B instead?
'tis the reporter's choice - I take your point and will change it.


Many thanks

Paul
>