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.