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

[Brooks Moses <brooks dot moses at codesourcery dot com>]

Paul Richard Thomas wrote:
> :ADDPATCH fortran:

:REVIEWMAIL:

> Regtested on Cygwin_NT/PIV and, by tonight, on x86_ia64 - OK for trunk and 4.2?

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.

- Brooks


-----------------------------------------------------------------

> +  /* Obtain the size of the union and check if there are any overlapping
> +     initializers.  */
> +  for (s = head; s; s = s->next)
> +    {
> +      HOST_WIDE_INT slen = s->offset + s->length;
> +      if (s->sym->value)
> +	{
> +	  if (s->offset < offset)
> +            overlap = true;
> +	  offset = slen;
> +	}
> +      length = length < slen ? slen : length;
> +    }

This seems to rely on the s->offset values being in ascending order; 
otherwise there will be false positives.  Can we rely on this?

> +  /* 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?

> +  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.

[...]
> +  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?  :)

[...]
> +  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.

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.

> +  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?