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

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

On 26/07/2023 16:26, Jason Merrill wrote:
> On 6/28/23 06:35, Alex Coplan wrote:
> > Hi,
> > 
> > This patch implements clang's __has_feature and __has_extension in GCC.
> > This is a v2 of the original RFC posted here:
> > 
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-May/617878.html
> > 
> > Changes since v1:
> >   - Follow the clang behaviour where -pedantic-errors means that
> >     __has_extension behaves exactly like __has_feature.
> >   - We're now more conservative with reporting C++ features as extensions
> >     available in C++98. For features where we issue a pedwarn in C++98
> >     mode, we no longer report these as available extensions for C++98.
> >   - Switch to using a hash_map to store the features. As well as ensuring
> >     lookup is constant time, this allows us to dynamically register
> >     features (right now based on frontend, but later we could allow the
> >     target to register additional features).
> >   - Also implement some Objective-C features, add a langhook to dispatch
> >     to each frontend to allow it to register language-specific features.
> 
> Hmm, it seems questionable to use a generic langhook for something that the
> generic code doesn't care about, only the c-family front ends.  A common
> pattern in c-family is to declare a signature in c-common.h and define it
> differently for the various front-ends, i.e. in the *-lang.cc files.

Thanks. I wasn't sure if, for each frontend, there was a source file
that gets linked into exactly one frontend, but it looks like the
*-lang.cc files will do the job. I'll rework the patch to drop the
langhook and use this approach instead.

> 
> > There is an outstanding question around what to do with
> > cxx_binary_literals in the C frontend for C2x. Should we introduce a new
> > c_binary_literals feature that is a feature in C2x and an extension
> > below that, or should we just continue using the cxx_binary_literals
> > feature and mark that as a standard feature in C2x? See the comment in
> > c_feature_table in the patch.
> 
> What does clang do here?

The status quo in clang is that there is no identifier that gets
reported as a feature for this in C (even with -std=c2x).
cxx_binary_literals is reported just as an extension (even with -std=c2x).
It does seem that there should be at least one identifier which reports
this as a feature with -std=c2x, though. WDYT?

> 
> > There is also some doubt over what to do with the undocumented "tls"
> > feature.  In clang this is gated on whether the target supports TLS, but
> > in clang (unlike GCC) it is a hard error to use TLS when the target
> > doesn't support it.  In GCC I believe you can always use TLS, you just
> > get emulated TLS in the case that the target doesn't support it
> > natively.  So in this patch GCC always reports having the "tls" feature.
> > Would appreciate if anyone has feedback on this aspect.
> 
> Hmm, I don't think GCC always supports TLS, given that the testsuite has a
> predicate to check for that support (and others to check for emulated or
> native support).

Hmm, I see there is a check_effective_target_tls predicate for this,
indeed. I wonder if this might be a holdover, though. I can't seem to
configure a GCC without TLS. Even if I configure with
--target=aarch64-none-elf --disable-tls, for example, I get emutls
if I compile code using thread-local variables.

Do we know of a GCC configuration where thread-local variables actually
get rejected (and hence check_effective_target_tls returns false)?

> 
> But I think it's right to report having "tls" for emulated support.
> 
> > I know Iain was concerned that it should be possible to have
> > target-specific features. Hopefully it is clear that the design in this
> > patch is more amenable in this. I think for Darwin it should be possible
> > to add a targetcm hook to register additional features (either passing
> > through a callback to allow the target code to add to the hash_map, or
> > exposing a separate langhook that the target can call to register
> > features).
> 
> The design seems a bit complicated still, with putting a callback into the
> map.  Do we need the callbacks?  Do we expect the value of __has_feature to
> change at different points in compilation?  Does that happen in clang?

This is a good point. Certainly if we were to add features that depend
on the target architecture features, then this can change mid-way
through a TU, so having this flexibility in the design does provide some
potential future-proofing.

I had a look through the existing features, and I did wonder about cases
like this:

__attribute__((no_sanitize("undefined")))
int f() {
  return __has_feature (undefined_behavior_sanitizer);
}

but of course since __has_feature is evaluated during preprocessing,
there's no way that the attribute could be taken into account here (and
indeed clang does not).

I'll drop the callbacks from the patch for now, unless you think we
should keep them for future-proofing.

> 
> > Bootstrapped/regtested on aarch64-linux-gnu and x86_64-apple-darwin. Any
> > thoughts?
> 
> Most of the patch needs more comments, particularly before various top-level
> definitions.

Ack. I'll add more comments in the next revision.

Thanks a lot for the review.

Alex

> 
> Jason
>