On Thu, Nov 2, 2017 at 1:08 PM, Marek Polacek <pola...@redhat.com> wrote:
> On Thu, Nov 02, 2017 at 09:53:33AM -0400, Jason Merrill wrote:
>> On Thu, Nov 2, 2017 at 8:21 AM, Richard Biener <rguent...@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.
>
> That was my thinking too. Also, I don't see anything target-specific in
> is_empty_type.
We probably want to call them something like default_is_empty_type and
default_is_empty_record, though.
>> > 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"?
>
> Sure, that works for me. Changed.
>
>> > 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?
>
> So I don't know what to do here, I could go either way. The explicit
> checks with ?: striked me as ugly but I can go back on that.
>
>> > 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)).
>
> Okay, done.
>
>> 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.
>
> For one thing I thought we should be consistent in treating these two
> structs, regarding being empty:
>
> struct S { struct { } a; int a[0]; };
> struct T { struct { } a; int a[]; };
>
> Without the (ugly, I know) special handling in is_empty_type, only T would be
> considered empty.
Why would T be considered empty? I don't see anything in
is_empty_type that would cause that.
> And that would mean that the g++.dg/abi/empty23.C test
> crashes (the abort triggers). The problem seems to be that when we're passing
> a pointer to an empty struct, it got turned into a null pointer.
That seems like a bug that needs fixing regardless of whether we end
up considering S and T empty.
> And yeah, when passing such a struct by value, the flexible array member is
> ignored. The ABI of passing struct with a flexible array member has changed
> in
> GCC 4.4.
Can we add a warning about such pass/return (as a separate patch)?
> + if (TREE_ADDRESSABLE (type))
> + return false;
I think we want to abort in this case, as the front end should have
turned this into an invisible reference already.
Jason
