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

["Marcin Kościelnicki" <koriakin@0x04.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