Richard Biener <rguent...@suse.de> writes:
> 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.
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
