recog: Use parameter packs for operator()

Message ID mpt7dwpwosm.fsf@arm.com
State New
Headers show
Series
  • recog: Use parameter packs for operator()
Related show

Commit Message

Richard Sandiford June 2, 2020, 2 p.m.
This patch uses parameter packs to define insn_gen_fn::operator().
I guess in some ways it's C++-ification for its own sake, but it does
make things simpler and removes the current artificial limit of 16
arguments.

Note that the call is still strongly typed: all arguments have to have
implicit conversions to rtx.  Error messages for bad arguments look
reasonable.

I'm sure there are more elegant ways of getting the function type,
but this version at least fits on one line, so I didn't try too
hard to find an alternative.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Richard


2020-06-02  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* recog.h (insn_gen_fn::f0, insn_gen_fn::f1, insn_gen_fn::f2)
	(insn_gen_fn::f3, insn_gen_fn::f4, insn_gen_fn::f5, insn_gen_fn::f6)
	(insn_gen_fn::f7, insn_gen_fn::f8, insn_gen_fn::f9, insn_gen_fn::f10)
	(insn_gen_fn::f11, insn_gen_fn::f12, insn_gen_fn::f13)
	(insn_gen_fn::f14, insn_gen_fn::f15, insn_gen_fn::f16): Delete.
	(insn_gen_fn::operator()): Replace overloaded definitions with
	a parameter-pack version.
---
 gcc/recog.h | 40 +++++-----------------------------------
 1 file changed, 5 insertions(+), 35 deletions(-)

Comments

Kees Cook via Gcc-patches June 9, 2020, 8:22 p.m. | #1
On Tue, 2020-06-02 at 15:00 +0100, Richard Sandiford wrote:
> This patch uses parameter packs to define insn_gen_fn::operator().

> I guess in some ways it's C++-ification for its own sake, but it does

> make things simpler and removes the current artificial limit of 16

> arguments.

> 

> Note that the call is still strongly typed: all arguments have to have

> implicit conversions to rtx.  Error messages for bad arguments look

> reasonable.

> 

> I'm sure there are more elegant ways of getting the function type,

> but this version at least fits on one line, so I didn't try too

> hard to find an alternative.

> 

> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

> 

> Richard

> 

> 

> 2020-06-02  Richard Sandiford  <richard.sandiford@arm.com>

> 

> gcc/

> 	* recog.h (insn_gen_fn::f0, insn_gen_fn::f1, insn_gen_fn::f2)

> 	(insn_gen_fn::f3, insn_gen_fn::f4, insn_gen_fn::f5, insn_gen_fn::f6)

> 	(insn_gen_fn::f7, insn_gen_fn::f8, insn_gen_fn::f9, insn_gen_fn::f10)

> 	(insn_gen_fn::f11, insn_gen_fn::f12, insn_gen_fn::f13)

> 	(insn_gen_fn::f14, insn_gen_fn::f15, insn_gen_fn::f16): Delete.

> 	(insn_gen_fn::operator()): Replace overloaded definitions with

> 	a parameter-pack version.

OK.

Jeff
>
Kees Cook via Gcc-patches June 15, 2020, 4:13 p.m. | #2
+  template<typename ...Ts>
+  rtx_insn *operator() (Ts... args...) const

Why is this declared as a variadic template **and** a varargs function?

I think the second ellipsis is wrong, it should be just:

+  template<typename ...Ts>
+  rtx_insn *operator() (Ts... args) const


And this seems like a more direct way to say "a list of N rtx types"
where N is sizeof...(args):

diff --git a/gcc/recog.h b/gcc/recog.h
index 0a71a02c4a9..fd22c58c92a 100644
--- a/gcc/recog.h
+++ b/gcc/recog.h
@@ -294,10 +294,13 @@ struct insn_gen_fn
 {
   typedef void (*stored_funcptr) (void);
 
+  template<typename T> using rtx_t = rtx;
+
   template<typename ...Ts>
-  rtx_insn *operator() (Ts... args...) const
+  rtx_insn *operator() (Ts... args) const
   {
-    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);
+    using funcptr = rtx_insn * (*) (rtx_t<Ts>...);
+    return ((funcptr) func) (args...);
   }
 
   // This is for compatibility of code that invokes functions like

The rtx_t<T> alias is the type 'rtx' for any T. The pack expansion
rtx_t<Ts>... is a list of rtx the same length as the pack Ts.

The 'funcptr' alias sin't necessary, but (IMHO) simplifies the
following line, by splitting the definition of the complicated
function pointer type from its use.
Richard Sandiford June 16, 2020, 10:42 a.m. | #3
Jonathan Wakely <jwakely@redhat.com> writes:
> +  template<typename ...Ts>

> +  rtx_insn *operator() (Ts... args...) const

>

> Why is this declared as a variadic template **and** a varargs function?

>

> I think the second ellipsis is wrong, it should be just:

>

> +  template<typename ...Ts>

> +  rtx_insn *operator() (Ts... args) const


Oops, yes.

> And this seems like a more direct way to say "a list of N rtx types"

> where N is sizeof...(args):

>

> diff --git a/gcc/recog.h b/gcc/recog.h

> index 0a71a02c4a9..fd22c58c92a 100644

> --- a/gcc/recog.h

> +++ b/gcc/recog.h

> @@ -294,10 +294,13 @@ struct insn_gen_fn

>  {

>    typedef void (*stored_funcptr) (void);

>  

> +  template<typename T> using rtx_t = rtx;

> +

>    template<typename ...Ts>

> -  rtx_insn *operator() (Ts... args...) const

> +  rtx_insn *operator() (Ts... args) const

>    {

> -    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);

> +    using funcptr = rtx_insn * (*) (rtx_t<Ts>...);

> +    return ((funcptr) func) (args...);

>    }

>  

>    // This is for compatibility of code that invokes functions like

>

> The rtx_t<T> alias is the type 'rtx' for any T. The pack expansion

> rtx_t<Ts>... is a list of rtx the same length as the pack Ts.


Yeah.  As mentioned on IRC, I'd originally done it like this, but didn't
like the ad-hoc type alias.  I can't remember what name I'd used, but the
problem in both cases is/was that the ad-hoc name looks odd when you're
used to seeing plain “rtx”.  It also felt weird to expose this kind of
internal, function-specific implementation detail at insn_gen_fn scope.

Using decltype also gave nicer error messages, e.g.:

    invalid conversion from ‘int’ to ‘rtx {aka rtx_def*}’

instead of:

    invalid conversion from ‘int’ to ‘insn_gen_fn::rtx_t<int> {aka rtx_def*}’

I think the latter is likely to confuse people when they see it
for the first time.

So in some ways I'd prefer to keep the decltype and just add a void cast
to suppress the warning.  (Seems odd to warn about expressions having no
effect in an unevaluated context.)

But how about the below instead?  Hopefully the alias name is mnemonic
enough.

> The 'funcptr' alias sin't necessary, but (IMHO) simplifies the

> following line, by splitting the definition of the complicated

> function pointer type from its use.


OK, I'll split it out.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.

Richard


Fixes a “left operand of comma has no effect” warning that some were
seeing.  Also fixes a spurious ellipsis that Jonathan Wakely pointed
out.

2020-06-16  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* coretypes.h (first_type): New alias template.
	* recog.h (insn_gen_fn::operator()): Use it instead of a decltype.
	Remove spurious “...” and split the function type out into a typedef.
---
 gcc/coretypes.h | 4 ++++
 gcc/recog.h     | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gcc/coretypes.h b/gcc/coretypes.h
index cda22697cc3..01ec2e23ce2 100644
--- a/gcc/coretypes.h
+++ b/gcc/coretypes.h
@@ -359,6 +359,10 @@ struct kv_pair
   const ValueType value;	/* the value of the name */
 };
 
+/* Alias of the first type, ignoring the second.  */
+template<typename T1, typename T2>
+using first_type = T1;
+
 #else
 
 struct _dont_use_rtx_here_;
diff --git a/gcc/recog.h b/gcc/recog.h
index 0a71a02c4a9..d674d384723 100644
--- a/gcc/recog.h
+++ b/gcc/recog.h
@@ -295,9 +295,10 @@ struct insn_gen_fn
   typedef void (*stored_funcptr) (void);
 
   template<typename ...Ts>
-  rtx_insn *operator() (Ts... args...) const
+  rtx_insn *operator() (Ts... args) const
   {
-    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);
+    typedef rtx_insn *(*funcptr) (first_type<rtx, Ts>...);
+    return ((funcptr) func) (args...);
   }
 
   // This is for compatibility of code that invokes functions like
Kees Cook via Gcc-patches June 16, 2020, 1:14 p.m. | #4
On 16/06/20 11:42 +0100, Richard Sandiford wrote:
>Jonathan Wakely <jwakely@redhat.com> writes:

>> +  template<typename ...Ts>

>> +  rtx_insn *operator() (Ts... args...) const

>>

>> Why is this declared as a variadic template **and** a varargs function?

>>

>> I think the second ellipsis is wrong, it should be just:

>>

>> +  template<typename ...Ts>

>> +  rtx_insn *operator() (Ts... args) const

>

>Oops, yes.

>

>> And this seems like a more direct way to say "a list of N rtx types"

>> where N is sizeof...(args):

>>

>> diff --git a/gcc/recog.h b/gcc/recog.h

>> index 0a71a02c4a9..fd22c58c92a 100644

>> --- a/gcc/recog.h

>> +++ b/gcc/recog.h

>> @@ -294,10 +294,13 @@ struct insn_gen_fn

>>  {

>>    typedef void (*stored_funcptr) (void);

>>

>> +  template<typename T> using rtx_t = rtx;

>> +

>>    template<typename ...Ts>

>> -  rtx_insn *operator() (Ts... args...) const

>> +  rtx_insn *operator() (Ts... args) const

>>    {

>> -    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);

>> +    using funcptr = rtx_insn * (*) (rtx_t<Ts>...);

>> +    return ((funcptr) func) (args...);

>>    }

>>

>>    // This is for compatibility of code that invokes functions like

>>

>> The rtx_t<T> alias is the type 'rtx' for any T. The pack expansion

>> rtx_t<Ts>... is a list of rtx the same length as the pack Ts.

>

>Yeah.  As mentioned on IRC, I'd originally done it like this, but didn't

>like the ad-hoc type alias.  I can't remember what name I'd used, but the

>problem in both cases is/was that the ad-hoc name looks odd when you're

>used to seeing plain “rtx”.  It also felt weird to expose this kind of

>internal, function-specific implementation detail at insn_gen_fn scope.

>

>Using decltype also gave nicer error messages, e.g.:

>

>    invalid conversion from ‘int’ to ‘rtx {aka rtx_def*}’

>

>instead of:

>

>    invalid conversion from ‘int’ to ‘insn_gen_fn::rtx_t<int> {aka rtx_def*}’

>

>I think the latter is likely to confuse people when they see it

>for the first time.

>

>So in some ways I'd prefer to keep the decltype and just add a void cast

>to suppress the warning.  (Seems odd to warn about expressions having no

>effect in an unevaluated context.)


Yes, arguably a G++ bug.

>But how about the below instead?  Hopefully the alias name is mnemonic

>enough.


Works for me, thanks.
Sebastian Huber June 18, 2020, 6:44 a.m. | #5
On 16/06/2020 12:42, Richard Sandiford wrote:

> [...]

> 2020-06-16  Richard Sandiford  <richard.sandiford@arm.com>

>

> gcc/

> 	* coretypes.h (first_type): New alias template.

> 	* recog.h (insn_gen_fn::operator()): Use it instead of a decltype.

> 	Remove spurious “...” and split the function type out into a typedef.

> ---

>   gcc/coretypes.h | 4 ++++

>   gcc/recog.h     | 5 +++--

>   2 files changed, 7 insertions(+), 2 deletions(-)

>

> diff --git a/gcc/coretypes.h b/gcc/coretypes.h

> index cda22697cc3..01ec2e23ce2 100644

> --- a/gcc/coretypes.h

> +++ b/gcc/coretypes.h

> @@ -359,6 +359,10 @@ struct kv_pair

>     const ValueType value;	/* the value of the name */

>   };

>   

> +/* Alias of the first type, ignoring the second.  */

> +template<typename T1, typename T2>

> +using first_type = T1;

> +

>   #else

>   

>   struct _dont_use_rtx_here_;

> diff --git a/gcc/recog.h b/gcc/recog.h

> index 0a71a02c4a9..d674d384723 100644

> --- a/gcc/recog.h

> +++ b/gcc/recog.h

> @@ -295,9 +295,10 @@ struct insn_gen_fn

>     typedef void (*stored_funcptr) (void);

>   

>     template<typename ...Ts>

> -  rtx_insn *operator() (Ts... args...) const

> +  rtx_insn *operator() (Ts... args) const

>     {

> -    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);

> +    typedef rtx_insn *(*funcptr) (first_type<rtx, Ts>...);

> +    return ((funcptr) func) (args...);

>     }

>   

>     // This is for compatibility of code that invokes functions like


I get this error on FreeBSD 12.1 with

c++ --version
FreeBSD clang version 8.0.1 (tags/RELEASE_801/final 366581) (based on 
LLVM 8.0.1)
Target: x86_64-unknown-freebsd12.1
Thread model: posix
InstalledDir: /usr/bin

In file included from ../../gnu-mirror-gcc-aff95ee/gcc/function.c:51:
../../gnu-mirror-gcc-aff95ee/gcc/recog.h:301:30: error: too many 
arguments to function call, expected 1, have 2
     return ((funcptr) func) (args...);
            ~~~~~~~~~~~~~~~~  ^~~~
../../gnu-mirror-gcc-aff95ee/gcc/function.c:3315:29: note: in 
instantiation of function template specialization 
'insn_gen_fn::operator()<rtx_def *, rtx_def *>' requested here
         emit_insn (GEN_FCN (icode) (parmreg, validated_mem));
Richard Sandiford June 18, 2020, 7:09 a.m. | #6
Sebastian Huber <sebastian.huber@embedded-brains.de> writes:
> On 16/06/2020 12:42, Richard Sandiford wrote:

>

>> [...]

>> 2020-06-16  Richard Sandiford  <richard.sandiford@arm.com>

>>

>> gcc/

>> 	* coretypes.h (first_type): New alias template.

>> 	* recog.h (insn_gen_fn::operator()): Use it instead of a decltype.

>> 	Remove spurious “...” and split the function type out into a typedef.

>> ---

>>   gcc/coretypes.h | 4 ++++

>>   gcc/recog.h     | 5 +++--

>>   2 files changed, 7 insertions(+), 2 deletions(-)

>>

>> diff --git a/gcc/coretypes.h b/gcc/coretypes.h

>> index cda22697cc3..01ec2e23ce2 100644

>> --- a/gcc/coretypes.h

>> +++ b/gcc/coretypes.h

>> @@ -359,6 +359,10 @@ struct kv_pair

>>     const ValueType value;	/* the value of the name */

>>   };

>>   

>> +/* Alias of the first type, ignoring the second.  */

>> +template<typename T1, typename T2>

>> +using first_type = T1;

>> +

>>   #else

>>   

>>   struct _dont_use_rtx_here_;

>> diff --git a/gcc/recog.h b/gcc/recog.h

>> index 0a71a02c4a9..d674d384723 100644

>> --- a/gcc/recog.h

>> +++ b/gcc/recog.h

>> @@ -295,9 +295,10 @@ struct insn_gen_fn

>>     typedef void (*stored_funcptr) (void);

>>   

>>     template<typename ...Ts>

>> -  rtx_insn *operator() (Ts... args...) const

>> +  rtx_insn *operator() (Ts... args) const

>>     {

>> -    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);

>> +    typedef rtx_insn *(*funcptr) (first_type<rtx, Ts>...);

>> +    return ((funcptr) func) (args...);

>>     }

>>   

>>     // This is for compatibility of code that invokes functions like

>

> I get this error on FreeBSD 12.1 with

>

> c++ --version

> FreeBSD clang version 8.0.1 (tags/RELEASE_801/final 366581) (based on 

> LLVM 8.0.1)

> Target: x86_64-unknown-freebsd12.1

> Thread model: posix

> InstalledDir: /usr/bin

>

> In file included from ../../gnu-mirror-gcc-aff95ee/gcc/function.c:51:

> ../../gnu-mirror-gcc-aff95ee/gcc/recog.h:301:30: error: too many 

> arguments to function call, expected 1, have 2

>      return ((funcptr) func) (args...);

>             ~~~~~~~~~~~~~~~~  ^~~~

> ../../gnu-mirror-gcc-aff95ee/gcc/function.c:3315:29: note: in 

> instantiation of function template specialization 

> 'insn_gen_fn::operator()<rtx_def *, rtx_def *>' requested here

>          emit_insn (GEN_FCN (icode) (parmreg, validated_mem));


Thanks for the report.  Was clang OK with the earlier version
(i.e. before 4e49b994de060d4a6c9318d0ed52ef038153426e)?

Richard
Sebastian Huber June 18, 2020, 7:14 a.m. | #7
On 18/06/2020 09:09, Richard Sandiford wrote:

> Sebastian Huber <sebastian.huber@embedded-brains.de> writes:

>> On 16/06/2020 12:42, Richard Sandiford wrote:

>>

>>> [...]

>>> 2020-06-16  Richard Sandiford  <richard.sandiford@arm.com>

>>>

>>> gcc/

>>> 	* coretypes.h (first_type): New alias template.

>>> 	* recog.h (insn_gen_fn::operator()): Use it instead of a decltype.

>>> 	Remove spurious “...” and split the function type out into a typedef.

>>> ---

>>>    gcc/coretypes.h | 4 ++++

>>>    gcc/recog.h     | 5 +++--

>>>    2 files changed, 7 insertions(+), 2 deletions(-)

>>>

>>> diff --git a/gcc/coretypes.h b/gcc/coretypes.h

>>> index cda22697cc3..01ec2e23ce2 100644

>>> --- a/gcc/coretypes.h

>>> +++ b/gcc/coretypes.h

>>> @@ -359,6 +359,10 @@ struct kv_pair

>>>      const ValueType value;	/* the value of the name */

>>>    };

>>>    

>>> +/* Alias of the first type, ignoring the second.  */

>>> +template<typename T1, typename T2>

>>> +using first_type = T1;

>>> +

>>>    #else

>>>    

>>>    struct _dont_use_rtx_here_;

>>> diff --git a/gcc/recog.h b/gcc/recog.h

>>> index 0a71a02c4a9..d674d384723 100644

>>> --- a/gcc/recog.h

>>> +++ b/gcc/recog.h

>>> @@ -295,9 +295,10 @@ struct insn_gen_fn

>>>      typedef void (*stored_funcptr) (void);

>>>    

>>>      template<typename ...Ts>

>>> -  rtx_insn *operator() (Ts... args...) const

>>> +  rtx_insn *operator() (Ts... args) const

>>>      {

>>> -    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);

>>> +    typedef rtx_insn *(*funcptr) (first_type<rtx, Ts>...);

>>> +    return ((funcptr) func) (args...);

>>>      }

>>>    

>>>      // This is for compatibility of code that invokes functions like

>> I get this error on FreeBSD 12.1 with

>>

>> c++ --version

>> FreeBSD clang version 8.0.1 (tags/RELEASE_801/final 366581) (based on

>> LLVM 8.0.1)

>> Target: x86_64-unknown-freebsd12.1

>> Thread model: posix

>> InstalledDir: /usr/bin

>>

>> In file included from ../../gnu-mirror-gcc-aff95ee/gcc/function.c:51:

>> ../../gnu-mirror-gcc-aff95ee/gcc/recog.h:301:30: error: too many

>> arguments to function call, expected 1, have 2

>>       return ((funcptr) func) (args...);

>>              ~~~~~~~~~~~~~~~~  ^~~~

>> ../../gnu-mirror-gcc-aff95ee/gcc/function.c:3315:29: note: in

>> instantiation of function template specialization

>> 'insn_gen_fn::operator()<rtx_def *, rtx_def *>' requested here

>>           emit_insn (GEN_FCN (icode) (parmreg, validated_mem));

> Thanks for the report.  Was clang OK with the earlier version

> (i.e. before 4e49b994de060d4a6c9318d0ed52ef038153426e)?

Yes, the last version I built successfully was 
b952c2cfcd74c284970e1b9bf1fca58f5f69ab23 on 10th of June. I build GCC 
roughly once per week.
Richard Sandiford June 18, 2020, 7:55 a.m. | #8
Sebastian Huber <sebastian.huber@embedded-brains.de> writes:
> On 18/06/2020 09:09, Richard Sandiford wrote:

>

>> Sebastian Huber <sebastian.huber@embedded-brains.de> writes:

>>> On 16/06/2020 12:42, Richard Sandiford wrote:

>>>

>>>> [...]

>>>> 2020-06-16  Richard Sandiford  <richard.sandiford@arm.com>

>>>>

>>>> gcc/

>>>> 	* coretypes.h (first_type): New alias template.

>>>> 	* recog.h (insn_gen_fn::operator()): Use it instead of a decltype.

>>>> 	Remove spurious “...” and split the function type out into a typedef.

>>>> ---

>>>>    gcc/coretypes.h | 4 ++++

>>>>    gcc/recog.h     | 5 +++--

>>>>    2 files changed, 7 insertions(+), 2 deletions(-)

>>>>

>>>> diff --git a/gcc/coretypes.h b/gcc/coretypes.h

>>>> index cda22697cc3..01ec2e23ce2 100644

>>>> --- a/gcc/coretypes.h

>>>> +++ b/gcc/coretypes.h

>>>> @@ -359,6 +359,10 @@ struct kv_pair

>>>>      const ValueType value;	/* the value of the name */

>>>>    };

>>>>    

>>>> +/* Alias of the first type, ignoring the second.  */

>>>> +template<typename T1, typename T2>

>>>> +using first_type = T1;

>>>> +

>>>>    #else

>>>>    

>>>>    struct _dont_use_rtx_here_;

>>>> diff --git a/gcc/recog.h b/gcc/recog.h

>>>> index 0a71a02c4a9..d674d384723 100644

>>>> --- a/gcc/recog.h

>>>> +++ b/gcc/recog.h

>>>> @@ -295,9 +295,10 @@ struct insn_gen_fn

>>>>      typedef void (*stored_funcptr) (void);

>>>>    

>>>>      template<typename ...Ts>

>>>> -  rtx_insn *operator() (Ts... args...) const

>>>> +  rtx_insn *operator() (Ts... args) const

>>>>      {

>>>> -    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);

>>>> +    typedef rtx_insn *(*funcptr) (first_type<rtx, Ts>...);

>>>> +    return ((funcptr) func) (args...);

>>>>      }

>>>>    

>>>>      // This is for compatibility of code that invokes functions like

>>> I get this error on FreeBSD 12.1 with

>>>

>>> c++ --version

>>> FreeBSD clang version 8.0.1 (tags/RELEASE_801/final 366581) (based on

>>> LLVM 8.0.1)

>>> Target: x86_64-unknown-freebsd12.1

>>> Thread model: posix

>>> InstalledDir: /usr/bin

>>>

>>> In file included from ../../gnu-mirror-gcc-aff95ee/gcc/function.c:51:

>>> ../../gnu-mirror-gcc-aff95ee/gcc/recog.h:301:30: error: too many

>>> arguments to function call, expected 1, have 2

>>>       return ((funcptr) func) (args...);

>>>              ~~~~~~~~~~~~~~~~  ^~~~

>>> ../../gnu-mirror-gcc-aff95ee/gcc/function.c:3315:29: note: in

>>> instantiation of function template specialization

>>> 'insn_gen_fn::operator()<rtx_def *, rtx_def *>' requested here

>>>           emit_insn (GEN_FCN (icode) (parmreg, validated_mem));

>> Thanks for the report.  Was clang OK with the earlier version

>> (i.e. before 4e49b994de060d4a6c9318d0ed52ef038153426e)?

> Yes, the last version I built successfully was 

> b952c2cfcd74c284970e1b9bf1fca58f5f69ab23 on 10th of June. I build GCC 

> roughly once per week.


Hmm, yeah, can reproduce with a recentish build from clang master:

template<typename T> using int_t = int;
void (*f)();
template<typename ...Ts>
void g(Ts... args) {
#if A
  ((void (*)(int_t<Ts>...)) f)(args...);
#elif B
  typedef void (*funcptr)(decltype((void)args, 0)...);
  ((funcptr) f)(args...);
#else
  typedef void (*funcptr)(int_t<Ts>...);
  ((funcptr) f)(args...);
#endif
}
void h(int x, int y) { g(x, y); }

Works with -DA and -DB, but fails with the final version.

So it looks like the obvious choices are:

(1) Remove the typedef.
(2) Go back to using decltype.
(3) Revert the whole thing and go back to the overloads.

Not sure about (1), because it leads to awkward formatting, and would
need a comment to say “Don't use a typedef!”.  The above error shows
that we're probably close to the edge of what compilers will support
reliably.

(2) seems to work more reliably, and gives nicer error messages,
but was unpopular for being unidiomatic C++.

I guess at this point, (3) is the way to go.

Thanks,
Richard
Kees Cook via Gcc-patches June 18, 2020, 9:02 a.m. | #9
On 18/06/20 08:55 +0100, Richard Sandiford wrote:
>Sebastian Huber <sebastian.huber@embedded-brains.de> writes:

>> On 18/06/2020 09:09, Richard Sandiford wrote:

>>

>>> Sebastian Huber <sebastian.huber@embedded-brains.de> writes:

>>>> On 16/06/2020 12:42, Richard Sandiford wrote:

>>>>

>>>>> [...]

>>>>> 2020-06-16  Richard Sandiford  <richard.sandiford@arm.com>

>>>>>

>>>>> gcc/

>>>>> 	* coretypes.h (first_type): New alias template.

>>>>> 	* recog.h (insn_gen_fn::operator()): Use it instead of a decltype.

>>>>> 	Remove spurious “...” and split the function type out into a typedef.

>>>>> ---

>>>>>    gcc/coretypes.h | 4 ++++

>>>>>    gcc/recog.h     | 5 +++--

>>>>>    2 files changed, 7 insertions(+), 2 deletions(-)

>>>>>

>>>>> diff --git a/gcc/coretypes.h b/gcc/coretypes.h

>>>>> index cda22697cc3..01ec2e23ce2 100644

>>>>> --- a/gcc/coretypes.h

>>>>> +++ b/gcc/coretypes.h

>>>>> @@ -359,6 +359,10 @@ struct kv_pair

>>>>>      const ValueType value;	/* the value of the name */

>>>>>    };

>>>>>

>>>>> +/* Alias of the first type, ignoring the second.  */

>>>>> +template<typename T1, typename T2>

>>>>> +using first_type = T1;

>>>>> +

>>>>>    #else

>>>>>

>>>>>    struct _dont_use_rtx_here_;

>>>>> diff --git a/gcc/recog.h b/gcc/recog.h

>>>>> index 0a71a02c4a9..d674d384723 100644

>>>>> --- a/gcc/recog.h

>>>>> +++ b/gcc/recog.h

>>>>> @@ -295,9 +295,10 @@ struct insn_gen_fn

>>>>>      typedef void (*stored_funcptr) (void);

>>>>>

>>>>>      template<typename ...Ts>

>>>>> -  rtx_insn *operator() (Ts... args...) const

>>>>> +  rtx_insn *operator() (Ts... args) const

>>>>>      {

>>>>> -    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);

>>>>> +    typedef rtx_insn *(*funcptr) (first_type<rtx, Ts>...);

>>>>> +    return ((funcptr) func) (args...);

>>>>>      }

>>>>>

>>>>>      // This is for compatibility of code that invokes functions like

>>>> I get this error on FreeBSD 12.1 with

>>>>

>>>> c++ --version

>>>> FreeBSD clang version 8.0.1 (tags/RELEASE_801/final 366581) (based on

>>>> LLVM 8.0.1)

>>>> Target: x86_64-unknown-freebsd12.1

>>>> Thread model: posix

>>>> InstalledDir: /usr/bin

>>>>

>>>> In file included from ../../gnu-mirror-gcc-aff95ee/gcc/function.c:51:

>>>> ../../gnu-mirror-gcc-aff95ee/gcc/recog.h:301:30: error: too many

>>>> arguments to function call, expected 1, have 2

>>>>       return ((funcptr) func) (args...);

>>>>              ~~~~~~~~~~~~~~~~  ^~~~

>>>> ../../gnu-mirror-gcc-aff95ee/gcc/function.c:3315:29: note: in

>>>> instantiation of function template specialization

>>>> 'insn_gen_fn::operator()<rtx_def *, rtx_def *>' requested here

>>>>           emit_insn (GEN_FCN (icode) (parmreg, validated_mem));

>>> Thanks for the report.  Was clang OK with the earlier version

>>> (i.e. before 4e49b994de060d4a6c9318d0ed52ef038153426e)?

>> Yes, the last version I built successfully was

>> b952c2cfcd74c284970e1b9bf1fca58f5f69ab23 on 10th of June. I build GCC

>> roughly once per week.

>

>Hmm, yeah, can reproduce with a recentish build from clang master:

>

>template<typename T> using int_t = int;

>void (*f)();

>template<typename ...Ts>

>void g(Ts... args) {

>#if A

>  ((void (*)(int_t<Ts>...)) f)(args...);

>#elif B

>  typedef void (*funcptr)(decltype((void)args, 0)...);

>  ((funcptr) f)(args...);

>#else

>  typedef void (*funcptr)(int_t<Ts>...);

>  ((funcptr) f)(args...);

>#endif

>}

>void h(int x, int y) { g(x, y); }

>

>Works with -DA and -DB, but fails with the final version.

>

>So it looks like the obvious choices are:

>

>(1) Remove the typedef.

>(2) Go back to using decltype.

>(3) Revert the whole thing and go back to the overloads.

>

>Not sure about (1), because it leads to awkward formatting, and would

>need a comment to say “Don't use a typedef!”.  The above error shows

>that we're probably close to the edge of what compilers will support

>reliably.


That's extremely surprising. It's basic C++11 and so Clang should have
been able to do it 10+ years ago. I'll look into it.

>(2) seems to work more reliably, and gives nicer error messages,

>but was unpopular for being unidiomatic C++.

>

>I guess at this point, (3) is the way to go.


Yuck, (2) is better than (3).

I'm not massively fond of (2) but (3) is horrible and just unnecessary
with variadics.
Kees Cook via Gcc-patches June 18, 2020, 9:28 a.m. | #10
On 18/06/20 10:02 +0100, Jonathan Wakely wrote:
>On 18/06/20 08:55 +0100, Richard Sandiford wrote:

>>Sebastian Huber <sebastian.huber@embedded-brains.de> writes:

>>>On 18/06/2020 09:09, Richard Sandiford wrote:

>>>

>>>>Sebastian Huber <sebastian.huber@embedded-brains.de> writes:

>>>>>On 16/06/2020 12:42, Richard Sandiford wrote:

>>>>>

>>>>>>[...]

>>>>>>2020-06-16  Richard Sandiford  <richard.sandiford@arm.com>

>>>>>>

>>>>>>gcc/

>>>>>>	* coretypes.h (first_type): New alias template.

>>>>>>	* recog.h (insn_gen_fn::operator()): Use it instead of a decltype.

>>>>>>	Remove spurious “...” and split the function type out into a typedef.

>>>>>>---

>>>>>>   gcc/coretypes.h | 4 ++++

>>>>>>   gcc/recog.h     | 5 +++--

>>>>>>   2 files changed, 7 insertions(+), 2 deletions(-)

>>>>>>

>>>>>>diff --git a/gcc/coretypes.h b/gcc/coretypes.h

>>>>>>index cda22697cc3..01ec2e23ce2 100644

>>>>>>--- a/gcc/coretypes.h

>>>>>>+++ b/gcc/coretypes.h

>>>>>>@@ -359,6 +359,10 @@ struct kv_pair

>>>>>>     const ValueType value;	/* the value of the name */

>>>>>>   };

>>>>>>

>>>>>>+/* Alias of the first type, ignoring the second.  */

>>>>>>+template<typename T1, typename T2>

>>>>>>+using first_type = T1;

>>>>>>+

>>>>>>   #else

>>>>>>

>>>>>>   struct _dont_use_rtx_here_;

>>>>>>diff --git a/gcc/recog.h b/gcc/recog.h

>>>>>>index 0a71a02c4a9..d674d384723 100644

>>>>>>--- a/gcc/recog.h

>>>>>>+++ b/gcc/recog.h

>>>>>>@@ -295,9 +295,10 @@ struct insn_gen_fn

>>>>>>     typedef void (*stored_funcptr) (void);

>>>>>>

>>>>>>     template<typename ...Ts>

>>>>>>-  rtx_insn *operator() (Ts... args...) const

>>>>>>+  rtx_insn *operator() (Ts... args) const

>>>>>>     {

>>>>>>-    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);

>>>>>>+    typedef rtx_insn *(*funcptr) (first_type<rtx, Ts>...);

>>>>>>+    return ((funcptr) func) (args...);

>>>>>>     }

>>>>>>

>>>>>>     // This is for compatibility of code that invokes functions like

>>>>>I get this error on FreeBSD 12.1 with

>>>>>

>>>>>c++ --version

>>>>>FreeBSD clang version 8.0.1 (tags/RELEASE_801/final 366581) (based on

>>>>>LLVM 8.0.1)

>>>>>Target: x86_64-unknown-freebsd12.1

>>>>>Thread model: posix

>>>>>InstalledDir: /usr/bin

>>>>>

>>>>>In file included from ../../gnu-mirror-gcc-aff95ee/gcc/function.c:51:

>>>>>../../gnu-mirror-gcc-aff95ee/gcc/recog.h:301:30: error: too many

>>>>>arguments to function call, expected 1, have 2

>>>>>      return ((funcptr) func) (args...);

>>>>>             ~~~~~~~~~~~~~~~~  ^~~~

>>>>>../../gnu-mirror-gcc-aff95ee/gcc/function.c:3315:29: note: in

>>>>>instantiation of function template specialization

>>>>>'insn_gen_fn::operator()<rtx_def *, rtx_def *>' requested here

>>>>>          emit_insn (GEN_FCN (icode) (parmreg, validated_mem));

>>>>Thanks for the report.  Was clang OK with the earlier version

>>>>(i.e. before 4e49b994de060d4a6c9318d0ed52ef038153426e)?

>>>Yes, the last version I built successfully was

>>>b952c2cfcd74c284970e1b9bf1fca58f5f69ab23 on 10th of June. I build GCC

>>>roughly once per week.

>>

>>Hmm, yeah, can reproduce with a recentish build from clang master:

>>

>>template<typename T> using int_t = int;

>>void (*f)();

>>template<typename ...Ts>

>>void g(Ts... args) {

>>#if A

>> ((void (*)(int_t<Ts>...)) f)(args...);

>>#elif B

>> typedef void (*funcptr)(decltype((void)args, 0)...);

>> ((funcptr) f)(args...);

>>#else

>> typedef void (*funcptr)(int_t<Ts>...);

>> ((funcptr) f)(args...);

>>#endif

>>}

>>void h(int x, int y) { g(x, y); }

>>

>>Works with -DA and -DB, but fails with the final version.

>>

>>So it looks like the obvious choices are:

>>

>>(1) Remove the typedef.

>>(2) Go back to using decltype.

>>(3) Revert the whole thing and go back to the overloads.

>>

>>Not sure about (1), because it leads to awkward formatting, and would

>>need a comment to say “Don't use a typedef!”.  The above error shows

>>that we're probably close to the edge of what compilers will support

>>reliably.

>

>That's extremely surprising. It's basic C++11 and so Clang should have

>been able to do it 10+ years ago. I'll look into it.

>

>>(2) seems to work more reliably, and gives nicer error messages,

>>but was unpopular for being unidiomatic C++.

>>

>>I guess at this point, (3) is the way to go.

>

>Yuck, (2) is better than (3).

>

>I'm not massively fond of (2) but (3) is horrible and just unnecessary

>with variadics.


I reported https://bugs.llvm.org/show_bug.cgi?id=46377 because it
doesn't seem to be known (which is almost as surprising as the fact it
fails in the first place). It used to work with Clang 3.0 back in 2011
but fails with every release since.

I thought I'd seen it before, but I was thinking of
https://bugs.llvm.org/show_bug.cgi?id=14858 which got fixed years ago.
Gerald Pfeifer June 21, 2020, 11:36 p.m. | #11
On Thu, 18 Jun 2020, Sebastian Huber wrote:
> b952c2cfcd74c284970e1b9bf1fca58f5f69ab23 on 10th of June. I build GCC 

> roughly once per week.


I have daily builds running on i386-unknown-freebsd11.3 (since this is
the flavor most likely to trigger issues) and the failure started in the 
24 hours leading up to 2020-06-18-16:40UTC.

clang 8.0.1 is the system compiler on FreeBSD 11, so we need a way for
that to work I'm afraid.

I filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95805 and included
the relevant part of the build log.

Gerald
Richard Sandiford June 22, 2020, 7:18 p.m. | #12
Gerald Pfeifer <gerald@pfeifer.com> writes:
> On Thu, 18 Jun 2020, Sebastian Huber wrote:

>> b952c2cfcd74c284970e1b9bf1fca58f5f69ab23 on 10th of June. I build GCC 

>> roughly once per week.

>

> I have daily builds running on i386-unknown-freebsd11.3 (since this is

> the flavor most likely to trigger issues) and the failure started in the 

> 24 hours leading up to 2020-06-18-16:40UTC.

>

> clang 8.0.1 is the system compiler on FreeBSD 11, so we need a way for

> that to work I'm afraid.


OK, I've applied the below as (hopefully) obvious after testing
on aarch64-linux-gnu.

> I filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95805 and included

> the relevant part of the build log.


But I forgot about the PR, sorry, so didn't add it to the commit message.

Richard
From c7a2a7d71ccc8a8c10867ef7027be5b617576c1d Mon Sep 17 00:00:00 2001
From: Richard Sandiford <richard.sandiford@arm.com>

Date: Mon, 22 Jun 2020 14:06:00 +0100
Subject: [PATCH] recog: Restore builds with Clang

Using parameter packs with function typedefs tripped a Clang bug
in which the packs were not being expanded correctly:

  https://bugs.llvm.org/show_bug.cgi?id=46377

Work around that by going back to the decltype approach, but adding
a cast to void to suppress a warning about unused values.

2020-06-22  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* coretypes.h (first_type): Delete.
	* recog.h (insn_gen_fn::operator()): Go back to using a decltype.
---
 gcc/coretypes.h | 4 ----
 gcc/recog.h     | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/gcc/coretypes.h b/gcc/coretypes.h
index 720f9f9c63f..6b6cfcdf210 100644
--- a/gcc/coretypes.h
+++ b/gcc/coretypes.h
@@ -359,10 +359,6 @@ struct kv_pair
   const ValueType value;	/* the value of the name */
 };
 
-/* Alias of the first type, ignoring the second.  */
-template<typename T1, typename T2>
-using first_type = T1;
-
 /* Iterator pair used for a collection iteration with range-based loops.  */
 
 template<typename T>
diff --git a/gcc/recog.h b/gcc/recog.h
index d674d384723..3e4b55bdf3f 100644
--- a/gcc/recog.h
+++ b/gcc/recog.h
@@ -297,7 +297,7 @@ struct insn_gen_fn
   template<typename ...Ts>
   rtx_insn *operator() (Ts... args) const
   {
-    typedef rtx_insn *(*funcptr) (first_type<rtx, Ts>...);
+    typedef rtx_insn *(*funcptr) (decltype ((void) args, NULL_RTX)...);
     return ((funcptr) func) (args...);
   }
 
-- 
2.17.1
Gerald Pfeifer June 23, 2020, 10:50 p.m. | #13
On Mon, 22 Jun 2020, Richard Sandiford wrote:
> OK, I've applied the below as (hopefully) obvious after testing

> on aarch64-linux-gnu.

> 

>> I filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95805 and included

>> the relevant part of the build log.

> 

> But I forgot about the PR, sorry, so didn't add it to the commit message.


No worries.  And thank you, I am happy to report those testers are
back in bootstrap land!

Gerald

Patch

diff --git a/gcc/recog.h b/gcc/recog.h
index 17c09fdba3b..0a71a02c4a9 100644
--- a/gcc/recog.h
+++ b/gcc/recog.h
@@ -292,43 +292,13 @@  typedef const char * (*insn_output_fn) (rtx *, rtx_insn *);
 
 struct insn_gen_fn
 {
-  typedef rtx_insn * (*f0) (void);
-  typedef rtx_insn * (*f1) (rtx);
-  typedef rtx_insn * (*f2) (rtx, rtx);
-  typedef rtx_insn * (*f3) (rtx, rtx, rtx);
-  typedef rtx_insn * (*f4) (rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f5) (rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f6) (rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f7) (rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f8) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f9) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f10) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f11) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f12) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f13) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f14) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f15) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f16) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-
   typedef void (*stored_funcptr) (void);
 
-  rtx_insn * operator () (void) const { return ((f0)func) (); }
-  rtx_insn * operator () (rtx a0) const { return ((f1)func) (a0); }
-  rtx_insn * operator () (rtx a0, rtx a1) const { return ((f2)func) (a0, a1); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2) const { return ((f3)func) (a0, a1, a2); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3) const { return ((f4)func) (a0, a1, a2, a3); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4) const { return ((f5)func) (a0, a1, a2, a3, a4); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5) const { return ((f6)func) (a0, a1, a2, a3, a4, a5); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6) const { return ((f7)func) (a0, a1, a2, a3, a4, a5, a6); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7) const { return ((f8)func) (a0, a1, a2, a3, a4, a5, a6, a7); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8) const { return ((f9)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9) const { return ((f10)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10) const { return ((f11)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11) const { return ((f12)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11, rtx a12) const { return ((f13)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11, rtx a12, rtx a13) const { return ((f14)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11, rtx a12, rtx a13, rtx a14) const { return ((f15)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11, rtx a12, rtx a13, rtx a14, rtx a15) const { return ((f16)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14, a15); }
+  template<typename ...Ts>
+  rtx_insn *operator() (Ts... args...) const
+  {
+    return ((rtx_insn *(*) (decltype(args, NULL_RTX)...)) func) (args...);
+  }
 
   // This is for compatibility of code that invokes functions like
   //   (*funcptr) (arg)