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-
