Re: [PATCH, Darwin] Fix PR57438 by avoiding empty function bodies and trailing labels.

[Segher Boessenkool]

On Fri, Nov 18, 2016 at 05:19:31PM +, Iain Sandoe wrote:
> I’d like to do that;  is there a way to force a jump table for a limited set 
> of cases? 
> (the example was about the smallest I could get where GCC elected to produce 
> a jump table instead of branches)

--param case-values-threshold ?


Segher

Re: [PATCH, Darwin] Fix PR57438 by avoiding empty function bodies and trailing labels.

[Iain Sandoe]

Hi Mike,

> On 18 Nov 2016, at 17:16, Mike Stump  wrote:
> 
> On Nov 18, 2016, at 4:33 AM, Iain Sandoe  wrote:
>> 
>> As discussed on IRC, I was under the impression that it is desired to move 
>> away from #ifdef towards if() and  I have been adding those where locally 
>> things have been touched - in this case it was only partially possible.
>> 
>> However, FAOD - you pointed out that for the RS6000 back-end #ifdef are 
>> still preferred,
> 
> Shudder.  We can encourage anyone that likes #if, to like if () instead.
> 
>> OK now for trunk?
> 
> Ok; I'm pretty sure that change can only impact darwin.  If you wanted to 
> reduce the test case to 3 cases, I think that would also show the problem 
> that show that your patch fixes it, ok with such a change, if you want.

I’d like to do that;  is there a way to force a jump table for a limited set of 
cases? 
(the example was about the smallest I could get where GCC elected to produce a 
jump table instead of branches)

Iain

> 
>> Open branches?
> 
> I'm fine with back porting.
> 

Re: [PATCH, Darwin] Fix PR57438 by avoiding empty function bodies and trailing labels.

[Mike Stump]

On Nov 18, 2016, at 4:33 AM, Iain Sandoe  wrote:
> 
> As discussed on IRC, I was under the impression that it is desired to move 
> away from #ifdef towards if() and  I have been adding those where locally 
> things have been touched - in this case it was only partially possible.
> 
> However, FAOD - you pointed out that for the RS6000 back-end #ifdef are still 
> preferred,

Shudder.  We can encourage anyone that likes #if, to like if () instead.

> OK now for trunk?

Ok; I'm pretty sure that change can only impact darwin.  If you wanted to 
reduce the test case to 3 cases, I think that would also show the problem that 
show that your patch fixes it, ok with such a change, if you want.

> Open branches?

I'm fine with back porting.

Re: [PATCH, Darwin] Fix PR57438 by avoiding empty function bodies and trailing labels.

[Iain Sandoe]

Hi Segher,
Thanks for the review, 

> On 6 Nov 2016, at 22:09, Segher Boessenkool  
> wrote:
> 

> On Sun, Nov 06, 2016 at 12:13:16PM -0800, Iain Sandoe wrote:
>> 2016-11-06  Iain Sandoe  
>> 
>>  PR target/57438
>>  * config/i386/i386.c (ix86_code_end): Note that we emitted code where 
>> the
>>  function might otherwise appear empty for picbase thunks.
>>  (ix86_output_function_epilogue): If we find a zero-sized function 
>> assume that
>>  reaching it is UB and trap.  If we find a trailing label append a nop.
>>  * config/rs6000/rs6000.c (rs6000_output_function_epilogue): If we find
>>  a zero-sized function assume that reaching it is UB and trap.  If we 
>> find a
>>  trailing label, append a nop.
> 
> These lines are too long.
fixed.
> 
>> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
>> index b0d2b64..326e2e9 100644
>> --- a/gcc/config/rs6000/rs6000.c
>> +++ b/gcc/config/rs6000/rs6000.c
>> @@ -30148,11 +30148,16 @@ rs6000_output_function_epilogue (FILE *file,
>> {
>> #if TARGET_MACHO
>>   macho_branch_islands ();
>> -  /* Mach-O doesn't support labels at the end of objects, so if
>> - it looks like we might want one, insert a NOP.  */
>> +
>> +  if (TARGET_MACHO)
> 
> Both #if and if?

As discussed on IRC, I was under the impression that it is desired to move away 
from #ifdef towards if() and  I have been adding those where locally things 
have been touched - in this case it was only partially possible.

However, FAOD - you pointed out that for the RS6000 back-end #ifdef are still 
preferred, and I’ve removed this change.

> +/* Mach-O doesn't support labels at the end of objects, so if
>> +   it looks like we might want one, take special action.
>> +
>> +   First, collect any sequence deleted debug labels.  */
> 
> "sequence of”.
indeed
> 
>> +/* If we have:
>> +   label:
>> +  barrier
>> +   That need to be guarded.  */
> 
> "then this needs to be guarded.", or similar?
amended,
> 
>> +/* up to now we've only seen notes or barriers.  */
> 
> Sentences start with a capital letter.
fixed
> 
>> +/* See if have a completely empty function body.  */
> 
> "we have”
amended
> 
>> +while (insn && ! INSN_P (insn))
>> +  insn = PREV_INSN (insn);
>> +/* If we don't find any, we've got an empty function body; i.e.
> 
> "any insn”?
amended
> 
>> +   completely empty - without a return or branch.  GCC declares
>> +   that reaching __builtin_unreachable() means UB (but we want
>> +   finite-sized function bodies; to help the user out, let's trap
>> +   the case.  */
> 
> Zero is finite size; "non-empty function bodies”?
yeah… amended.
> 
> The rs6000 code looks fine, thanks; but please work on the text a bit :-)

Sorry, that was poor proof-reading (too many times looking at the same code, I 
guess).

OK now for trunk?
Open branches?
Iain

PR target/57438
* config/i386/i386.c (ix86_code_end): Note that we emitted code
where the function might otherwise appear empty for picbase thunks.
(ix86_output_function_epilogue): If we find a zero-sized function
assume that reaching it is UB and trap.  If we find a trailing label
append a nop.
* config/rs6000/rs6000.c (rs6000_output_function_epilogue): If we
find a zero-sized function assume that reaching it is UB and trap.
If we find a trailing label, append a nop.



PR57438-revised.patch
Description: Binary data

Re: [PATCH, Darwin] Fix PR57438 by avoiding empty function bodies and trailing labels.

[Mike Stump]

On Nov 6, 2016, at 12:13 PM, Iain Sandoe  wrote:
> 
> OK for trunk?
> OK for open branches?

For the darwin parts, Ok.

> 2016-11-06  Iain Sandoe  
> 
>   PR target/57438
>   * config/i386/i386.c (ix86_code_end): Note that we emitted code where 
> the
>   function might otherwise appear empty for picbase thunks.
>   (ix86_output_function_epilogue): If we find a zero-sized function 
> assume that
>   reaching it is UB and trap.  If we find a trailing label append a nop.
>   * config/rs6000/rs6000.c (rs6000_output_function_epilogue): If we find
>   a zero-sized function assume that reaching it is UB and trap.  If we 
> find a
>   trailing label, append a nop.
> 
> gcc/testsuite/
> 
> 2016-11-06  Iain Sandoe  
> 
>   PR target/57438
>   * gcc.dg/pr57438-1.c: New.
>   * gcc.dg/pr57438-2.c: New.

Re: [PATCH, Darwin] Fix PR57438 by avoiding empty function bodies and trailing labels.

[Segher Boessenkool]

Hi Iain,

On Sun, Nov 06, 2016 at 12:13:16PM -0800, Iain Sandoe wrote:
> 2016-11-06  Iain Sandoe  
> 
>   PR target/57438
>   * config/i386/i386.c (ix86_code_end): Note that we emitted code where 
> the
>   function might otherwise appear empty for picbase thunks.
>   (ix86_output_function_epilogue): If we find a zero-sized function 
> assume that
>   reaching it is UB and trap.  If we find a trailing label append a nop.
>   * config/rs6000/rs6000.c (rs6000_output_function_epilogue): If we find
>   a zero-sized function assume that reaching it is UB and trap.  If we 
> find a
>   trailing label, append a nop.

These lines are too long.

> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index b0d2b64..326e2e9 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -30148,11 +30148,16 @@ rs6000_output_function_epilogue (FILE *file,
>  {
>  #if TARGET_MACHO
>macho_branch_islands ();
> -  /* Mach-O doesn't support labels at the end of objects, so if
> - it looks like we might want one, insert a NOP.  */
> +
> +  if (TARGET_MACHO)

Both #if and if?

> +/* Mach-O doesn't support labels at the end of objects, so if
> +   it looks like we might want one, take special action.
> +
> +   First, collect any sequence deleted debug labels.  */

"sequence of".

> +/* If we have:
> +   label:
> +  barrier
> +   That need to be guarded.  */

"then this needs to be guarded.", or similar?

> +/* up to now we've only seen notes or barriers.  */

Sentences start with a capital letter.

> + /* See if have a completely empty function body.  */

"we have"

> + while (insn && ! INSN_P (insn))
> +   insn = PREV_INSN (insn);
> + /* If we don't find any, we've got an empty function body; i.e.

"any insn"?

> +completely empty - without a return or branch.  GCC declares
> +that reaching __builtin_unreachable() means UB (but we want
> +finite-sized function bodies; to help the user out, let's trap
> +the case.  */

Zero is finite size; "non-empty function bodies"?

The rs6000 code looks fine, thanks; but please work on the text a bit :-)


Segher

[PATCH, Darwin] Fix PR57438 by avoiding empty function bodies and trailing labels.

[Iain Sandoe]

Hi Folks,

PR57438 has two manifestations - which arise from the same root cause.

A.
Empty function bodies causes two problems for Darwin's linker (i) zero-length 
FDEs and (ii) coincident label addresses that might point to items of differing 
weakness. 

B.
Trailing local labels can be problematic when they end a function because 
similarly they might apparently point to a following weak function, leading to 
the linker concluding that there's a pointer-diff to a weak symbol (which is 
not allowed).

Both conditions arise from __builtin_unreachable() lowering to a barrier and no 
more; GCC defines reaching code marked as unreachable as UB.

The solution for both is to emit some finite amount of code;
  in the case of A a trap is emitted,
  in the case of B a nop.

Both the X86 and PowerPC ports have code that is supposed to solve B, but which 
doesn’t seem to take into account the emitted barrier (I’m guessing output was 
amended after the code was added, and no test-case to catch the regression).

The patch updates the code - and also checks for completely empty functions 
(regardless of absent labels those will create an issue when the frame tables 
are emitted).

x86 has one extra wrinkle over powerpc; we emit pic load thunks, late and 
manually (as direct asm) but not late enough to avoid the code that is 
detecting empty functions - so we need to signal to that code that the function 
is not really “empty” even tho there’s no rtx.

OK for trunk?
OK for open branches?
Iain

gcc/

2016-11-06  Iain Sandoe  

PR target/57438
* config/i386/i386.c (ix86_code_end): Note that we emitted code where 
the
function might otherwise appear empty for picbase thunks.
(ix86_output_function_epilogue): If we find a zero-sized function 
assume that
reaching it is UB and trap.  If we find a trailing label append a nop.
* config/rs6000/rs6000.c (rs6000_output_function_epilogue): If we find
a zero-sized function assume that reaching it is UB and trap.  If we 
find a
trailing label, append a nop.

gcc/testsuite/

2016-11-06  Iain Sandoe  

PR target/57438
* gcc.dg/pr57438-1.c: New.
* gcc.dg/pr57438-2.c: New.


---
 gcc/config/i386/i386.c   |  88 --
 gcc/config/rs6000/rs6000.c   |  43 +++--
 gcc/testsuite/gcc.dg/pr57438-1.c |  15 +++
 gcc/testsuite/gcc.dg/pr57438-2.c | 194 +++
 4 files changed, 306 insertions(+), 34 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr57438-1.c
 create mode 100644 gcc/testsuite/gcc.dg/pr57438-2.c

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 9e9fe02..a60bc69 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -11866,6 +11866,9 @@ ix86_code_end (void)
   current_function_decl = decl;
   allocate_struct_function (decl, false);
   init_function_start (decl);
+  /* We're about to hide the function body from callees of final_* by
+emitting it directly; tell them we're a thunk, if they care.  */
+  cfun->is_thunk = true;
   first_function_block_is_cold = false;
   /* Make sure unwind info is emitted for the thunk if needed.  */
   final_start_function (emit_barrier (), asm_out_file, 1);
@@ -14562,36 +14565,65 @@ ix86_output_function_epilogue (FILE *file 
ATTRIBUTE_UNUSED, HOST_WIDE_INT)
   if (pic_offset_table_rtx
   && !ix86_use_pseudo_pic_reg ())
 SET_REGNO (pic_offset_table_rtx, REAL_PIC_OFFSET_TABLE_REGNUM);
-#if TARGET_MACHO
-  /* Mach-O doesn't support labels at the end of objects, so if
- it looks like we might want one, insert a NOP.  */
-  {
-rtx_insn *insn = get_last_insn ();
-rtx_insn *deleted_debug_label = NULL;
-while (insn
-  && NOTE_P (insn)
-  && NOTE_KIND (insn) != NOTE_INSN_DELETED_LABEL)
-  {
-   /* Don't insert a nop for NOTE_INSN_DELETED_DEBUG_LABEL
-  notes only, instead set their CODE_LABEL_NUMBER to -1,
-  otherwise there would be code generation differences
-  in between -g and -g0.  */
-   if (NOTE_P (insn) && NOTE_KIND (insn) == NOTE_INSN_DELETED_DEBUG_LABEL)
- deleted_debug_label = insn;
+
+  if (TARGET_MACHO)
+{
+  rtx_insn *insn = get_last_insn ();
+  rtx_insn *deleted_debug_label = NULL;
+
+  /* Mach-O doesn't support labels at the end of objects, so if
+ it looks like we might want one, take special action.
+First, Collect any sequence deleted debug labels.  */
+  while (insn
+&& NOTE_P (insn)
+&& NOTE_KIND (insn) != NOTE_INSN_DELETED_LABEL)
+   {
+ /* Don't insert a nop for NOTE_INSN_DELETED_DEBUG_LABEL
+notes only, instead set their CODE_LABEL_NUMBER to -1,
+otherwise there would be code generation differences
+in between -g and -g0.  */
+ if (NOTE_P (insn) && NOTE_KIND (insn)
+