Re: [PATCH 2/5] s390: Fix missing .size directives.

[Marcin Kościelnicki]

On 21/01/16 10:58, Andreas Krebbel wrote:

On 01/20/2016 02:16 PM, Andreas Krebbel wrote:

On 01/02/2016 08:16 PM, Marcin Kościelnicki wrote:

It seems at some point the .size hook was hijacked to emit some
machine-specific directives, and the actual .size directive was
forgotten.  This caused problems for split-stack support, since
linker couldn't scan the function body for non-split-stack calls.

gcc/ChangeLog:

* config/s390/s390.c (s390_asm_declare_function_size): Add code
to actually emit the .size directive.


...


  s390_asm_declare_function_size (FILE *asm_out_file,
-   const char *fnname ATTRIBUTE_UNUSED, tree decl)
+   const char *fnname, tree decl)
  {
+  if (!flag_inhibit_size_directive)
+ASM_OUTPUT_MEASURED_SIZE (asm_out_file, fnname);
if (DECL_FUNCTION_SPECIFIC_TARGET (decl) == NULL)
  return;
fprintf (asm_out_file, "\t.machine pop\n");


It would be good to use the original ASM_DECLARE_FUNCTION_SIZE macro from 
config/elfos.h here.  This
probably would require to change its name in s390.h first and then use it from
s390_asm_declare_function_size. Not really beautiful but at least changes to 
the original macro
would not require adjusting our backend.


I've looked into how the other archs are doing this and didn't find anything 
better than just
including the code from the original macro. The real fix probably would be to 
turn this into a
target hook instead.

I've committed the patch now since it fixes a real problem not only with 
split-stack.

Thanks!

-Andreas-



I did a version that reincludes elfos.h, but it didn't finish testing 
(it made it through bootstrap) before you applied the patch:


diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index 21a5687..c56b909 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -6832,10 +6832,17 @@ s390_asm_output_function_prefix (FILE *asm_out_file,

 /* Write an extra function footer after the very end of the function.  */

+/* Get elfos.h's original ASM_DECLARE_FUNCTION_SIZE, so that we can 
delegate

+   to it below.  */
+
+#undef ASM_DECLARE_FUNCTION_SIZE
+#include "../elfos.h"
+
 void
 s390_asm_declare_function_size (FILE *asm_out_file,
-   const char *fnname ATTRIBUTE_UNUSED, 
tree decl)

+   const char *fnname, tree decl)
 {
+  ASM_DECLARE_FUNCTION_SIZE(asm_out_file, fnname, decl);
   if (DECL_FUNCTION_SPECIFIC_TARGET (decl) == NULL)
 return;
   fprintf (asm_out_file, "\t.machine pop\n");


But, this is much uglier, and the macro is very unlikely to change in 
the first place.  I guess we should stay with the applied patch.


Thanks,

Marcin

Re: [PATCH 2/5] s390: Fix missing .size directives.

[Andreas Krebbel]

On 01/20/2016 02:16 PM, Andreas Krebbel wrote:
> On 01/02/2016 08:16 PM, Marcin Kościelnicki wrote:
>> It seems at some point the .size hook was hijacked to emit some
>> machine-specific directives, and the actual .size directive was
>> forgotten.  This caused problems for split-stack support, since
>> linker couldn't scan the function body for non-split-stack calls.
>>
>> gcc/ChangeLog:
>>
>>  * config/s390/s390.c (s390_asm_declare_function_size): Add code
>>  to actually emit the .size directive.
> 
> ...
> 
>>  s390_asm_declare_function_size (FILE *asm_out_file,
>> -const char *fnname ATTRIBUTE_UNUSED, tree decl)
>> +const char *fnname, tree decl)
>>  {
>> +  if (!flag_inhibit_size_directive)
>> +ASM_OUTPUT_MEASURED_SIZE (asm_out_file, fnname);
>>if (DECL_FUNCTION_SPECIFIC_TARGET (decl) == NULL)
>>  return;
>>fprintf (asm_out_file, "\t.machine pop\n");
> 
> It would be good to use the original ASM_DECLARE_FUNCTION_SIZE macro from 
> config/elfos.h here.  This
> probably would require to change its name in s390.h first and then use it from
> s390_asm_declare_function_size. Not really beautiful but at least changes to 
> the original macro
> would not require adjusting our backend.

I've looked into how the other archs are doing this and didn't find anything 
better than just
including the code from the original macro. The real fix probably would be to 
turn this into a
target hook instead.

I've committed the patch now since it fixes a real problem not only with 
split-stack.

Thanks!

-Andreas-

Re: [PATCH 2/5] s390: Fix missing .size directives.

[Andreas Krebbel]

On 01/02/2016 08:16 PM, Marcin Kościelnicki wrote:
> It seems at some point the .size hook was hijacked to emit some
> machine-specific directives, and the actual .size directive was
> forgotten.  This caused problems for split-stack support, since
> linker couldn't scan the function body for non-split-stack calls.
> 
> gcc/ChangeLog:
> 
>   * config/s390/s390.c (s390_asm_declare_function_size): Add code
>   to actually emit the .size directive.

...

>  s390_asm_declare_function_size (FILE *asm_out_file,
> - const char *fnname ATTRIBUTE_UNUSED, tree decl)
> + const char *fnname, tree decl)
>  {
> +  if (!flag_inhibit_size_directive)
> +ASM_OUTPUT_MEASURED_SIZE (asm_out_file, fnname);
>if (DECL_FUNCTION_SPECIFIC_TARGET (decl) == NULL)
>  return;
>fprintf (asm_out_file, "\t.machine pop\n");

It would be good to use the original ASM_DECLARE_FUNCTION_SIZE macro from 
config/elfos.h here.  This
probably would require to change its name in s390.h first and then use it from
s390_asm_declare_function_size. Not really beautiful but at least changes to 
the original macro
would not require adjusting our backend.

-Andreas-

Re: [PATCH 2/5] s390: Fix missing .size directives.

[Dominik Vogt]

On Wed, Jan 20, 2016 at 02:16:23PM +0100, Andreas Krebbel wrote:
> On 01/02/2016 08:16 PM, Marcin Kościelnicki wrote:
> >  s390_asm_declare_function_size (FILE *asm_out_file,
> > -   const char *fnname ATTRIBUTE_UNUSED, tree decl)
> > +   const char *fnname, tree decl)
> >  {
> > +  if (!flag_inhibit_size_directive)
> > +ASM_OUTPUT_MEASURED_SIZE (asm_out_file, fnname);
> >if (DECL_FUNCTION_SPECIFIC_TARGET (decl) == NULL)
> >  return;
> >fprintf (asm_out_file, "\t.machine pop\n");
> 
> It would be good to use the original ASM_DECLARE_FUNCTION_SIZE macro from 
> config/elfos.h here.  This
> probably would require to change its name in s390.h first and then use it from
> s390_asm_declare_function_size. Not really beautiful but at least changes to 
> the original macro
> would not require adjusting our backend.

Maybe it's better not to invent yet another solution to deal with
this and just do it like proposed in the patch.  So if the default
implementation is ever changed, the same search pattern will find
all identical copies of the code.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[PATCH 2/5] s390: Fix missing .size directives.

[Marcin Kościelnicki]

It seems at some point the .size hook was hijacked to emit some
machine-specific directives, and the actual .size directive was
forgotten.  This caused problems for split-stack support, since
linker couldn't scan the function body for non-split-stack calls.

gcc/ChangeLog:

* config/s390/s390.c (s390_asm_declare_function_size): Add code
to actually emit the .size directive.
---
 gcc/ChangeLog  | 5 +
 gcc/config/s390/s390.c | 4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 2c572a7..6aef3f9 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,5 +1,10 @@
 2016-01-02  Marcin Kościelnicki  
 
+   * config/s390/s390.c (s390_asm_declare_function_size): Add code
+   to actually emit the .size directive.
+
+2016-01-02  Marcin Kościelnicki  
+
* config/s390/s390.md (pool_section_start): Use switch_to_section
to select proper read-only data section instead of hardcoding .rodata.
(pool_section_end): Use switch_to_section to match the above.
diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index 16045f0..9dc8d1e 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -6834,8 +6834,10 @@ s390_asm_output_function_prefix (FILE *asm_out_file,
 
 void
 s390_asm_declare_function_size (FILE *asm_out_file,
-   const char *fnname ATTRIBUTE_UNUSED, tree decl)
+   const char *fnname, tree decl)
 {
+  if (!flag_inhibit_size_directive)
+ASM_OUTPUT_MEASURED_SIZE (asm_out_file, fnname);
   if (DECL_FUNCTION_SPECIFIC_TARGET (decl) == NULL)
 return;
   fprintf (asm_out_file, "\t.machine pop\n");
-- 
2.6.4