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

[Marcin KoÅcielnicki <koriakin at 0x04 dot net>]

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