Re: [PATCH] S390: Hotpatching fixes.
On Fri, Mar 27, 2015 at 10:30:38AM +0100, Andreas Krebbel wrote:
> 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.
Agreed, I also wondered what would be so special about nested functions
here.
Sure, one could hotpatch them with code clobbering the static chain
register, but that wouldn't be a gcc issue.
Jakub
Re: [PATCH] S390: Hotpatching fixes.
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-
Re: [PATCH] S390: Hotpatching fixes.
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.
Jakub
Re: [PATCH] S390: Hotpatching fixes.
New patch with review results:
* 6-byte-NOP only for ZARCH
* Formatting.
Ciao
Dominik ^_^ ^_^
--
Dominik Vogt
IBM Germany
>From 176268849643c46427ea873c35390700ea7a4489 Mon Sep 17 00:00:00 2001
From: Dominik Vogt
Date: Mon, 23 Feb 2015 13:48:26 +0100
Subject: [PATCH 1/2] S390: Hotpatching fixes.
* Properly align function labels with -mhotpatch and add test cases.
* Include the nops after the function label in the area covered by cfi and
debug information.
* Correct a typo in the documentation.
* Fix formatting in the generated 6-byte-NOP and adapt the test cases.
---
gcc/config/s390/s390.c | 66 ++---
gcc/config/s390/s390.md | 25 +++
gcc/doc/invoke.texi | 4 +-
gcc/testsuite/gcc.target/s390/hotpatch-1.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-10.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-11.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-12.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-13.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-14.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-15.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-16.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-17.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-18.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-19.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-2.c | 3 +-
gcc/testsuite/gcc.target/s390/hotpatch-21.c | 14 ++
gcc/testsuite/gcc.target/s390/hotpatch-22.c | 14 ++
gcc/testsuite/gcc.target/s390/hotpatch-23.c | 14 ++
gcc/testsuite/gcc.target/s390/hotpatch-24.c | 14 ++
gcc/testsuite/gcc.target/s390/hotpatch-3.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-4.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-5.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-6.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-7.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-8.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-9.c | 2 +-
26 files changed, 144 insertions(+), 46 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/s390/hotpatch-21.c
create mode 100644 gcc/testsuite/gcc.target/s390/hotpatch-22.c
create mode 100644 gcc/testsuite/gcc.target/s390/hotpatch-23.c
create mode 100644 gcc/testsuite/gcc.target/s390/hotpatch-24.c
diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index 1924f2a..c907577 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -5295,6 +5295,7 @@ s390_asm_output_function_label (FILE *asm_out_file, const char *fname,
if (hotpatch_p)
{
+ unsigned int function_alignment;
int i;
/* Add a trampoline code area before the function label and initialize it
@@ -5308,34 +5309,14 @@ s390_asm_output_function_label (FILE *asm_out_file, const char *fname,
stored directly before the label without crossing a cacheline
boundary. All this is necessary to make sure the trampoline code can
be changed atomically. */
+ function_alignment = MAX (8, DECL_ALIGN (decl) / BITS_PER_UNIT);
+ if (! DECL_USER_ALIGN (decl))
+ function_alignment = MAX (function_alignment,
+ (unsigned int) align_functions);
+ ASM_OUTPUT_ALIGN (asm_out_file, floor_log2 (function_alignment));
}
ASM_OUTPUT_LABEL (asm_out_file, fname);
-
- /* Output a series of NOPs after the function label. */
- if (hotpatch_p)
-{
- while (hw_after > 0)
- {
- if (hw_after >= 3 && TARGET_CPU_ZARCH)
- {
- asm_fprintf (asm_out_file, "\tbrcl\t\t0,0\n");
- hw_after -= 3;
- }
- else if (hw_after >= 2)
- {
- gcc_assert (hw_after == 2 || !TARGET_CPU_ZARCH);
- asm_fprintf (asm_out_file, "\tnop\t0\n");
- hw_after -= 2;
- }
- else
- {
- gcc_assert (hw_after == 1);
- asm_fprintf (asm_out_file, "\tnopr\t%%r7\n");
- hw_after -= 1;
- }
- }
-}
}
/* Output machine-dependent UNSPECs occurring in address constant X
@@ -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)
+{
+ rtx_insn *insn;
+
+ /* Inject nops for hotpatching. */
+ for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
+ {
+ if (NOTE_P (insn) && NOTE_KIND (insn) == NOTE_INSN_FUNCTION_BEG)
+ break;
+ }
+ gcc_assert (insn);
+ /* Output a series of NOPs after the NOTE_INSN_FUNCTION_BEG. */
+ while (hw_after > 0)
+ {
+ if (hw_after >= 3 && TARGET_CPU_ZARCH)
+ {
+ insn = emit_insn_after (gen_nop_6_byte (), insn);
+ hw_after -= 3;
+ }
+ else if (hw_after >= 2)
+ {
+ insn = emit_insn
Re: [PATCH] S390: Hotpatching fixes.
On Mon, Mar 09, 2015 at 12:22:21PM +0100, Dominik Vogt wrote:
> @@ -5308,34 +5309,14 @@ s390_asm_output_function_label (FILE *asm_out_file,
> const char *fname,
>stored directly before the label without crossing a cacheline
>boundary. All this is necessary to make sure the trampoline code can
>be changed atomically. */
> + function_alignment = MAX (8, DECL_ALIGN (decl) / BITS_PER_UNIT);
> + if (! DECL_USER_ALIGN (decl))
> + function_alignment = MAX (function_alignment,
> + (unsigned int) align_functions);
Wrong formatting. function_alignment should be only 2 columns to the right
from if.
> +(define_insn "nop_6_byte"
> + [(unspec_volatile [(const_int 0)] UNSPECV_NOP_6_BYTE)]
> + ""
Shouldn't this use "TARGET_ZARCH" condition?
BTW, have you tried say:
__attribute__((hotpatch (0, 6))) void foo (void) { __builtin_unreachable (); }
?
Have you verified there never are real instructions before
NOTE_INSN_FUNCTION_BEG?
Jakub
Re: [PATCH] S390: Hotpatching fixes.
Updated patch after internal review:
* Moved the hotpatch specific NOP patterns to the normal NOP
patterns in the .md file.
* Make function_alignment unsigned and cast align_function
instead.
(ChangeLog is still the same.)
Ciao
Dominik ^_^ ^_^
--
Dominik Vogt
IBM Germany
>From e0083a3044797bf13ebdc9294fb0ebc117cbed4b Mon Sep 17 00:00:00 2001
From: Dominik Vogt
Date: Mon, 23 Feb 2015 13:48:26 +0100
Subject: [PATCH] S390: Hotpatching fixes.
* Properly align function labels with -mhotpatch and add test cases.
* Include the nops after the function label in the area covered by cfi and
debug information.
* Correct a typo in the documentation.
* Fix formatting in the generated 6-byte-NOP and adapt the test cases.
---
gcc/config/s390/s390.c | 66 ++---
gcc/config/s390/s390.md | 25 +++
gcc/doc/invoke.texi | 4 +-
gcc/testsuite/gcc.target/s390/hotpatch-1.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-10.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-11.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-12.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-13.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-14.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-15.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-16.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-17.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-18.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-19.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-2.c | 3 +-
gcc/testsuite/gcc.target/s390/hotpatch-21.c | 14 ++
gcc/testsuite/gcc.target/s390/hotpatch-22.c | 14 ++
gcc/testsuite/gcc.target/s390/hotpatch-23.c | 14 ++
gcc/testsuite/gcc.target/s390/hotpatch-24.c | 14 ++
gcc/testsuite/gcc.target/s390/hotpatch-3.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-4.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-5.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-6.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-7.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-8.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-9.c | 2 +-
26 files changed, 144 insertions(+), 46 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/s390/hotpatch-21.c
create mode 100644 gcc/testsuite/gcc.target/s390/hotpatch-22.c
create mode 100644 gcc/testsuite/gcc.target/s390/hotpatch-23.c
create mode 100644 gcc/testsuite/gcc.target/s390/hotpatch-24.c
diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index 1924f2a..d0584e8 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -5295,6 +5295,7 @@ s390_asm_output_function_label (FILE *asm_out_file, const char *fname,
if (hotpatch_p)
{
+ unsigned int function_alignment;
int i;
/* Add a trampoline code area before the function label and initialize it
@@ -5308,34 +5309,14 @@ s390_asm_output_function_label (FILE *asm_out_file, const char *fname,
stored directly before the label without crossing a cacheline
boundary. All this is necessary to make sure the trampoline code can
be changed atomically. */
+ function_alignment = MAX (8, DECL_ALIGN (decl) / BITS_PER_UNIT);
+ if (! DECL_USER_ALIGN (decl))
+ function_alignment = MAX (function_alignment,
+ (unsigned int) align_functions);
+ ASM_OUTPUT_ALIGN (asm_out_file, floor_log2 (function_alignment));
}
ASM_OUTPUT_LABEL (asm_out_file, fname);
-
- /* Output a series of NOPs after the function label. */
- if (hotpatch_p)
-{
- while (hw_after > 0)
- {
- if (hw_after >= 3 && TARGET_CPU_ZARCH)
- {
- asm_fprintf (asm_out_file, "\tbrcl\t\t0,0\n");
- hw_after -= 3;
- }
- else if (hw_after >= 2)
- {
- gcc_assert (hw_after == 2 || !TARGET_CPU_ZARCH);
- asm_fprintf (asm_out_file, "\tnop\t0\n");
- hw_after -= 2;
- }
- else
- {
- gcc_assert (hw_after == 1);
- asm_fprintf (asm_out_file, "\tnopr\t%%r7\n");
- hw_after -= 1;
- }
- }
-}
}
/* Output machine-dependent UNSPECs occurring in address constant X
@@ -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)
+{
+ rtx_insn *insn;
+
+ /* Inject nops for hotpatching. */
+ for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
+ {
+ if (NOTE_P (insn) && NOTE_KIND (insn) == NOTE_INSN_FUNCTION_BEG)
+ break;
+ }
+ gcc_assert (insn);
+ /* Output a series of NOPs after the NOTE_INSN_FUNCTION_BEG. */
+
[PATCH] S390: Hotpatching fixes.
S390: Hotpatching fixes.
* Properly align function labels with -mhotpatch and add test cases.
* Include the nops after the function label in the area covered by cfi and
debug information.
* Correct a typo in the documentation.
* Fix formatting in the generated 6-byte-NOP and adapt the test cases.
Ciao
Dominik ^_^ ^_^
--
Dominik Vogt
IBM Germany
>From 2d42c989a83fac102294ebdff6e68ca4bd571915 Mon Sep 17 00:00:00 2001
From: Dominik Vogt
Date: Mon, 23 Feb 2015 13:48:26 +0100
Subject: [PATCH] S390: Hotpatching fixes.
* Properly align function labels with -mhotpatch and add test cases.
* Include the nops after the function label in the area covered by cfi and
debug information.
* Correct a typo in the documentation.
* Fix formatting in the generated 6-byte-NOP and adapt the test cases.
---
gcc/config/s390/s390.c | 65 ++---
gcc/config/s390/s390.md | 28 +
gcc/doc/invoke.texi | 4 +-
gcc/testsuite/gcc.target/s390/hotpatch-1.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-10.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-11.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-12.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-13.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-14.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-15.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-16.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-17.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-18.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-19.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-2.c | 3 +-
gcc/testsuite/gcc.target/s390/hotpatch-21.c | 14 +++
gcc/testsuite/gcc.target/s390/hotpatch-22.c | 14 +++
gcc/testsuite/gcc.target/s390/hotpatch-23.c | 14 +++
gcc/testsuite/gcc.target/s390/hotpatch-24.c | 14 +++
gcc/testsuite/gcc.target/s390/hotpatch-3.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-4.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-5.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-6.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-7.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-8.c | 2 +-
gcc/testsuite/gcc.target/s390/hotpatch-9.c | 2 +-
26 files changed, 146 insertions(+), 46 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/s390/hotpatch-21.c
create mode 100644 gcc/testsuite/gcc.target/s390/hotpatch-22.c
create mode 100644 gcc/testsuite/gcc.target/s390/hotpatch-23.c
create mode 100644 gcc/testsuite/gcc.target/s390/hotpatch-24.c
diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index 1924f2a..bac3555 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -5295,6 +5295,7 @@ s390_asm_output_function_label (FILE *asm_out_file, const char *fname,
if (hotpatch_p)
{
+ int function_alignment;
int i;
/* Add a trampoline code area before the function label and initialize it
@@ -5308,34 +5309,13 @@ s390_asm_output_function_label (FILE *asm_out_file, const char *fname,
stored directly before the label without crossing a cacheline
boundary. All this is necessary to make sure the trampoline code can
be changed atomically. */
+ function_alignment = MAX (8, DECL_ALIGN (decl) / BITS_PER_UNIT);
+ if (! DECL_USER_ALIGN (decl))
+ function_alignment = MAX (function_alignment, align_functions);
+ ASM_OUTPUT_ALIGN (asm_out_file, floor_log2 (function_alignment));
}
ASM_OUTPUT_LABEL (asm_out_file, fname);
-
- /* Output a series of NOPs after the function label. */
- if (hotpatch_p)
-{
- while (hw_after > 0)
- {
- if (hw_after >= 3 && TARGET_CPU_ZARCH)
- {
- asm_fprintf (asm_out_file, "\tbrcl\t\t0,0\n");
- hw_after -= 3;
- }
- else if (hw_after >= 2)
- {
- gcc_assert (hw_after == 2 || !TARGET_CPU_ZARCH);
- asm_fprintf (asm_out_file, "\tnop\t0\n");
- hw_after -= 2;
- }
- else
- {
- gcc_assert (hw_after == 1);
- asm_fprintf (asm_out_file, "\tnopr\t%%r7\n");
- hw_after -= 1;
- }
- }
-}
}
/* Output machine-dependent UNSPECs occurring in address constant X
@@ -11368,6 +11348,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 +11484,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)
+{
+ rtx_insn *insn;
+
+ /* Inject nops for hotpatching. */
+ for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
+ {
+ if (NOTE_P (insn) && NOTE_KIND (insn) == NOTE_INSN_FUNCTION_BEG)
+ break;
+ }
+ gcc_assert (insn);
+ /* Output a series of
