diff mbox series

c++: Don't advertise cxx_constexpr_string_builtins [PR113658]

Message ID ZbzDi+RJsBUBKnvS@arm.com
State New
Headers show
Series c++: Don't advertise cxx_constexpr_string_builtins [PR113658] | expand

Commit Message

Alex Coplan Feb. 2, 2024, 10:27 a.m. UTC 
Bootstrapped/regtested on x86_64-apple-darwin, OK for trunk?

Thanks,
Alex

-- >8 --

When __has_feature was introduced for GCC 14, I included the feature
cxx_constexpr_string_builtins, since of the relevant string builtins
that GCC implements, it seems to support constexpr evaluation of those
builtins.

However, as the PR shows, GCC doesn't implement the full list of
builtins in the clang documentation.  After enumerating the builtins,
the clang docs [1] say:

> Support for constant expression evaluation for the above builtins can
> be detected with __has_feature(cxx_constexpr_string_builtins).

and a strict reading of this would suggest we can't really support
constexpr evaluation of a builtin if we don't implement the builtin in
the first place.

So the conservatively correct thing to do seems to be to stop
advertising the feature altogether to avoid failing to build code which
assumes the presence of this feature implies the presence of all the
builtins listed in the clang documentation.

[1] : https://clang.llvm.org/docs/LanguageExtensions.html#string-builtins

gcc/cp/ChangeLog:

	PR c++/113658
	* cp-objcp-common.cc (cp_feature_table): Remove entry for
	cxx_constexpr_string_builtins.

gcc/testsuite/ChangeLog:

	PR c++/113658
	* g++.dg/ext/pr113658.C: New test.

Comments

Marek Polacek Feb. 2, 2024, 2:34 p.m. UTC | #1 
On Fri, Feb 02, 2024 at 10:27:23AM +0000, Alex Coplan wrote:
> Bootstrapped/regtested on x86_64-apple-darwin, OK for trunk?
> 
> Thanks,
> Alex
> 
> -- >8 --
> 
> When __has_feature was introduced for GCC 14, I included the feature
> cxx_constexpr_string_builtins, since of the relevant string builtins
> that GCC implements, it seems to support constexpr evaluation of those
> builtins.
> 
> However, as the PR shows, GCC doesn't implement the full list of
> builtins in the clang documentation.  After enumerating the builtins,
> the clang docs [1] say:
> 
> > Support for constant expression evaluation for the above builtins can
> > be detected with __has_feature(cxx_constexpr_string_builtins).
> 
> and a strict reading of this would suggest we can't really support
> constexpr evaluation of a builtin if we don't implement the builtin in
> the first place.
> 
> So the conservatively correct thing to do seems to be to stop
> advertising the feature altogether to avoid failing to build code which
> assumes the presence of this feature implies the presence of all the
> builtins listed in the clang documentation.
> 
> [1] : https://clang.llvm.org/docs/LanguageExtensions.html#string-builtins
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/113658
> 	* cp-objcp-common.cc (cp_feature_table): Remove entry for
> 	cxx_constexpr_string_builtins.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/113658
> 	* g++.dg/ext/pr113658.C: New test.

> diff --git a/gcc/cp/cp-objcp-common.cc b/gcc/cp/cp-objcp-common.cc
> index f06edf04ef0..85dde0459fa 100644
> --- a/gcc/cp/cp-objcp-common.cc
> +++ b/gcc/cp/cp-objcp-common.cc
> @@ -110,7 +110,6 @@ static constexpr cp_feature_info cp_feature_table[] =
>    { "cxx_alignof", cxx11 },
>    { "cxx_attributes", cxx11 },
>    { "cxx_constexpr", cxx11 },
> -  { "cxx_constexpr_string_builtins", cxx11 },
>    { "cxx_decltype", cxx11 },
>    { "cxx_decltype_incomplete_return_types", cxx11 },
>    { "cxx_default_function_template_args", cxx11 },
> diff --git a/gcc/testsuite/g++.dg/ext/pr113658.C b/gcc/testsuite/g++.dg/ext/pr113658.C
> new file mode 100644
> index 00000000000..f4a34888f28
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ext/pr113658.C

Might be better to name this has-feature2.C

> @@ -0,0 +1,13 @@

Please include
// PR c++/113658

> +// { dg-do compile }
> +// { dg-options "" }

Why dg-options ""?  It doesn't seem to have any purpose here.

> +// PR113658: we shouldn't declare support for cxx_constexpr_string_builtins as
> +// GCC is missing some of the builtins that clang implements.
> +
> +#if __has_feature (cxx_constexpr_string_builtins)
> +#error
> +#endif
> +
> +#if __has_extension (cxx_constexpr_string_builtins)
> +#error
> +#endif


Marek
Alex Coplan Feb. 2, 2024, 3:45 p.m. UTC | #2 
On 02/02/2024 09:34, Marek Polacek wrote:
> On Fri, Feb 02, 2024 at 10:27:23AM +0000, Alex Coplan wrote:
> > Bootstrapped/regtested on x86_64-apple-darwin, OK for trunk?
> > 
> > Thanks,
> > Alex
> > 
> > -- >8 --
> > 
> > When __has_feature was introduced for GCC 14, I included the feature
> > cxx_constexpr_string_builtins, since of the relevant string builtins
> > that GCC implements, it seems to support constexpr evaluation of those
> > builtins.
> > 
> > However, as the PR shows, GCC doesn't implement the full list of
> > builtins in the clang documentation.  After enumerating the builtins,
> > the clang docs [1] say:
> > 
> > > Support for constant expression evaluation for the above builtins can
> > > be detected with __has_feature(cxx_constexpr_string_builtins).
> > 
> > and a strict reading of this would suggest we can't really support
> > constexpr evaluation of a builtin if we don't implement the builtin in
> > the first place.
> > 
> > So the conservatively correct thing to do seems to be to stop
> > advertising the feature altogether to avoid failing to build code which
> > assumes the presence of this feature implies the presence of all the
> > builtins listed in the clang documentation.
> > 
> > [1] : https://clang.llvm.org/docs/LanguageExtensions.html#string-builtins
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	PR c++/113658
> > 	* cp-objcp-common.cc (cp_feature_table): Remove entry for
> > 	cxx_constexpr_string_builtins.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	PR c++/113658
> > 	* g++.dg/ext/pr113658.C: New test.
> 
> > diff --git a/gcc/cp/cp-objcp-common.cc b/gcc/cp/cp-objcp-common.cc
> > index f06edf04ef0..85dde0459fa 100644
> > --- a/gcc/cp/cp-objcp-common.cc
> > +++ b/gcc/cp/cp-objcp-common.cc
> > @@ -110,7 +110,6 @@ static constexpr cp_feature_info cp_feature_table[] =
> >    { "cxx_alignof", cxx11 },
> >    { "cxx_attributes", cxx11 },
> >    { "cxx_constexpr", cxx11 },
> > -  { "cxx_constexpr_string_builtins", cxx11 },
> >    { "cxx_decltype", cxx11 },
> >    { "cxx_decltype_incomplete_return_types", cxx11 },
> >    { "cxx_default_function_template_args", cxx11 },
> > diff --git a/gcc/testsuite/g++.dg/ext/pr113658.C b/gcc/testsuite/g++.dg/ext/pr113658.C
> > new file mode 100644
> > index 00000000000..f4a34888f28
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/ext/pr113658.C
> 
> Might be better to name this has-feature2.C
> 
> > @@ -0,0 +1,13 @@
> 
> Please include
> // PR c++/113658

Can do.

> 
> > +// { dg-do compile }
> > +// { dg-options "" }
> 
> Why dg-options ""?  It doesn't seem to have any purpose here.

That is to disable -pedantic-errors which IIRC is added by default in
the testsuite options.

With -pedantic-errors we would have __has_extension behaving like
__has_feature, and I wanted to check in the test that this doesn't get
reported as a feature or extension.

Incidentally it also means we don't have to provide a dummy declaration,
with -pedantic-errors we would get a warning about an empty TU which
would make the test fail.

Thanks,
Alex

> 
> > +// PR113658: we shouldn't declare support for cxx_constexpr_string_builtins as
> > +// GCC is missing some of the builtins that clang implements.
> > +
> > +#if __has_feature (cxx_constexpr_string_builtins)
> > +#error
> > +#endif
> > +
> > +#if __has_extension (cxx_constexpr_string_builtins)
> > +#error
> > +#endif
> 
> 
> Marek
>
Marek Polacek Feb. 2, 2024, 4:27 p.m. UTC | #3 
On Fri, Feb 02, 2024 at 03:45:48PM +0000, Alex Coplan wrote:
> On 02/02/2024 09:34, Marek Polacek wrote:
> > On Fri, Feb 02, 2024 at 10:27:23AM +0000, Alex Coplan wrote:
> > > Bootstrapped/regtested on x86_64-apple-darwin, OK for trunk?
> > > 
> > > Thanks,
> > > Alex
> > > 
> > > -- >8 --
> > > 
> > > When __has_feature was introduced for GCC 14, I included the feature
> > > cxx_constexpr_string_builtins, since of the relevant string builtins
> > > that GCC implements, it seems to support constexpr evaluation of those
> > > builtins.
> > > 
> > > However, as the PR shows, GCC doesn't implement the full list of
> > > builtins in the clang documentation.  After enumerating the builtins,
> > > the clang docs [1] say:
> > > 
> > > > Support for constant expression evaluation for the above builtins can
> > > > be detected with __has_feature(cxx_constexpr_string_builtins).
> > > 
> > > and a strict reading of this would suggest we can't really support
> > > constexpr evaluation of a builtin if we don't implement the builtin in
> > > the first place.
> > > 
> > > So the conservatively correct thing to do seems to be to stop
> > > advertising the feature altogether to avoid failing to build code which
> > > assumes the presence of this feature implies the presence of all the
> > > builtins listed in the clang documentation.
> > > 
> > > [1] : https://clang.llvm.org/docs/LanguageExtensions.html#string-builtins
> > > 
> > > gcc/cp/ChangeLog:
> > > 
> > > 	PR c++/113658
> > > 	* cp-objcp-common.cc (cp_feature_table): Remove entry for
> > > 	cxx_constexpr_string_builtins.
> > > 
> > > gcc/testsuite/ChangeLog:
> > > 
> > > 	PR c++/113658
> > > 	* g++.dg/ext/pr113658.C: New test.
> > 
> > > diff --git a/gcc/cp/cp-objcp-common.cc b/gcc/cp/cp-objcp-common.cc
> > > index f06edf04ef0..85dde0459fa 100644
> > > --- a/gcc/cp/cp-objcp-common.cc
> > > +++ b/gcc/cp/cp-objcp-common.cc
> > > @@ -110,7 +110,6 @@ static constexpr cp_feature_info cp_feature_table[] =
> > >    { "cxx_alignof", cxx11 },
> > >    { "cxx_attributes", cxx11 },
> > >    { "cxx_constexpr", cxx11 },
> > > -  { "cxx_constexpr_string_builtins", cxx11 },
> > >    { "cxx_decltype", cxx11 },
> > >    { "cxx_decltype_incomplete_return_types", cxx11 },
> > >    { "cxx_default_function_template_args", cxx11 },
> > > diff --git a/gcc/testsuite/g++.dg/ext/pr113658.C b/gcc/testsuite/g++.dg/ext/pr113658.C
> > > new file mode 100644
> > > index 00000000000..f4a34888f28
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/ext/pr113658.C
> > 
> > Might be better to name this has-feature2.C
> > 
> > > @@ -0,0 +1,13 @@
> > 
> > Please include
> > // PR c++/113658
> 
> Can do.
> 
> > 
> > > +// { dg-do compile }
> > > +// { dg-options "" }
> > 
> > Why dg-options ""?  It doesn't seem to have any purpose here.
> 
> That is to disable -pedantic-errors which IIRC is added by default in
> the testsuite options.

Right.

> With -pedantic-errors we would have __has_extension behaving like
> __has_feature, and I wanted to check in the test that this doesn't get
> reported as a feature or extension.
 
Oh I see.  A comment to that effect might be helpful.

Otherwise LGTM, thanks.

Marek
Jakub Jelinek Feb. 2, 2024, 4:32 p.m. UTC | #4 
On Fri, Feb 02, 2024 at 11:27:09AM -0500, Marek Polacek wrote:
> > With -pedantic-errors we would have __has_extension behaving like
> > __has_feature, and I wanted to check in the test that this doesn't get
> > reported as a feature or extension.
>  
> Oh I see.  A comment to that effect might be helpful.

Don't we have over 1200 other tests with dg-options "" which
don't have comments why they are doing that?

	Jakub
Marek Polacek Feb. 2, 2024, 4:37 p.m. UTC | #5 
On Fri, Feb 02, 2024 at 05:32:31PM +0100, Jakub Jelinek wrote:
> On Fri, Feb 02, 2024 at 11:27:09AM -0500, Marek Polacek wrote:
> > > With -pedantic-errors we would have __has_extension behaving like
> > > __has_feature, and I wanted to check in the test that this doesn't get
> > > reported as a feature or extension.
> >  
> > Oh I see.  A comment to that effect might be helpful.
> 
> Don't we have over 1200 other tests with dg-options "" which
> don't have comments why they are doing that?

Yes, but the __has_extension / __has_feature thing doesn't seem
obvious at all, and the test passed for me with and without
dg-options "".

But I'm fine with the patch either way.

Marek
Jason Merrill Feb. 9, 2024, 5:55 p.m. UTC | #6 
On 2/2/24 10:45, Alex Coplan wrote:
> On 02/02/2024 09:34, Marek Polacek wrote:
>> On Fri, Feb 02, 2024 at 10:27:23AM +0000, Alex Coplan wrote:
>>> Bootstrapped/regtested on x86_64-apple-darwin, OK for trunk?
>>>
>>> Thanks,
>>> Alex
>>>
>>> -- >8 --
>>>
>>> When __has_feature was introduced for GCC 14, I included the feature
>>> cxx_constexpr_string_builtins, since of the relevant string builtins
>>> that GCC implements, it seems to support constexpr evaluation of those
>>> builtins.
>>>
>>> However, as the PR shows, GCC doesn't implement the full list of
>>> builtins in the clang documentation.  After enumerating the builtins,
>>> the clang docs [1] say:
>>>
>>>> Support for constant expression evaluation for the above builtins can
>>>> be detected with __has_feature(cxx_constexpr_string_builtins).
>>>
>>> and a strict reading of this would suggest we can't really support
>>> constexpr evaluation of a builtin if we don't implement the builtin in
>>> the first place.
>>>
>>> So the conservatively correct thing to do seems to be to stop
>>> advertising the feature altogether to avoid failing to build code which
>>> assumes the presence of this feature implies the presence of all the
>>> builtins listed in the clang documentation.
>>>
>>> [1] : https://clang.llvm.org/docs/LanguageExtensions.html#string-builtins
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	PR c++/113658
>>> 	* cp-objcp-common.cc (cp_feature_table): Remove entry for
>>> 	cxx_constexpr_string_builtins.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	PR c++/113658
>>> 	* g++.dg/ext/pr113658.C: New test.
>>
>>> diff --git a/gcc/cp/cp-objcp-common.cc b/gcc/cp/cp-objcp-common.cc
>>> index f06edf04ef0..85dde0459fa 100644
>>> --- a/gcc/cp/cp-objcp-common.cc
>>> +++ b/gcc/cp/cp-objcp-common.cc
>>> @@ -110,7 +110,6 @@ static constexpr cp_feature_info cp_feature_table[] =
>>>     { "cxx_alignof", cxx11 },
>>>     { "cxx_attributes", cxx11 },
>>>     { "cxx_constexpr", cxx11 },
>>> -  { "cxx_constexpr_string_builtins", cxx11 },
>>>     { "cxx_decltype", cxx11 },
>>>     { "cxx_decltype_incomplete_return_types", cxx11 },
>>>     { "cxx_default_function_template_args", cxx11 },
>>> diff --git a/gcc/testsuite/g++.dg/ext/pr113658.C b/gcc/testsuite/g++.dg/ext/pr113658.C
>>> new file mode 100644
>>> index 00000000000..f4a34888f28
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/ext/pr113658.C
>>
>> Might be better to name this has-feature2.C
>>
>> Please include
>> // PR c++/113658
> 
> Can do.

OK with those two testcase adjustments.

Jason
diff mbox series

Patch

diff --git a/gcc/cp/cp-objcp-common.cc b/gcc/cp/cp-objcp-common.cc
index f06edf04ef0..85dde0459fa 100644
--- a/gcc/cp/cp-objcp-common.cc
+++ b/gcc/cp/cp-objcp-common.cc
@@ -110,7 +110,6 @@  static constexpr cp_feature_info cp_feature_table[] =
   { "cxx_alignof", cxx11 },
   { "cxx_attributes", cxx11 },
   { "cxx_constexpr", cxx11 },
-  { "cxx_constexpr_string_builtins", cxx11 },
   { "cxx_decltype", cxx11 },
   { "cxx_decltype_incomplete_return_types", cxx11 },
   { "cxx_default_function_template_args", cxx11 },
diff --git a/gcc/testsuite/g++.dg/ext/pr113658.C b/gcc/testsuite/g++.dg/ext/pr113658.C
new file mode 100644
index 00000000000..f4a34888f28
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/pr113658.C
@@ -0,0 +1,13 @@ 
+// { dg-do compile }
+// { dg-options "" }
+
+// PR113658: we shouldn't declare support for cxx_constexpr_string_builtins as
+// GCC is missing some of the builtins that clang implements.
+
+#if __has_feature (cxx_constexpr_string_builtins)
+#error
+#endif
+
+#if __has_extension (cxx_constexpr_string_builtins)
+#error
+#endif