| Message ID | 20200422095040.GE2424@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | calls: Introduce cxx17_empty_base_field_p [PR94383] | expand |
On Wed, 22 Apr 2020, Jakub Jelinek wrote: > Hi! > > On Tue, Apr 21, 2020 at 03:58:52PM +0100, Richard Sandiford wrote: > > > if (TREE_CODE (field) != FIELD_DECL) > > > continue; > > > > > > - sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep); > > > + /* Ignore C++17 empty base fields, while their type indicates > > > + they do contain padding, they have zero size and thus don't > > > + contain any padding. */ > > > + if (DECL_ARTIFICIAL (field) > > > + && DECL_NAME (field) == NULL_TREE > > > + && RECORD_OR_UNION_TYPE_P (TREE_TYPE (field)) > > > + && DECL_SIZE (field) > > > + && integer_zerop (DECL_SIZE (field)) > > > + && (*avoid_c17empty_field & AVOID)) > > > + { > > As multiple targets are affected apparently, I believe at least > aarch64, arm, powerpc64le, s390{,x} and ia64, > I think we should have a middle-end predicate for this, so that if we need > to tweak it, we can do it in one spot. > > So is the following ok (of course after testing)? OK. Richard. > 2020-04-22 Jakub Jelinek <jakub@redhat.com> > > PR target/94383 > * calls.h (cxx17_empty_base_field_p): Declare. > * calls.c (cxx17_empty_base_field_p): Define. > > --- gcc/calls.h.jj 2020-01-12 11:54:36.214416411 +0100 > +++ gcc/calls.h 2020-04-22 11:44:09.037853379 +0200 > @@ -135,5 +135,6 @@ extern tree get_attr_nonstring_decl (tre > extern void maybe_warn_nonstring_arg (tree, tree); > extern bool get_size_range (tree, tree[2], bool = false); > extern rtx rtx_for_static_chain (const_tree, bool); > +extern bool cxx17_empty_base_field_p (const_tree); > > #endif // GCC_CALLS_H > --- gcc/calls.c.jj 2020-03-27 22:27:09.615964438 +0100 > +++ gcc/calls.c 2020-04-22 11:44:17.621722376 +0200 > @@ -6261,5 +6261,22 @@ must_pass_va_arg_in_stack (tree type) > return targetm.calls.must_pass_in_stack (arg); > } > > +/* Return true if FIELD is the C++17 empty base field that should > + be ignored for ABI calling convention decisions in order to > + maintain ABI compatibility between C++14 and earlier, which doesn't > + add this FIELD to classes with empty bases, and C++17 and later > + which does. */ > + > +bool > +cxx17_empty_base_field_p (const_tree field) > +{ > + return (TREE_CODE (field) == FIELD_DECL > + && DECL_ARTIFICIAL (field) > + && DECL_NAME (field) == NULL_TREE > + && RECORD_OR_UNION_TYPE_P (TREE_TYPE (field)) > + && DECL_SIZE (field) > + && integer_zerop (DECL_SIZE (field))); > +} > + > /* Tell the garbage collector about GTY markers in this source file. */ > #include "gt-calls.h" > > > Jakub > >
Jakub Jelinek <jakub@redhat.com> writes: > Hi! > > On Tue, Apr 21, 2020 at 03:58:52PM +0100, Richard Sandiford wrote: >> > if (TREE_CODE (field) != FIELD_DECL) >> > continue; >> > >> > - sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep); >> > + /* Ignore C++17 empty base fields, while their type indicates >> > + they do contain padding, they have zero size and thus don't >> > + contain any padding. */ >> > + if (DECL_ARTIFICIAL (field) >> > + && DECL_NAME (field) == NULL_TREE >> > + && RECORD_OR_UNION_TYPE_P (TREE_TYPE (field)) >> > + && DECL_SIZE (field) >> > + && integer_zerop (DECL_SIZE (field)) >> > + && (*avoid_c17empty_field & AVOID)) >> > + { > > As multiple targets are affected apparently, I believe at least > aarch64, arm, powerpc64le, s390{,x} and ia64, > I think we should have a middle-end predicate for this, so that if we need > to tweak it, we can do it in one spot. > > So is the following ok (of course after testing)? > > 2020-04-22 Jakub Jelinek <jakub@redhat.com> > > PR target/94383 > * calls.h (cxx17_empty_base_field_p): Declare. > * calls.c (cxx17_empty_base_field_p): Define. > > --- gcc/calls.h.jj 2020-01-12 11:54:36.214416411 +0100 > +++ gcc/calls.h 2020-04-22 11:44:09.037853379 +0200 > @@ -135,5 +135,6 @@ extern tree get_attr_nonstring_decl (tre > extern void maybe_warn_nonstring_arg (tree, tree); > extern bool get_size_range (tree, tree[2], bool = false); > extern rtx rtx_for_static_chain (const_tree, bool); > +extern bool cxx17_empty_base_field_p (const_tree); > > #endif // GCC_CALLS_H > --- gcc/calls.c.jj 2020-03-27 22:27:09.615964438 +0100 > +++ gcc/calls.c 2020-04-22 11:44:17.621722376 +0200 > @@ -6261,5 +6261,22 @@ must_pass_va_arg_in_stack (tree type) > return targetm.calls.must_pass_in_stack (arg); > } > > +/* Return true if FIELD is the C++17 empty base field that should > + be ignored for ABI calling convention decisions in order to > + maintain ABI compatibility between C++14 and earlier, which doesn't > + add this FIELD to classes with empty bases, and C++17 and later > + which does. */ > + > +bool > +cxx17_empty_base_field_p (const_tree field) > +{ > + return (TREE_CODE (field) == FIELD_DECL > + && DECL_ARTIFICIAL (field) > + && DECL_NAME (field) == NULL_TREE Given what was said on irc about DECL_NAME not necessarily being significant for DECL_ARTIFICIAL decls, would it be better to drop this part of the check? Thanks, Richard > + && RECORD_OR_UNION_TYPE_P (TREE_TYPE (field)) > + && DECL_SIZE (field) > + && integer_zerop (DECL_SIZE (field))); > +} > + > /* Tell the garbage collector about GTY markers in this source file. */ > #include "gt-calls.h" > > > Jakub
On Wed, Apr 22, 2020 at 11:45:19AM +0100, Richard Sandiford wrote: > Given what was said on irc about DECL_NAME not necessarily being > significant for DECL_ARTIFICIAL decls, would it be better to drop > this part of the check? My preference was have it as narrow as possible for the time being, because we are shortly before release. We can replace it with an assertion or whatever later. Perhaps even the predicate should check for non-NULL and non-zero TYPE_SIZE (TREE_TYPE (field)). Jakub
On Wed, 22 Apr 2020, Jakub Jelinek wrote: > On Wed, Apr 22, 2020 at 11:45:19AM +0100, Richard Sandiford wrote: > > Given what was said on irc about DECL_NAME not necessarily being > > significant for DECL_ARTIFICIAL decls, would it be better to drop > > this part of the check? > > My preference was have it as narrow as possible for the time being, > because we are shortly before release. > We can replace it with an assertion or whatever later. > Perhaps even the predicate should check for non-NULL and non-zero > TYPE_SIZE (TREE_TYPE (field)). Btw, do we ever have more than one of those? The predicate doesn't check if the field is the "first" one (does it reliably appear before non-FIELD_DECLs and thus is it always == TYPE_FIELDS (DECL_CONTEXT (field))?) Richard.
On Wed, Apr 22, 2020 at 12:58:52PM +0200, Richard Biener wrote: > On Wed, 22 Apr 2020, Jakub Jelinek wrote: > > > On Wed, Apr 22, 2020 at 11:45:19AM +0100, Richard Sandiford wrote: > > > Given what was said on irc about DECL_NAME not necessarily being > > > significant for DECL_ARTIFICIAL decls, would it be better to drop > > > this part of the check? > > > > My preference was have it as narrow as possible for the time being, > > because we are shortly before release. > > We can replace it with an assertion or whatever later. > > Perhaps even the predicate should check for non-NULL and non-zero > > TYPE_SIZE (TREE_TYPE (field)). > > Btw, do we ever have more than one of those? The predicate doesn't I think there can be at most one in each TYPE_FIELDS chain, but fields of that can have it in their TYPE_FIELDS of their TREE_TYPEs too of course. > check if the field is the "first" one (does it reliably appear > before non-FIELD_DECLs and thus is it always == TYPE_FIELDS (DECL_CONTEXT > (field))?) I don't think anything guarantees that, in C++ I think usually the methods appear first and there can be all kinds of other things in the chain (e.g. using declarations etc.). Jakub
Jakub Jelinek <jakub@redhat.com> writes: > On Wed, Apr 22, 2020 at 11:45:19AM +0100, Richard Sandiford wrote: >> Given what was said on irc about DECL_NAME not necessarily being >> significant for DECL_ARTIFICIAL decls, would it be better to drop >> this part of the check? > > My preference was have it as narrow as possible for the time being, > because we are shortly before release. > We can replace it with an assertion or whatever later. But my point was that, if the DECL_NAME does actually act to exclude some type X during a "normal" frontend-to-asm run, that type X might then be treated differently during LTO, because the DECL_NAME might then be null. (Unless I misunderstood what Richi said on IRC.) So checking DECL_NAME might then introduce an ABI incompatiblity between non-LTO and LTO code for X. Is that not right? Am I just being too paranoid? :-) This is the part that I'm most uncomfortable with as far as aarch64 is concerned. I think for aarch64, just: if (DECL_SIZE (field) && integer_zerop (DECL_SIZE (field))) continue; would be correct from an ABI perspective. The only reason for not doing that is that it might then "fix" the ABI for other cases besides the one that we're trying to fix. So the extra "&&"s can be justified in trying to narrow down the scope/impact of the fix given how late we are in the cycle. (Note: this is just my understanding of the ABI, not a definitive statement.) But as far as the aarch64 ABI is concerned, it's hard to find a good justification for checking the DECL_NAME . Adding that particular "&&" feels more dangerous than safe. Thanks, Richard
On Wed, 22 Apr 2020, Richard Sandiford wrote: > Jakub Jelinek <jakub@redhat.com> writes: > > On Wed, Apr 22, 2020 at 11:45:19AM +0100, Richard Sandiford wrote: > >> Given what was said on irc about DECL_NAME not necessarily being > >> significant for DECL_ARTIFICIAL decls, would it be better to drop > >> this part of the check? > > > > My preference was have it as narrow as possible for the time being, > > because we are shortly before release. > > We can replace it with an assertion or whatever later. > > But my point was that, if the DECL_NAME does actually act to exclude > some type X during a "normal" frontend-to-asm run, that type X might > then be treated differently during LTO, because the DECL_NAME might then > be null. (Unless I misunderstood what Richi said on IRC.) So checking > DECL_NAME might then introduce an ABI incompatiblity between non-LTO and > LTO code for X. > > Is that not right? Am I just being too paranoid? :-) The predicate checks !DECL_NAME (field), it's unlikely that LTO will invent a DECL_NAME out of nothing. What I was saying is that for DECL_ARTIFICIAL/DECL_IGNORED_P fields with DECL_NAME (field) != NULL that LTO might choose to clear DECL_NAME (field) because it is clearly not necessary (it currently does no such thing). > This is the part that I'm most uncomfortable with as far as aarch64 > is concerned. I think for aarch64, just: > > if (DECL_SIZE (field) && integer_zerop (DECL_SIZE (field))) > continue; > > would be correct from an ABI perspective. Either we require that only "complete" fields exist and you can elide the DECL_SIZE (field) check or we assume that !DECL_SIZE (field) fields have zero size. I think decls never have a NULL DECL_SIZE. > The only reason for not > doing that is that it might then "fix" the ABI for other cases besides > the one that we're trying to fix. So the extra "&&"s can be justified > in trying to narrow down the scope/impact of the fix given how late > we are in the cycle. > > (Note: this is just my understanding of the ABI, not a definitive > statement.) We could do if (integer_zerop (DECL_SIZE (field))) { gcc_assert (cxx17_empty_bae_field_p (field)); continue; } so fix it as written in the ABI but assert we've not fixed something we don't know. It'll ICE then and we can reconsider for the found case. Richard.
Richard Biener <rguenther@suse.de> writes: > On Wed, 22 Apr 2020, Richard Sandiford wrote: > >> Jakub Jelinek <jakub@redhat.com> writes: >> > On Wed, Apr 22, 2020 at 11:45:19AM +0100, Richard Sandiford wrote: >> >> Given what was said on irc about DECL_NAME not necessarily being >> >> significant for DECL_ARTIFICIAL decls, would it be better to drop >> >> this part of the check? >> > >> > My preference was have it as narrow as possible for the time being, >> > because we are shortly before release. >> > We can replace it with an assertion or whatever later. >> >> But my point was that, if the DECL_NAME does actually act to exclude >> some type X during a "normal" frontend-to-asm run, that type X might >> then be treated differently during LTO, because the DECL_NAME might then >> be null. (Unless I misunderstood what Richi said on IRC.) So checking >> DECL_NAME might then introduce an ABI incompatiblity between non-LTO and >> LTO code for X. >> >> Is that not right? Am I just being too paranoid? :-) > > The predicate checks !DECL_NAME (field), it's unlikely that LTO > will invent a DECL_NAME out of nothing. Yeah, the case I was talking about was if some theoretical type X had a field with a nonnull DECL_NAME in the frontend, but that name got cleared during LTO. > What I was saying is that for DECL_ARTIFICIAL/DECL_IGNORED_P fields > with DECL_NAME (field) != NULL that LTO might choose to clear > DECL_NAME (field) because it is clearly not necessary (it currently > does no such thing). Ah, OK. So checking the name is OK now, but we'd have to remember to come back to this if we ever did clear the DECL_NAME in future, otherwise it sounds like we could introduce a new ABI incompatibility. I still think it's less risky not to check at all though... >> This is the part that I'm most uncomfortable with as far as aarch64 >> is concerned. I think for aarch64, just: >> >> if (DECL_SIZE (field) && integer_zerop (DECL_SIZE (field))) >> continue; >> >> would be correct from an ABI perspective. > > Either we require that only "complete" fields exist and you can > elide the DECL_SIZE (field) check or we assume that !DECL_SIZE (field) > fields have zero size. I think decls never have a NULL DECL_SIZE. Even better :-) >> The only reason for not >> doing that is that it might then "fix" the ABI for other cases besides >> the one that we're trying to fix. So the extra "&&"s can be justified >> in trying to narrow down the scope/impact of the fix given how late >> we are in the cycle. >> >> (Note: this is just my understanding of the ABI, not a definitive >> statement.) > > We could do > > if (integer_zerop (DECL_SIZE (field))) > { > gcc_assert (cxx17_empty_bae_field_p (field)); > continue; > } > > so fix it as written in the ABI but assert we've not fixed something > we don't know. It'll ICE then and we can reconsider for the found case. AIUI, DECL_SIZEs are expected to be zero for things like :0 bitfields though. Thanks, Richard
On Wed, Apr 22, 2020 at 12:17:02PM +0100, Richard Sandiford wrote:
> But my point was that, if the DECL_NAME does actually act to exclude
I'm fine with dropping DECL_NAME test there, on the other side would like to
add
&& TYPE_SIZE (TREE_TYPE (field))
&& !integer_zerop (TYPE_SIZE (TREE_TYPE (field)))
in there because that is what all these empty bases should satisfy too.
If the predicate says that it is the C++17 empty base field, then it better
should be a narrow check.
Now, in the backend, one has the -Wpsabi diagnostics that also talks about
C++17, so I think it is better to use that predicate in there.
But, what one could do is verify that all other
if (DECL_SIZE (field) && integer_zerop (DECL_SIZE (field)))
fields don't matter for the ABI decisions.
So
if (TREE_CODE (field) != FIELD_DECL)
continue;
if (cxx17_empty_base_field_p (field))
{
/* hint -Wpsabi warning here somehow. */
continue;
}
sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep);
/* Verify that other zero sized fields don't affect the
ABI decisions. */
if (DECL_SIZE (field) && integer_zerop (DECL_SIZE (field)))
gcc_assert (sub_count == 0);
if (sub_count < 0)
return -1;
count += sub_count;
?
Jakub
Jakub Jelinek <jakub@redhat.com> writes: > On Wed, Apr 22, 2020 at 12:17:02PM +0100, Richard Sandiford wrote: >> But my point was that, if the DECL_NAME does actually act to exclude > > I'm fine with dropping DECL_NAME test there, on the other side would like to > add > && TYPE_SIZE (TREE_TYPE (field)) > && !integer_zerop (TYPE_SIZE (TREE_TYPE (field))) > in there because that is what all these empty bases should satisfy too. Sounds good to me FWIW. > If the predicate says that it is the C++17 empty base field, then it better > should be a narrow check. > Now, in the backend, one has the -Wpsabi diagnostics that also talks about > C++17, so I think it is better to use that predicate in there. Ack. > But, what one could do is verify that all other > if (DECL_SIZE (field) && integer_zerop (DECL_SIZE (field))) > fields don't matter for the ABI decisions. > So > if (TREE_CODE (field) != FIELD_DECL) > continue; > > if (cxx17_empty_base_field_p (field)) > { > /* hint -Wpsabi warning here somehow. */ > continue; > } > > sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep); > > /* Verify that other zero sized fields don't affect the > ABI decisions. */ > if (DECL_SIZE (field) && integer_zerop (DECL_SIZE (field))) > gcc_assert (sub_count == 0); > > if (sub_count < 0) > return -1; > count += sub_count; > ? I fear this will actually trip in practice, but I'd have to go back and check. (This came up in the context of the SVE parameter-passing rules, where we ended up deliberately checking DECL_SIZE to avoid zero-size user-level decls.) E.g. I'd expect a :0 bitfield to have a zero size and an integer TREE_TYPE, so the recursive call should return -1. AIUI we should skip these kinds of bitfield too, but again that's just my understanding, not a definitive statement. Either way, I think we should deal with the other zero-size cases separately after GCC 10. Thanks, Richard
On Wed, Apr 22, 2020 at 01:33:45PM +0100, Richard Sandiford wrote: > Jakub Jelinek <jakub@redhat.com> writes: > > On Wed, Apr 22, 2020 at 12:17:02PM +0100, Richard Sandiford wrote: > >> But my point was that, if the DECL_NAME does actually act to exclude > > > > I'm fine with dropping DECL_NAME test there, on the other side would like to > > add > > && TYPE_SIZE (TREE_TYPE (field)) > > && !integer_zerop (TYPE_SIZE (TREE_TYPE (field))) > > in there because that is what all these empty bases should satisfy too. > > Sounds good to me FWIW. Thus below in the patch form. Ok for trunk? > > /* Verify that other zero sized fields don't affect the > > ABI decisions. */ > > if (DECL_SIZE (field) && integer_zerop (DECL_SIZE (field))) > > gcc_assert (sub_count == 0); > > > > if (sub_count < 0) > > return -1; > > count += sub_count; > > ? > > I fear this will actually trip in practice, but I'd have to go back and > check. (This came up in the context of the SVE parameter-passing rules, > where we ended up deliberately checking DECL_SIZE to avoid zero-size > user-level decls.) > > E.g. I'd expect a :0 bitfield to have a zero size and an integer > TREE_TYPE, so the recursive call should return -1. AIUI we should > skip these kinds of bitfield too, but again that's just my understanding, > not a definitive statement. Indeed, struct S { int : 0; }; in C has (at least on x86) sizeof 0, so does struct T { struct S a, b, c, d; }; Sure, the backend needs to decide whether those change the ABI decisions or not. E.g. on rs6000 where I was considering similar check that assertion triggers on struct S { int : 0; }; struct T { struct S a, b, c, d; } t; struct U { struct T e; float f, g, h, i; } u; void foo (struct U); int bar (void) { foo (u); return 1; } 2020-04-22 Jakub Jelinek <jakub@redhat.com> PR target/94383 * calls.h (cxx17_empty_base_field_p): Declare. * calls.c (cxx17_empty_base_field_p): Define. --- gcc/calls.h.jj 2020-01-12 11:54:36.214416411 +0100 +++ gcc/calls.h 2020-04-22 11:44:09.037853379 +0200 @@ -135,5 +135,6 @@ extern tree get_attr_nonstring_decl (tre extern void maybe_warn_nonstring_arg (tree, tree); extern bool get_size_range (tree, tree[2], bool = false); extern rtx rtx_for_static_chain (const_tree, bool); +extern bool cxx17_empty_base_field_p (const_tree); #endif // GCC_CALLS_H --- gcc/calls.c.jj 2020-03-27 22:27:09.615964438 +0100 +++ gcc/calls.c 2020-04-22 11:44:17.621722376 +0200 @@ -6261,5 +6261,23 @@ must_pass_va_arg_in_stack (tree type) return targetm.calls.must_pass_in_stack (arg); } +/* Return true if FIELD is the C++17 empty base field that should + be ignored for ABI calling convention decisions in order to + maintain ABI compatibility between C++14 and earlier, which doesn't + add this FIELD to classes with empty bases, and C++17 and later + which does. */ + +bool +cxx17_empty_base_field_p (const_tree field) +{ + return (TREE_CODE (field) == FIELD_DECL + && DECL_ARTIFICIAL (field) + && RECORD_OR_UNION_TYPE_P (TREE_TYPE (field)) + && DECL_SIZE (field) + && integer_zerop (DECL_SIZE (field)) + && TYPE_SIZE (TREE_TYPE (field)) + && !integer_zerop (TYPE_SIZE (TREE_TYPE (field)))); +} + /* Tell the garbage collector about GTY markers in this source file. */ #include "gt-calls.h" Jakub
On Wed, 2020-04-22 at 11:50 +0200, Jakub Jelinek via Gcc-patches wrote: > Hi! > > On Tue, Apr 21, 2020 at 03:58:52PM +0100, Richard Sandiford wrote: > > > if (TREE_CODE (field) != FIELD_DECL) > > > continue; > > > > > > - sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep); > > > + /* Ignore C++17 empty base fields, while their type indicates > > > + they do contain padding, they have zero size and thus don't > > > + contain any padding. */ > > > + if (DECL_ARTIFICIAL (field) > > > + && DECL_NAME (field) == NULL_TREE > > > + && RECORD_OR_UNION_TYPE_P (TREE_TYPE (field)) > > > + && DECL_SIZE (field) > > > + && integer_zerop (DECL_SIZE (field)) > > > + && (*avoid_c17empty_field & AVOID)) > > > + { > > As multiple targets are affected apparently, I believe at least > aarch64, arm, powerpc64le, s390{,x} and ia64, > I think we should have a middle-end predicate for this, so that if we need > to tweak it, we can do it in one spot. > > So is the following ok (of course after testing)? > > 2020-04-22 Jakub Jelinek <jakub@redhat.com> > > PR target/94383 > * calls.h (cxx17_empty_base_field_p): Declare. > * calls.c (cxx17_empty_base_field_p): Define. OK. jeff >
--- gcc/calls.h.jj 2020-01-12 11:54:36.214416411 +0100 +++ gcc/calls.h 2020-04-22 11:44:09.037853379 +0200 @@ -135,5 +135,6 @@ extern tree get_attr_nonstring_decl (tre extern void maybe_warn_nonstring_arg (tree, tree); extern bool get_size_range (tree, tree[2], bool = false); extern rtx rtx_for_static_chain (const_tree, bool); +extern bool cxx17_empty_base_field_p (const_tree); #endif // GCC_CALLS_H --- gcc/calls.c.jj 2020-03-27 22:27:09.615964438 +0100 +++ gcc/calls.c 2020-04-22 11:44:17.621722376 +0200 @@ -6261,5 +6261,22 @@ must_pass_va_arg_in_stack (tree type) return targetm.calls.must_pass_in_stack (arg); } +/* Return true if FIELD is the C++17 empty base field that should + be ignored for ABI calling convention decisions in order to + maintain ABI compatibility between C++14 and earlier, which doesn't + add this FIELD to classes with empty bases, and C++17 and later + which does. */ + +bool +cxx17_empty_base_field_p (const_tree field) +{ + return (TREE_CODE (field) == FIELD_DECL + && DECL_ARTIFICIAL (field) + && DECL_NAME (field) == NULL_TREE + && RECORD_OR_UNION_TYPE_P (TREE_TYPE (field)) + && DECL_SIZE (field) + && integer_zerop (DECL_SIZE (field))); +} + /* Tell the garbage collector about GTY markers in this source file. */ #include "gt-calls.h"