Re: [PATCH] Don't put objects with flexible array members in small data

[Martin Sebor <msebor at gmail dot com>]

On 11/21/19 3:59 AM, Richard Sandiford wrote:
> Sandra Loosemore <sandra@codesourcery.com> writes:
> > This patch is for PR target/92499, a bug reported by GLIBC maintainers
> > against nios2 target.  The initial failure symptom was a linker error
> > resulting from use of GP-relative addressing on an object that wasn't
> > allocated in the small data section.  It turns out that the real problem
> > is that the TARGET_IN_SMALL_DATA_P hook uses the function
> > int_size_in_bytes to check whether an object is "small", and this
> > function treats flexible array members as having size 0.  All backends
> > except for tic6x that implement TARGET_IN_SMALL_DATA_P use a similar
> > implementation of the hook that has the same problem.
> >
> > The solution here is to add a new function that returns -1 as the size
> > of any type with a flexible array member, and adjust all the affected
> > backends to use it.  I swiped the predicate for identifying these types
> > from the C front end.
> >
> > I regression-tested this on nios2-elf and nios2-linux-gnu.  I'm not set
> > up to test any of the other affected back ends, but the code change is
> > trivial and identical to what I did for nios2.  OK to commit?  I'd like
> > to backport this patch to all active branches as well, once it is in on
> > trunk.
>
> For local decls like the ones in the testcase, we should be able to
> query the actual size of the decl using DECL_SIZE(_UNIT).  But for
> externs we have to make conservatively correct assumptions based only
> on the type, so I agree this looks like the right way of fixing it.

In my testing, DECL_SIZE(_UNIT) doesn't reliably report the size
of an object with an initialized flexible array member (I think
pr91672 might be one of the symptoms of this problem).  To work
around it I recently added the component_ref_size function to
tree.c that computes this size.

Martin

> The problem is that it's going to be an ABI break for existing code that
> does something like:
>
>      struct flexible
>      {
>        int length;
>        int data[];
>      };
>
>      extern struct flexible foo;
>      int get_foo (void) { return foo.data[0]; }
>
> since the code might have been compiled on the assumption that foo
> fits within the small-data limit.
>
> So I think we need to restrict the change to targets that have
> explicitly bought into the ABI change, i.e. just nios2 for now.
> It might also be worth mentioning in the release notes.
>
> The target-independent bits are OK for trunk unless someone objects
> in the next 24 hrs.  I'm not sure it's appropriate to backport given
> the ABI change though.
>
> Thanks,
> Richard