On Wed, 22 Apr 2020, Richard Sandiford wrote:
> Jakub Jelinek <ja...@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.
