Re: [PATCH][RFC] c-family: Implement __has_feature and __has_extension [PR60512]

[Alex Coplan <alex.coplan@arm.com>]

Hi Iain,

On 14/05/2023 17:05, Iain Sandoe wrote:
> Hi Alex,
> 
> thanks for working on this.
> 
> I’ve applied this patch and evaluated on a few Darwin versions (which is the
> target currently most affected, I believe):
> 
> > On 9 May 2023, at 13:07, Alex Coplan <alex.coplan@arm.com> wrote:
> 
> > This patch implements clang's __has_feature and __has_extension in GCC.
> 
> Thanks, this blocks consuming Darwin SDK headers “properly” (PR 90709 as
> linked to  60512) (which is why I had a WIP patch too).
> 
> So I am very keen to see this land in GCC-14, but have some  issues to deal 
> with and would be looking for ideas about how to handle them by extending or
> amending the patch.
> 
> The main concern I have at the moment is that it seems to me that we need
> more flexible and general predicates for declaring feature/ext support:
> 
>   a) on target (see below for examples)
>   b) on potentially multiple flags and language version at the same time (see below)
>   c) what about features that exist for a closed range of language versions?
> 
> As mentioned by Jakub in a conversation about this on irc (months ago!) the
> current identifiers potentially clash with use symbols.
> 
> IFF we add feature designations (which IMO we should, since this approach does
> help simplify testcases and configurations) we should add them into the
> implementation namespace:
> 
> e.g. ( for C) 
> _GNU_nested_functions or __nested_functions
> 
> > Currently the patch aims to implement all documented features (and some
> > undocumented ones) following the documentation at
> > https://clang.llvm.org/docs/LanguageExtensions.html
> 
> TL;DR 
> without guards or target-specific opt out this breaks bootstrap on Darwin.

Thanks for trying out the patch and pointing this out, this blocker has
now been addressed by relaxing the C++ parser as per
g:b106f11dc6adb8df15cc5c268896d314c76ca35f.

The patch can now survive bootstrap on Darwin (it looks like we'll need
to adjust some Objective-C++ tests in light of the new pedwarn, but that
looks to be straightforward).

<snip>

> (one reason to allow target opt-in/out of specific features)
> 
> > with the following omissions:
> 
> > - Objective-C-specific features.
> 
> I can clearly append the objective-c(++) cases to the end of the respective
> lists, but then we need to make them conditional on language, version and
> dialect (some will not be appropriate to GNU runtime).
> 
> this is why I think we need more flexible predicates on declaring features
> and extensions.

Would it help mitigate these concerns if I implemented some Objective-C
features as part of this patch (say, those implemented by your WIP
patch)?

My feeling is that the vast majority of extensions / features have
similar logic, so we should exploit that redundancy to keep things terse
in the encoding for the general case. Where we need more flexible
predicates (e.g. for objc_nonfragile_abi in your WIP patch), those can
be handled on a case-by-case basis by adding a new enumerator and logic
to handle that specially.

What do you think, does that sound OK to you?

> 
> ----
> 
> index 2b4c82facf7..5b8429244b2 100644
> --- a/gcc/c-family/c-common.cc
> +++ b/gcc/c-family/c-common.cc
> 
> +struct hf_feature_info
> 
> +  { "enumerator_attributes",		  0, 0 },
> +  { "tls", 0, 0 },
> 
> Do all GCC targets support tls?

This is a good point. In clang, the features tls, c_thread_local, and
cxx_thread_local are all gated on whether the target supports TLS.
But in clang, it is a hard error to use TLS variables on a target which
doesn't support TLS. So it seems the features are used to check whether
code can make use of TLS constructs.

In GCC, AFAICT, TLS variables never get rejected, since GCC just uses
emulated TLS in the case that the target doesn't support TLS for real.

This then begs the question of how these features should be interpreted.
For c{,xx}_thread_local I'd expect that we want them to return true
whenever the language-level constructs are useable (even if we end up
using emutls).

I think it's defensible to take the position that GCC "always supports
TLS" since (AFAIK) you can make use of thread-local variables regardless
of whether the target really supports TLS (since you just get emutls if
it doesn't). So it's not clear which interpretation we should use for
the "tls" feature.

> 
> What about things like this:
> 
>  attribute_availability_tvos, 
>  attribute_availability_watchos, 
>  attribute_availability_driverkit, 

FWIW, clang looks to define these unconditionally, so restricting these
to a given target would be deviating from its precedent.

However, I don't think it would be hard to extend the implementation in
this patch to support target-specific features if required. I think
perhaps a langhook that targets can call to add their own features would
be a reasonable approach.

> ?
> 
> Even if they are implemented centrally, it is unlikely that all targets would want
> to claim support (although note that the availability stuff does seem now to be
> used on other platforms - like android, zos and hlsl.)
> 
> (non-bug-related reasons to consider target-specific predicates)
> 
> ===
> 
> Once again, thanks for working on this - and I am keen to see it land,
> Iain
> 

Thanks,
Alex