Re: PING^3: [PATCH] i386: Don't generate alias for function return thunk

[H.J. Lu]

On Thu, Mar 15, 2018 at 12:54 PM, Jan Hubicka  wrote:
>> On Thu, Mar 15, 2018 at 10:47 AM, Jan Hubicka  wrote:
>> >> On Thu, Mar 15, 2018 at 9:03 AM, Jan Hubicka  wrote:
>> >> >> > What is the reason for using different names for return and indirect 
>> >> >> > thunks at first place?
>> >> >> >
>> >> >>
>> >> >> These 2 thunks are identical.  But one may want to provide an
>> >> >> alternate thunk only for
>> >> >> indirect branch and leaves return thunk alone.  You can't do that if
>> >> >> both have the same
>> >> >> name.
>> >> >
>> >> > Hmm, OK, what is the benefit to have two different thunks? It is just
>> >> > safety precaution so we could adjust one without adjusting the other in
>> >> > future?
>> >> >
>> >>
>> >> That is correct.
>> >
>> > Hmm, I guess the patch is OK. Things are slightly more flexible this way 
>> > and
>> > duplicating thunk is not terribly expensive. One can always link with
>> > non-comdat+ alias.
>> >
>>
>> That is true.  OK to backport to GCC 7 after a few days?
> OK.  I suppose you are testing return thunks on some real environment, like 
> GCC bootstrap :)

Yes.

-- 
H.J.

Re: PING^3: [PATCH] i386: Don't generate alias for function return thunk

[Jan Hubicka]

> On Thu, Mar 15, 2018 at 10:47 AM, Jan Hubicka  wrote:
> >> On Thu, Mar 15, 2018 at 9:03 AM, Jan Hubicka  wrote:
> >> >> > What is the reason for using different names for return and indirect 
> >> >> > thunks at first place?
> >> >> >
> >> >>
> >> >> These 2 thunks are identical.  But one may want to provide an
> >> >> alternate thunk only for
> >> >> indirect branch and leaves return thunk alone.  You can't do that if
> >> >> both have the same
> >> >> name.
> >> >
> >> > Hmm, OK, what is the benefit to have two different thunks? It is just
> >> > safety precaution so we could adjust one without adjusting the other in
> >> > future?
> >> >
> >>
> >> That is correct.
> >
> > Hmm, I guess the patch is OK. Things are slightly more flexible this way and
> > duplicating thunk is not terribly expensive. One can always link with
> > non-comdat+ alias.
> >
> 
> That is true.  OK to backport to GCC 7 after a few days?
OK.  I suppose you are testing return thunks on some real environment, like GCC 
bootstrap :)
Honza
> 
> Thanks.
> 
> -- 
> H.J.

Re: PING^3: [PATCH] i386: Don't generate alias for function return thunk

[Jan Hubicka]

> On Thu, Mar 15, 2018 at 10:47 AM, Jan Hubicka  wrote:
> >> On Thu, Mar 15, 2018 at 9:03 AM, Jan Hubicka  wrote:
> >> >> > What is the reason for using different names for return and indirect 
> >> >> > thunks at first place?
> >> >> >
> >> >>
> >> >> These 2 thunks are identical.  But one may want to provide an
> >> >> alternate thunk only for
> >> >> indirect branch and leaves return thunk alone.  You can't do that if
> >> >> both have the same
> >> >> name.
> >> >
> >> > Hmm, OK, what is the benefit to have two different thunks? It is just
> >> > safety precaution so we could adjust one without adjusting the other in
> >> > future?
> >> >
> >>
> >> That is correct.
> >
> > Hmm, I guess the patch is OK. Things are slightly more flexible this way and
> > duplicating thunk is not terribly expensive. One can always link with
> > non-comdat+ alias.
> >
> 
> That is true.  OK to backport to GCC 7 after a few days?

OK.
Honza
> 
> Thanks.
> 
> -- 
> H.J.

Re: PING^3: [PATCH] i386: Don't generate alias for function return thunk

[H.J. Lu]

On Thu, Mar 15, 2018 at 10:47 AM, Jan Hubicka  wrote:
>> On Thu, Mar 15, 2018 at 9:03 AM, Jan Hubicka  wrote:
>> >> > What is the reason for using different names for return and indirect 
>> >> > thunks at first place?
>> >> >
>> >>
>> >> These 2 thunks are identical.  But one may want to provide an
>> >> alternate thunk only for
>> >> indirect branch and leaves return thunk alone.  You can't do that if
>> >> both have the same
>> >> name.
>> >
>> > Hmm, OK, what is the benefit to have two different thunks? It is just
>> > safety precaution so we could adjust one without adjusting the other in
>> > future?
>> >
>>
>> That is correct.
>
> Hmm, I guess the patch is OK. Things are slightly more flexible this way and
> duplicating thunk is not terribly expensive. One can always link with
> non-comdat+ alias.
>

That is true.  OK to backport to GCC 7 after a few days?

Thanks.

-- 
H.J.

Re: PING^3: [PATCH] i386: Don't generate alias for function return thunk

[Jan Hubicka]

> On Thu, Mar 15, 2018 at 9:03 AM, Jan Hubicka  wrote:
> >> > What is the reason for using different names for return and indirect 
> >> > thunks at first place?
> >> >
> >>
> >> These 2 thunks are identical.  But one may want to provide an
> >> alternate thunk only for
> >> indirect branch and leaves return thunk alone.  You can't do that if
> >> both have the same
> >> name.
> >
> > Hmm, OK, what is the benefit to have two different thunks? It is just
> > safety precaution so we could adjust one without adjusting the other in
> > future?
> >
> 
> That is correct.

Hmm, I guess the patch is OK. Things are slightly more flexible this way and
duplicating thunk is not terribly expensive. One can always link with
non-comdat+ alias.

Honza
> 
> 
> -- 
> H.J.

Re: PING^3: [PATCH] i386: Don't generate alias for function return thunk

[H.J. Lu]

On Thu, Mar 15, 2018 at 9:03 AM, Jan Hubicka  wrote:
>> > What is the reason for using different names for return and indirect 
>> > thunks at first place?
>> >
>>
>> These 2 thunks are identical.  But one may want to provide an
>> alternate thunk only for
>> indirect branch and leaves return thunk alone.  You can't do that if
>> both have the same
>> name.
>
> Hmm, OK, what is the benefit to have two different thunks? It is just
> safety precaution so we could adjust one without adjusting the other in
> future?
>

That is correct.


-- 
H.J.

Re: PING^3: [PATCH] i386: Don't generate alias for function return thunk

[Jan Hubicka]

> > What is the reason for using different names for return and indirect thunks 
> > at first place?
> >
> 
> These 2 thunks are identical.  But one may want to provide an
> alternate thunk only for
> indirect branch and leaves return thunk alone.  You can't do that if
> both have the same
> name.

Hmm, OK, what is the benefit to have two different thunks? It is just
safety precaution so we could adjust one without adjusting the other in
future?

Honza

Re: PING^3: [PATCH] i386: Don't generate alias for function return thunk

[H.J. Lu]

On Thu, Mar 15, 2018 at 8:51 AM, Jan Hubicka  wrote:
>> On Sun, Mar 11, 2018 at 7:39 AM, H.J. Lu  wrote:
>> > On Mon, Mar 5, 2018 at 4:17 AM, H.J. Lu  wrote:
>> >> On Mon, Feb 26, 2018 at 12:48 PM, H.J. Lu  wrote:
>> >>> Function return thunks shouldn't be aliased to indirect branch thunks
>> >>> since indirect branch thunks are placed in COMDAT section and a COMDAT
>> >>> section with indirect branch may not have function return thunk.  This
>> >>> patch generates function return thunks directly.
>> >>>
>> >>> Tested on i686 and x86-64.  OK for trunk?
>> >>>
>> >>> H.J.
>> >>> ---
>> >>> gcc/
>> >>>
>> >>> PR target/84574
>> >>> * config/i386/i386.c (indirect_thunk_needed): Update comments.
>> >>> (indirect_thunk_bnd_needed): Likewise.
>> >>> (indirect_thunks_used): Likewise.
>> >>> (indirect_thunks_bnd_used): Likewise.
>> >>> (indirect_return_needed): New.
>> >>> (indirect_return_bnd_needed): Likewise.
>> >>> (output_indirect_thunk_function): Add a bool argument for
>> >>> function return.
>> >>> (output_indirect_thunk_function): Don't generate alias for
>> >>> function return thunk.
>> >>> (ix86_code_end): Call output_indirect_thunk_function to generate
>> >>> function return thunks.
>> >>> (ix86_output_function_return): Set indirect_return_bnd_needed
>> >>> and indirect_return_needed instead of indirect_thunk_bnd_needed
>> >>> and indirect_thunk_needed.
>> >>>
>> >>> gcc/testsuite/
>> >>>
>> >>> PR target/84574
>> >>> * gcc.target/i386/ret-thunk-9.c: Expect __x86_return_thunk
>> >>> label instead of __x86_indirect_thunk label.
>> >>
>
> So problem here is that you may output the comdat without return thunk from 
> one unit
> and with return thunk with different unit and comdat merging will then render 
> return thunk
> unreachable?

Yes.  return thunk becomes undefined.

> What is the reason for using different names for return and indirect thunks 
> at first place?
>

These 2 thunks are identical.  But one may want to provide an
alternate thunk only for
indirect branch and leaves return thunk alone.  You can't do that if
both have the same
name.

-- 
H.J.

Re: PING^3: [PATCH] i386: Don't generate alias for function return thunk

[Jan Hubicka]

> On Sun, Mar 11, 2018 at 7:39 AM, H.J. Lu  wrote:
> > On Mon, Mar 5, 2018 at 4:17 AM, H.J. Lu  wrote:
> >> On Mon, Feb 26, 2018 at 12:48 PM, H.J. Lu  wrote:
> >>> Function return thunks shouldn't be aliased to indirect branch thunks
> >>> since indirect branch thunks are placed in COMDAT section and a COMDAT
> >>> section with indirect branch may not have function return thunk.  This
> >>> patch generates function return thunks directly.
> >>>
> >>> Tested on i686 and x86-64.  OK for trunk?
> >>>
> >>> H.J.
> >>> ---
> >>> gcc/
> >>>
> >>> PR target/84574
> >>> * config/i386/i386.c (indirect_thunk_needed): Update comments.
> >>> (indirect_thunk_bnd_needed): Likewise.
> >>> (indirect_thunks_used): Likewise.
> >>> (indirect_thunks_bnd_used): Likewise.
> >>> (indirect_return_needed): New.
> >>> (indirect_return_bnd_needed): Likewise.
> >>> (output_indirect_thunk_function): Add a bool argument for
> >>> function return.
> >>> (output_indirect_thunk_function): Don't generate alias for
> >>> function return thunk.
> >>> (ix86_code_end): Call output_indirect_thunk_function to generate
> >>> function return thunks.
> >>> (ix86_output_function_return): Set indirect_return_bnd_needed
> >>> and indirect_return_needed instead of indirect_thunk_bnd_needed
> >>> and indirect_thunk_needed.
> >>>
> >>> gcc/testsuite/
> >>>
> >>> PR target/84574
> >>> * gcc.target/i386/ret-thunk-9.c: Expect __x86_return_thunk
> >>> label instead of __x86_indirect_thunk label.
> >>

So problem here is that you may output the comdat without return thunk from one 
unit
and with return thunk with different unit and comdat merging will then render 
return thunk
unreachable?

What is the reason for using different names for return and indirect thunks at 
first place?

Honza

PING^3: [PATCH] i386: Don't generate alias for function return thunk

[H.J. Lu]

On Sun, Mar 11, 2018 at 7:39 AM, H.J. Lu  wrote:
> On Mon, Mar 5, 2018 at 4:17 AM, H.J. Lu  wrote:
>> On Mon, Feb 26, 2018 at 12:48 PM, H.J. Lu  wrote:
>>> Function return thunks shouldn't be aliased to indirect branch thunks
>>> since indirect branch thunks are placed in COMDAT section and a COMDAT
>>> section with indirect branch may not have function return thunk.  This
>>> patch generates function return thunks directly.
>>>
>>> Tested on i686 and x86-64.  OK for trunk?
>>>
>>> H.J.
>>> ---
>>> gcc/
>>>
>>> PR target/84574
>>> * config/i386/i386.c (indirect_thunk_needed): Update comments.
>>> (indirect_thunk_bnd_needed): Likewise.
>>> (indirect_thunks_used): Likewise.
>>> (indirect_thunks_bnd_used): Likewise.
>>> (indirect_return_needed): New.
>>> (indirect_return_bnd_needed): Likewise.
>>> (output_indirect_thunk_function): Add a bool argument for
>>> function return.
>>> (output_indirect_thunk_function): Don't generate alias for
>>> function return thunk.
>>> (ix86_code_end): Call output_indirect_thunk_function to generate
>>> function return thunks.
>>> (ix86_output_function_return): Set indirect_return_bnd_needed
>>> and indirect_return_needed instead of indirect_thunk_bnd_needed
>>> and indirect_thunk_needed.
>>>
>>> gcc/testsuite/
>>>
>>> PR target/84574
>>> * gcc.target/i386/ret-thunk-9.c: Expect __x86_return_thunk
>>> label instead of __x86_indirect_thunk label.
>>
>> PING:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2018-02/msg01460.html
>>
>
> PING.
>

PING.


-- 
H.J.

PING^2: [PATCH] i386: Don't generate alias for function return thunk

[H.J. Lu]

On Mon, Mar 5, 2018 at 4:17 AM, H.J. Lu  wrote:
> On Mon, Feb 26, 2018 at 12:48 PM, H.J. Lu  wrote:
>> Function return thunks shouldn't be aliased to indirect branch thunks
>> since indirect branch thunks are placed in COMDAT section and a COMDAT
>> section with indirect branch may not have function return thunk.  This
>> patch generates function return thunks directly.
>>
>> Tested on i686 and x86-64.  OK for trunk?
>>
>> H.J.
>> ---
>> gcc/
>>
>> PR target/84574
>> * config/i386/i386.c (indirect_thunk_needed): Update comments.
>> (indirect_thunk_bnd_needed): Likewise.
>> (indirect_thunks_used): Likewise.
>> (indirect_thunks_bnd_used): Likewise.
>> (indirect_return_needed): New.
>> (indirect_return_bnd_needed): Likewise.
>> (output_indirect_thunk_function): Add a bool argument for
>> function return.
>> (output_indirect_thunk_function): Don't generate alias for
>> function return thunk.
>> (ix86_code_end): Call output_indirect_thunk_function to generate
>> function return thunks.
>> (ix86_output_function_return): Set indirect_return_bnd_needed
>> and indirect_return_needed instead of indirect_thunk_bnd_needed
>> and indirect_thunk_needed.
>>
>> gcc/testsuite/
>>
>> PR target/84574
>> * gcc.target/i386/ret-thunk-9.c: Expect __x86_return_thunk
>> label instead of __x86_indirect_thunk label.
>
> PING:
>
> https://gcc.gnu.org/ml/gcc-patches/2018-02/msg01460.html
>

PING.

-- 
H.J.

PING: [PATCH] i386: Don't generate alias for function return thunk

[H.J. Lu]

On Mon, Feb 26, 2018 at 12:48 PM, H.J. Lu  wrote:
> Function return thunks shouldn't be aliased to indirect branch thunks
> since indirect branch thunks are placed in COMDAT section and a COMDAT
> section with indirect branch may not have function return thunk.  This
> patch generates function return thunks directly.
>
> Tested on i686 and x86-64.  OK for trunk?
>
> H.J.
> ---
> gcc/
>
> PR target/84574
> * config/i386/i386.c (indirect_thunk_needed): Update comments.
> (indirect_thunk_bnd_needed): Likewise.
> (indirect_thunks_used): Likewise.
> (indirect_thunks_bnd_used): Likewise.
> (indirect_return_needed): New.
> (indirect_return_bnd_needed): Likewise.
> (output_indirect_thunk_function): Add a bool argument for
> function return.
> (output_indirect_thunk_function): Don't generate alias for
> function return thunk.
> (ix86_code_end): Call output_indirect_thunk_function to generate
> function return thunks.
> (ix86_output_function_return): Set indirect_return_bnd_needed
> and indirect_return_needed instead of indirect_thunk_bnd_needed
> and indirect_thunk_needed.
>
> gcc/testsuite/
>
> PR target/84574
> * gcc.target/i386/ret-thunk-9.c: Expect __x86_return_thunk
> label instead of __x86_indirect_thunk label.

PING:

https://gcc.gnu.org/ml/gcc-patches/2018-02/msg01460.html

-- 
H.J.

[PATCH] i386: Don't generate alias for function return thunk

[H.J. Lu]

Function return thunks shouldn't be aliased to indirect branch thunks
since indirect branch thunks are placed in COMDAT section and a COMDAT
section with indirect branch may not have function return thunk.  This
patch generates function return thunks directly.

Tested on i686 and x86-64.  OK for trunk?

H.J.
---
gcc/

PR target/84574
* config/i386/i386.c (indirect_thunk_needed): Update comments.
(indirect_thunk_bnd_needed): Likewise.
(indirect_thunks_used): Likewise.
(indirect_thunks_bnd_used): Likewise.
(indirect_return_needed): New.
(indirect_return_bnd_needed): Likewise.
(output_indirect_thunk_function): Add a bool argument for
function return.
(output_indirect_thunk_function): Don't generate alias for
function return thunk.
(ix86_code_end): Call output_indirect_thunk_function to generate
function return thunks.
(ix86_output_function_return): Set indirect_return_bnd_needed
and indirect_return_needed instead of indirect_thunk_bnd_needed
and indirect_thunk_needed.

gcc/testsuite/

PR target/84574
* gcc.target/i386/ret-thunk-9.c: Expect __x86_return_thunk
label instead of __x86_indirect_thunk label.
---
 gcc/config/i386/i386.c  | 95 +++--
 gcc/testsuite/gcc.target/i386/ret-thunk-9.c |  2 +-
 2 files changed, 36 insertions(+), 61 deletions(-)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 49f872d103f..9041485bd61 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -10799,19 +10799,23 @@ ix86_setup_frame_addresses (void)
labels in call and return thunks.  */
 static int indirectlabelno;
 
-/* True if call and return thunk functions are needed.  */
+/* True if call thunk function is needed.  */
 static bool indirect_thunk_needed = false;
-/* True if call and return thunk functions with the BND prefix are
-   needed.  */
+/* True if call thunk function with the BND prefix is needed.  */
 static bool indirect_thunk_bnd_needed = false;
 
 /* Bit masks of integer registers, which contain branch target, used
-   by call and return thunks functions.  */
+   by call thunk functions.  */
 static int indirect_thunks_used;
 /* Bit masks of integer registers, which contain branch target, used
-   by call and return thunks functions with the BND prefix.  */
+   by call thunk functions with the BND prefix.  */
 static int indirect_thunks_bnd_used;
 
+/* True if return thunk function is needed.  */
+static bool indirect_return_needed = false;
+/* True if return thunk function with the BND prefix is needed.  */
+static bool indirect_return_bnd_needed = false;
+
 /* True if return thunk function via CX is needed.  */
 static bool indirect_return_via_cx;
 /* True if return thunk function via CX with the BND prefix is
@@ -11007,17 +11011,18 @@ output_indirect_thunk (enum indirect_thunk_prefix 
need_prefix,
 /* Output a funtion with a call and return thunk for indirect branch.
If BND_P is true, the BND prefix is needed.  If REGNO != UNVALID_REGNUM,
the function address is in REGNO.  Otherwise, the function address is
-   on the top of stack.  */
+   on the top of stack.  Thunk is used for function return if RET_P is
+   true.  */
 
 static void
 output_indirect_thunk_function (enum indirect_thunk_prefix need_prefix,
-   unsigned int regno)
+   unsigned int regno, bool ret_p)
 {
   char name[32];
   tree decl;
 
   /* Create __x86_indirect_thunk/__x86_indirect_thunk_bnd.  */
-  indirect_thunk_name (name, regno, need_prefix, false);
+  indirect_thunk_name (name, regno, need_prefix, ret_p);
   decl = build_decl (BUILTINS_LOCATION, FUNCTION_DECL,
 get_identifier (name),
 build_function_type_list (void_type_node, NULL_TREE));
@@ -11060,50 +11065,6 @@ output_indirect_thunk_function (enum 
indirect_thunk_prefix need_prefix,
ASM_OUTPUT_LABEL (asm_out_file, name);
   }
 
-  /* Create alias for __x86_return_thunk/__x86_return_thunk_bnd or
- __x86_return_thunk_ecx/__x86_return_thunk_ecx_bnd.  */
-  bool need_alias;
-  if (regno == INVALID_REGNUM)
-need_alias = true;
-  else if (regno == CX_REG)
-{
-  if (need_prefix == indirect_thunk_prefix_bnd)
-   need_alias = indirect_return_via_cx_bnd;
-  else
-   need_alias = indirect_return_via_cx;
-}
-  else
-need_alias = false;
-
-  if (need_alias)
-{
-  char alias[32];
-
-  indirect_thunk_name (alias, regno, need_prefix, true);
-#if TARGET_MACHO
-  if (TARGET_MACHO)
-   {
- fputs ("\t.weak_definition\t", asm_out_file);
- assemble_name (asm_out_file, alias);
- fputs ("\n\t.private_extern\t", asm_out_file);
- assemble_name (asm_out_file, alias);
- putc ('\n', asm_out_file);
- ASM_OUTPUT_LABEL (asm_out_file, alias);
-   }
-#else
-