diff mbox series

[v2] coroutines: Copy attributes to the outlined functions [PR95518, PR95813]

Message ID BCD99369-CFA8-4129-A030-997BC4987661@sandoe.co.uk
State New
Headers show
Series [v2] coroutines: Copy attributes to the outlined functions [PR95518, PR95813] | expand

Commit Message

Iain Sandoe June 24, 2020, 11 a.m. UTC 
Hi Nathan,

Nathan Sidwell <nathan@acm.org> wrote:

> On 6/11/20 3:53 PM, Iain Sandoe wrote:
>> Hi
>> We had omitted the copying of function attributes (including the
>> 'used' status).  Mark the outlined functions as artificial, since
>> they are; some diagnostic processing tests this.
> 
> Do we do the right thing for say attribute((section("bob"))?  

I’ve made sure of that in the attached patch (also user-defined alignment).

> what if the user tries attribute((alias("bob")),
> presumable we don't want both ramp and actor to alias bob?  I think this might be tricky.

As we discussed off-list, there might be some attributes that are inappropiate for coroutines.
We can diagnose those (when we understand what they are) when building the ramp / splitting the actor out (as a separate patch, when it becomes required; i.e. not part of this patch).

The attached patch copies all attributes from the original function (and those that are already on the decl) onto the outlined ones - this fixes 95518 and 95813 (which is almost a dup).

OK now for master / 10.2?
thanks,
Iain

 coroutines: Copy attributes to the outlined functions
 [PR95518,PR95813]

We had omitted the copying of function attributes, we now copy
the used, alignment, section values from the original decal and
the complete set of function attributes.  It is likely that some
function attributes don't really make sense for coroutines, but
that can be disgnosed separately.
Also mark the outlined functions as artificial, since they are; and
some diagnostic processing tests this.

gcc/cp/ChangeLog:

	PR c++/95518
	PR c++/95813
	* coroutines.cc (act_des_fn): Copy function
	attributes onto the outlined coroutine helpers.

gcc/testsuite/ChangeLog:

	PR c++/95518
	PR c++/95813
	* g++.dg/coroutines/pr95518.C: New test.
        * g++.dg/coroutines/pr95813.C: New test.
---
 gcc/cp/coroutines.cc                      | 12 ++++++
 gcc/testsuite/g++.dg/coroutines/pr95518.C | 28 ++++++++++++++
 gcc/testsuite/g++.dg/coroutines/pr95813.C | 46 +++++++++++++++++++++++
 3 files changed, 86 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95518.C
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95813.C

Comments

Nathan Sidwell June 24, 2020, 1:35 p.m. UTC | #1 
On 6/24/20 7:00 AM, Iain Sandoe wrote:
> Hi Nathan,
> 
> Nathan Sidwell <nathan@acm.org> wrote:
> 
>> On 6/11/20 3:53 PM, Iain Sandoe wrote:
>>> Hi
>>> We had omitted the copying of function attributes (including the
>>> 'used' status).  Mark the outlined functions as artificial, since
>>> they are; some diagnostic processing tests this.
>>
>> Do we do the right thing for say attribute((section("bob"))?
> 
> I’ve made sure of that in the attached patch (also user-defined alignment).
> 
>> what if the user tries attribute((alias("bob")),
>> presumable we don't want both ramp and actor to alias bob?  I think this might be tricky.
> 
> As we discussed off-list, there might be some attributes that are inappropiate for coroutines.
> We can diagnose those (when we understand what they are) when building the ramp / splitting the actor out (as a separate patch, when it becomes required; i.e. not part of this patch).
> 
> The attached patch copies all attributes from the original function (and those that are already on the decl) onto the outlined ones - this fixes 95518 and 95813 (which is almost a dup).
> 
> OK now for master / 10.2?

ok, thanks for checking!

> thanks,
> Iain
> 
>   coroutines: Copy attributes to the outlined functions
>   [PR95518,PR95813]
> 
> We had omitted the copying of function attributes, we now copy
> the used, alignment, section values from the original decal and
> the complete set of function attributes.  It is likely that some
> function attributes don't really make sense for coroutines, but
> that can be disgnosed separately.
> Also mark the outlined functions as artificial, since they are; and
> some diagnostic processing tests this.
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/95518
> 	PR c++/95813
> 	* coroutines.cc (act_des_fn): Copy function
> 	attributes onto the outlined coroutine helpers.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/95518
> 	PR c++/95813
> 	* g++.dg/coroutines/pr95518.C: New test.
>          * g++.dg/coroutines/pr95813.C: New test.
> ---
>   gcc/cp/coroutines.cc                      | 12 ++++++
>   gcc/testsuite/g++.dg/coroutines/pr95518.C | 28 ++++++++++++++
>   gcc/testsuite/g++.dg/coroutines/pr95813.C | 46 +++++++++++++++++++++++
>   3 files changed, 86 insertions(+)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95518.C
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/pr95813.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 9cdb0c591d5..64b97535c8d 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -3530,12 +3530,24 @@ act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name)
>     tree fn_name = get_fn_local_identifier (orig, name);
>     tree fn = build_lang_decl (FUNCTION_DECL, fn_name, fn_type);
>     DECL_CONTEXT (fn) = DECL_CONTEXT (orig);
> +  DECL_ARTIFICIAL (fn) = true;
>     DECL_INITIAL (fn) = error_mark_node;
>     tree id = get_identifier ("frame_ptr");
>     tree fp = build_lang_decl (PARM_DECL, id, coro_frame_ptr);
>     DECL_CONTEXT (fp) = fn;
>     DECL_ARG_TYPE (fp) = type_passed_as (coro_frame_ptr);
>     DECL_ARGUMENTS (fn) = fp;
> +  /* Copy selected attributes from the original function.  */
> +  TREE_USED (fn) = TREE_USED (orig);
> +  if (DECL_SECTION_NAME (orig))
> +    set_decl_section_name (fn, DECL_SECTION_NAME (orig));
> +  /* Copy any alignment that the FE added.  */
> +  if (DECL_ALIGN (orig))
> +    SET_DECL_ALIGN (fn, DECL_ALIGN (orig));
> +  /* Copy any alignment the user added.  */
> +  DECL_USER_ALIGN (fn) = DECL_USER_ALIGN (orig);
> +  /* Apply attributes from the original fn.  */
> +  DECL_ATTRIBUTES (fn) = copy_list (DECL_ATTRIBUTES (orig));
>     return fn;
>   }
>   
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr95518.C b/gcc/testsuite/g++.dg/coroutines/pr95518.C
> new file mode 100644
> index 00000000000..b1717677810
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr95518.C
> @@ -0,0 +1,28 @@
> +// { dg-additional-options "-O -Wunused-function" }
> +
> +#if __has_include (<coroutine>)
> +#include <coroutine>
> +using namespace std;
> +#elif defined (__clang__) && __has_include (<experimental/coroutine>)
> +#include <experimental/coroutine>
> +namespace std { using namespace experimental; }
> +#endif
> +
> +struct dummy
> +{
> +    struct promise_type
> +    {
> +        dummy get_return_object() const noexcept { return {}; }
> +        std::suspend_never initial_suspend() const noexcept { return {}; }
> +        std::suspend_never final_suspend() const noexcept { return {}; }
> +        void return_void() const noexcept {}
> +        void unhandled_exception() const noexcept {}
> +    };
> +};
> +
> +// This checks that the attribute is passed on to the outlined coroutine
> +// functions (so that there should be no diagnostic).
> +[[maybe_unused]] static dummy foo()
> +{
> +    co_return;
> +}
> diff --git a/gcc/testsuite/g++.dg/coroutines/pr95813.C b/gcc/testsuite/g++.dg/coroutines/pr95813.C
> new file mode 100644
> index 00000000000..445cdf1f7ef
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/pr95813.C
> @@ -0,0 +1,46 @@
> +//  { dg-additional-options  "-Wall -O" }
> +
> +// This should complete without any diagnostic.
> +
> +#include <coroutine>
> +#include <exception>
> +
> +template <typename T>
> +class lazy {
> +    T _v = 0;
> +public:
> +    lazy() {}
> +    bool await_ready() {return true;}
> +    void await_suspend(auto x) noexcept {}
> +    T await_resume() { return _v; }
> +};
> +
> +namespace std {
> +
> +template <typename T, typename... Args>
> +struct coroutine_traits<lazy<T>, Args...> {
> +    struct promise_type {
> +        suspend_always initial_suspend() const { return {}; }
> +        suspend_always final_suspend() const { return {}; }
> +        void return_value(T val) {}
> +        lazy<T> get_return_object() {
> +            return lazy<T>();
> +        }
> +        void unhandled_exception() {
> +            std::terminate();
> +        }
> +    };
> +};
> +}
> +
> +struct xxx {
> +    static lazy<int> func() {
> +        co_return 1;
> +    }
> +};
> +
> +#if 0
> +lazy<int> foo() {
> +    co_return co_await xxx::func();
> +}
> +#endif
>
diff mbox series

Patch

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 9cdb0c591d5..64b97535c8d 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -3530,12 +3530,24 @@  act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name)
   tree fn_name = get_fn_local_identifier (orig, name);
   tree fn = build_lang_decl (FUNCTION_DECL, fn_name, fn_type);
   DECL_CONTEXT (fn) = DECL_CONTEXT (orig);
+  DECL_ARTIFICIAL (fn) = true;
   DECL_INITIAL (fn) = error_mark_node;
   tree id = get_identifier ("frame_ptr");
   tree fp = build_lang_decl (PARM_DECL, id, coro_frame_ptr);
   DECL_CONTEXT (fp) = fn;
   DECL_ARG_TYPE (fp) = type_passed_as (coro_frame_ptr);
   DECL_ARGUMENTS (fn) = fp;
+  /* Copy selected attributes from the original function.  */
+  TREE_USED (fn) = TREE_USED (orig);
+  if (DECL_SECTION_NAME (orig))
+    set_decl_section_name (fn, DECL_SECTION_NAME (orig));
+  /* Copy any alignment that the FE added.  */
+  if (DECL_ALIGN (orig))
+    SET_DECL_ALIGN (fn, DECL_ALIGN (orig));
+  /* Copy any alignment the user added.  */
+  DECL_USER_ALIGN (fn) = DECL_USER_ALIGN (orig);
+  /* Apply attributes from the original fn.  */
+  DECL_ATTRIBUTES (fn) = copy_list (DECL_ATTRIBUTES (orig));
   return fn;
 }
 
diff --git a/gcc/testsuite/g++.dg/coroutines/pr95518.C b/gcc/testsuite/g++.dg/coroutines/pr95518.C
new file mode 100644
index 00000000000..b1717677810
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr95518.C
@@ -0,0 +1,28 @@ 
+// { dg-additional-options "-O -Wunused-function" }
+
+#if __has_include (<coroutine>)
+#include <coroutine>
+using namespace std;
+#elif defined (__clang__) && __has_include (<experimental/coroutine>)
+#include <experimental/coroutine>
+namespace std { using namespace experimental; }
+#endif
+
+struct dummy
+{
+    struct promise_type
+    {
+        dummy get_return_object() const noexcept { return {}; }
+        std::suspend_never initial_suspend() const noexcept { return {}; }
+        std::suspend_never final_suspend() const noexcept { return {}; }
+        void return_void() const noexcept {}
+        void unhandled_exception() const noexcept {}
+    };
+};
+
+// This checks that the attribute is passed on to the outlined coroutine
+// functions (so that there should be no diagnostic).
+[[maybe_unused]] static dummy foo()
+{ 
+    co_return;
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/pr95813.C b/gcc/testsuite/g++.dg/coroutines/pr95813.C
new file mode 100644
index 00000000000..445cdf1f7ef
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr95813.C
@@ -0,0 +1,46 @@ 
+//  { dg-additional-options  "-Wall -O" }
+
+// This should complete without any diagnostic.
+
+#include <coroutine>
+#include <exception>
+
+template <typename T>
+class lazy {
+    T _v = 0;
+public:
+    lazy() {}
+    bool await_ready() {return true;}
+    void await_suspend(auto x) noexcept {}
+    T await_resume() { return _v; }
+};
+
+namespace std {
+
+template <typename T, typename... Args>
+struct coroutine_traits<lazy<T>, Args...> {
+    struct promise_type {
+        suspend_always initial_suspend() const { return {}; }
+        suspend_always final_suspend() const { return {}; }
+        void return_value(T val) {}
+        lazy<T> get_return_object() {
+            return lazy<T>();
+        }
+        void unhandled_exception() {
+            std::terminate();
+        }
+    };
+};
+}
+
+struct xxx {
+    static lazy<int> func() {
+        co_return 1;
+    }
+};
+
+#if 0
+lazy<int> foo() {
+    co_return co_await xxx::func();
+}
+#endif