Adjust empty class parameter passing ABI (PR c++/60336)

[Jason Merrill]

On Thu, Nov 2, 2017 at 8:21 AM, Richard Biener <rguenther@suse.de> wrote:
> On Wed, 1 Nov 2017, Marek Polacek wrote:
>
>> On Fri, Oct 27, 2017 at 12:46:12PM +0200, Richard Biener wrote:
>> > On Fri, 27 Oct 2017, Jakub Jelinek wrote:
>> >
>> > > On Fri, Oct 27, 2017 at 12:31:46PM +0200, Richard Biener wrote:
>> > > > I fear it doesn't work at all with LTO (you'll always get the old ABI
>> > > > if I read the patch correctly).  This is because the function
>> > > > computing the size looks at flag_abi_version which isn't saved
>> > > > per function / TU.
>> > > >
>> > > > Similarly you'll never get the ABI warning with LTO (less of a big
>> > > > deal of course) because the langhook doesn't reflect things correctly
>> > > > either.
>> > > >
>> > > > So...  can we instead compute whether a type is "empty" according
>> > > > to the ABI early and store the result in the type (thinking of
>> > > > doing this in layout_type?).  Similarly set a flag whether to
>> > > > warn.  Why do you warn from backends / code emission and not
>> > > > from the FEs?  Is that to avoid warnings for calls that got inlined?
>> > > > Maybe the FE could set a flag on the call itself (ok, somewhat
>> > > > awkward to funnel through gimple).
>> > >
>> > > Warning in the FE is too early both because of the inlining, never
>> > > emitted functions and because whether an empty struct is passed differently
>> > > from the past matters on the backend (whether its psABI says it should be
>> > > not passed at all or not).
>> > >
>> > > Perhaps if empty types are rare enough it could be an artificial attribute
>> > > on the type if we can't get a spare bit for that.  But computing in the FE
>> > > or before free_lang_data and saving on the type whether it is empty or not
>> > > seems reasonable to me.
>> >
>> > There are 18 unused bits in tree_type_common if we don't want to re-use
>> > any.  For the warning I first thought of setting TREE_NO_WARNING on it
>> > but that bit is used already.  OTOH given the "fit" of TREE_NO_WARNING
>> > I'd move TYPE_ARTIFICIAL somewhere else.
>>
>> All right, should be done in the below.  I've introduced two new flags,
>> TYPE_EMPTY_P (says whether the type is empty according to the psABI), and
>> TYPE_WARN_EMPTY_P (whether we should warn).  I've added two new fields to
>> type_type_common and moved TYPE_ARTIFICIAL there; TYPE_WARN_EMPTY_P is now
>> mapped to nowarning_flag.  So this should work with LTO, as demonstrated
>> by g++.dg/lto/pr60336_0.C.
>>
>> Regarding LTO and -Wabi warning, I've added Optimization to c.opt so that
>> we get warnings with LTO.  But as pointed out IRC, this doesn't fully work
>> with cross-inlining.  I tried to do some flags merging in inline_call, but
>> that didn't help, one of the problems is that warn_abi_version lives in
>> c-family only.  Not sure if I'll be able to improve things here though.
>>
>> Bootstrapped/regtested on x86_64-linux, ppc64-linux, and aarch64-linux.
>> Bootstrap-lto passed on x86_64-linux and ppc64-linux.
>
> To me the tree.c stuff is_empty_type looks awfully ABI dependent
> and should thus reside in i386.c near the target hook implementation?

I think there should be a default version in common code, to hopefully
be shared by all targets that want this behavior.

> What goes wrong if we do not introduce new int_maybe_empty_type_size
> and maybe_empty_type_size but instead change int_size_in_bytes and
> size_in_bytes to return 0 if TYPE_EMPTY_P ()?  If the ABI can omit
> passing things assuming the size is zero should work as well, no?

We need to distinguish between size in general and size for calling
convention purposes, but the function names should mention the calling
convention rather than "maybe_empty".  Maybe something like
"arg_size_in_bytes"?

> Otherwise I'd really prefer seeing explicit TYPE_EMPTY_P checks
> which would reduce the number of "indirect" greps one has to do when
> looking for effects of TYPE_EMPTY_P.

Hmm, yes, I was hoping we could encapsulate this in target code, but
needing these flags for LTO messes that up; if we can't have full
encapsulation, maybe we want less?

> Still needs FE and target maintainer approval -- the target maintainer
> wants to look at the seemingly ABI independent functions in tree.c.

Instead of moving array_type_nelts_top to tree.c, you can use
integer_minus_onep (array_type_nelts (ftype)).

I'm still not sure why you want to consider a type with a flexible
array member non-empty.  Is this to avoid changing the C ABI?  I'm
surprised it's even allowed to pass/return by value a struct with a
flexible array member, since that doesn't copy the contents of the
array.

Jason