[PATCH] S390: Hotpatching fixes.

[Andreas Krebbel]

On 03/26/2015 09:56 PM, Jakub Jelinek wrote:
> Hi!
> 
> On Mon, Mar 09, 2015 at 01:19:38PM +0100, Dominik Vogt wrote:
>> @@ -11368,6 +11349,7 @@ static void
>>  s390_reorg (void)
>>  {
>>    bool pool_overflow = false;
>> +  int hw_before, hw_after;
>>  
>>    /* Make sure all splits have been performed; splits after
>>       machine_dependent_reorg might confuse insn length counts.  */
>> @@ -11503,6 +11485,40 @@ s390_reorg (void)
>>        if (insn_added_p)
>>  	shorten_branches (get_insns ());
>>      }
>> +
>> +  s390_function_num_hotpatch_hw (current_function_decl, &hw_before, &hw_after);
>> +  if (hw_after > 0)
> 
> Two minor issues, both for nested functions:
> 
> 1) the s390_function_num_hotpatch_hw assigns to ints whose addresses are
> passed as arguments, even when it later decides to return false and in this
> spot you ignore the return value.  Which means that hw_after could be
> non-zero, even when you should be ignoring it.
> So, either you should check above the return value too, or
> change s390_function_num_hotpatch_hw so that it stores 0 for the nested
> functions before returning false.
> 2) as s390_function_num_hotpatch_hw is now called twice for the same
> function, for nested functions you'll get the warning reported twice too.
> Perhaps add some additional argument whether you want the warning or not
> and use it in one of the callers and not in the other one?
> Plus supposedly add testsuite coverage for that.

At a second glance it is not really clear to me why we disable hotpatching for nested functions at
all. While it is probably a bit difficult to actually hotpatch them I don't see why we should
prevent it. We probably just copied that over from the x86 ms_hook_prologue attribute implementation:

static bool
ix86_function_ms_hook_prologue (const_tree fn)
{
  if (fn && lookup_attribute ("ms_hook_prologue", DECL_ATTRIBUTES (fn)))
    {
      if (decl_function_context (fn) != NULL_TREE)
	error_at (DECL_SOURCE_LOCATION (fn),
		  "ms_hook_prologue is not compatible with nested function");
      else
        return true;
    }
  return false;
}

Also the kernel guys (one of the main users of that feature) confirmed that they in principle prefer
hotpatching to behave more like -pg and -pg does insert an mcount call for nested functions.
(Although I would be surprised to hear of nested functions in the Linux kernel).

So I'm inclined to just remove that special handling of nested functions.

This would also fix 1)

Bye,

-Andreas-