diff mbox series

S/390: Improve storing asan frame_pc

Message ID 20190702085154.26981-1-iii@linux.ibm.com
State New
Headers show
Series S/390: Improve storing asan frame_pc | expand

Commit Message

Ilya Leoshkevich July 2, 2019, 8:51 a.m. UTC 
Bootstrapped and regtested on x86_64-redhat-linux, s390x-redhat-linux
and ppc64le-redhat-linux.

Currently s390 emits the following sequence to store a frame_pc:

	a:
	.LASANPC0:

		lg	%r1,.L5-.L4(%r13)
		la	%r1,0(%r1,%r12)
		stg	%r1,176(%r11)

	.L5:
		.quad	.LASANPC0@GOTOFF

The reason GOT indirection is used instead of larl is that gcc does not
know that .LASANPC0, being a code label, is aligned on a 2-byte
boundary, and larl can load only even addresses.

This patch provides such an alignment hint.  Since targets don't provide
their instruction alignments yet, the new hook is introduced for that
purpose.  It returns 1-byte alignment by default, so this change is a
no-op for targets other than s390.

As a result, we get the desired:

		larl	%r1,.LASANPC0
		stg	%r1,176(%r11)

gcc/ChangeLog:

2019-06-28  Ilya Leoshkevich  <iii@linux.ibm.com>

	* asan.c (asan_emit_stack_protection): Provide an alignment
	hint.
	* config/s390/s390.c (TARGET_INSN_ALIGNMENT): Specify that s390
	requires instructions to be aligned on a 2-byte boundary.
	* doc/tm.texi: Document TARGET_INSN_ALIGNMENT.
	* doc/tm.texi.in: Likewise.
	* target.def (insn_alignment): New hook.

gcc/testsuite/ChangeLog:

2019-06-28  Ilya Leoshkevich  <iii@linux.ibm.com>

	* gcc.target/s390/asan-no-gotoff.c: New test.
---
 gcc/asan.c                                     |  1 +
 gcc/config/s390/s390.c                         |  3 +++
 gcc/doc/tm.texi                                |  7 +++++++
 gcc/doc/tm.texi.in                             |  2 ++
 gcc/target.def                                 |  9 +++++++++
 gcc/testsuite/gcc.target/s390/asan-no-gotoff.c | 15 +++++++++++++++
 6 files changed, 37 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/s390/asan-no-gotoff.c

Comments

Segher Boessenkool July 2, 2019, 1:02 p.m. UTC | #1 
Hi Ilya,

On Tue, Jul 02, 2019 at 10:51:54AM +0200, Ilya Leoshkevich wrote:
> +#undef TARGET_INSN_ALIGNMENT
> +#define TARGET_INSN_ALIGNMENT 16

There already is FUNCTION_BOUNDARY for something similar, which fits in
well with STACK_BOUNDARY, PARM_BOUNDARY, many more *_BOUNDARY.  I realise
you may prefer a hook, but as long as we aren't getting rid of all the
other macros, what's the point?


Segher
Segher Boessenkool July 2, 2019, 1:19 p.m. UTC | #2 
On Tue, Jul 02, 2019 at 08:02:16AM -0500, Segher Boessenkool wrote:
> On Tue, Jul 02, 2019 at 10:51:54AM +0200, Ilya Leoshkevich wrote:
> > +#undef TARGET_INSN_ALIGNMENT
> > +#define TARGET_INSN_ALIGNMENT 16
> 
> There already is FUNCTION_BOUNDARY for something similar, which fits in
> well with STACK_BOUNDARY, PARM_BOUNDARY, many more *_BOUNDARY.  I realise
> you may prefer a hook, but as long as we aren't getting rid of all the
> other macros, what's the point?

And maybe LABEL_BOUNDARY is bettter for this than INSN_BOUNDARY as well?


Segher
Ilya Leoshkevich July 2, 2019, 1:33 p.m. UTC | #3 
> Am 02.07.2019 um 15:19 schrieb Segher Boessenkool <segher@kernel.crashing.org>:
> 
> On Tue, Jul 02, 2019 at 08:02:16AM -0500, Segher Boessenkool wrote:
>> On Tue, Jul 02, 2019 at 10:51:54AM +0200, Ilya Leoshkevich wrote:
>>> +#undef TARGET_INSN_ALIGNMENT
>>> +#define TARGET_INSN_ALIGNMENT 16
>> 
>> There already is FUNCTION_BOUNDARY for something similar, which fits in
>> well with STACK_BOUNDARY, PARM_BOUNDARY, many more *_BOUNDARY.  I realise
>> you may prefer a hook, but as long as we aren't getting rid of all the
>> other macros, what's the point?
> 
> And maybe LABEL_BOUNDARY is bettter for this than INSN_BOUNDARY as well?

Can’t we just use FUNCTION_BOUNDARY then?
I think .LASANPC is always emitted at the beginning of a function.

Best regards,
Ilya
Jakub Jelinek July 2, 2019, 1:39 p.m. UTC | #4 
On Tue, Jul 02, 2019 at 03:33:28PM +0200, Ilya Leoshkevich wrote:
> > Am 02.07.2019 um 15:19 schrieb Segher Boessenkool <segher@kernel.crashing.org>:
> > 
> > On Tue, Jul 02, 2019 at 08:02:16AM -0500, Segher Boessenkool wrote:
> >> On Tue, Jul 02, 2019 at 10:51:54AM +0200, Ilya Leoshkevich wrote:
> >>> +#undef TARGET_INSN_ALIGNMENT
> >>> +#define TARGET_INSN_ALIGNMENT 16
> >> 
> >> There already is FUNCTION_BOUNDARY for something similar, which fits in
> >> well with STACK_BOUNDARY, PARM_BOUNDARY, many more *_BOUNDARY.  I realise
> >> you may prefer a hook, but as long as we aren't getting rid of all the
> >> other macros, what's the point?
> > 
> > And maybe LABEL_BOUNDARY is bettter for this than INSN_BOUNDARY as well?
> 
> Can’t we just use FUNCTION_BOUNDARY then?
> I think .LASANPC is always emitted at the beginning of a function.

Isn't e.g. the hotpatch sequence emitted before it?

	Jakub
Ilya Leoshkevich July 2, 2019, 1:55 p.m. UTC | #5 
> Am 02.07.2019 um 15:39 schrieb Jakub Jelinek <jakub@redhat.com>:
> 
> On Tue, Jul 02, 2019 at 03:33:28PM +0200, Ilya Leoshkevich wrote:
>>> Am 02.07.2019 um 15:19 schrieb Segher Boessenkool <segher@kernel.crashing.org>:
>>> 
>>> On Tue, Jul 02, 2019 at 08:02:16AM -0500, Segher Boessenkool wrote:
>>>> On Tue, Jul 02, 2019 at 10:51:54AM +0200, Ilya Leoshkevich wrote:
>>>>> +#undef TARGET_INSN_ALIGNMENT
>>>>> +#define TARGET_INSN_ALIGNMENT 16
>>>> 
>>>> There already is FUNCTION_BOUNDARY for something similar, which fits in
>>>> well with STACK_BOUNDARY, PARM_BOUNDARY, many more *_BOUNDARY.  I realise
>>>> you may prefer a hook, but as long as we aren't getting rid of all the
>>>> other macros, what's the point?
>>> 
>>> And maybe LABEL_BOUNDARY is bettter for this than INSN_BOUNDARY as well?
>> 
>> Can’t we just use FUNCTION_BOUNDARY then?
>> I think .LASANPC is always emitted at the beginning of a function.
> 
> Isn't e.g. the hotpatch sequence emitted before it?

You are right, with -fpatchable-function-entry it’s moved.

So, I guess I should stick with the current approach.
I could change TARGET_INSN_ALIGNMENT hook to INSN_BOUNDARY macro if that
would better match the current design.  I would still call it INSN, and
not LABEL, because LABEL can refer to data.
Segher Boessenkool July 2, 2019, 2:26 p.m. UTC | #6 
On Tue, Jul 02, 2019 at 03:55:56PM +0200, Ilya Leoshkevich wrote:
> > Am 02.07.2019 um 15:39 schrieb Jakub Jelinek <jakub@redhat.com>:
> > On Tue, Jul 02, 2019 at 03:33:28PM +0200, Ilya Leoshkevich wrote:
> >>> Am 02.07.2019 um 15:19 schrieb Segher Boessenkool <segher@kernel.crashing.org>:
> >>> 
> >>> On Tue, Jul 02, 2019 at 08:02:16AM -0500, Segher Boessenkool wrote:
> >>>> On Tue, Jul 02, 2019 at 10:51:54AM +0200, Ilya Leoshkevich wrote:
> >>>>> +#undef TARGET_INSN_ALIGNMENT
> >>>>> +#define TARGET_INSN_ALIGNMENT 16
> >>>> 
> >>>> There already is FUNCTION_BOUNDARY for something similar, which fits in
> >>>> well with STACK_BOUNDARY, PARM_BOUNDARY, many more *_BOUNDARY.  I realise
> >>>> you may prefer a hook, but as long as we aren't getting rid of all the
> >>>> other macros, what's the point?
> >>> 
> >>> And maybe LABEL_BOUNDARY is bettter for this than INSN_BOUNDARY as well?
> >> 
> >> Can’t we just use FUNCTION_BOUNDARY then?
> >> I think .LASANPC is always emitted at the beginning of a function.
> > 
> > Isn't e.g. the hotpatch sequence emitted before it?
> 
> You are right, with -fpatchable-function-entry it’s moved.
> 
> So, I guess I should stick with the current approach.
> I could change TARGET_INSN_ALIGNMENT hook to INSN_BOUNDARY macro if that
> would better match the current design.  I would still call it INSN, and
> not LABEL, because LABEL can refer to data.

On some archs LABEL_BOUNDARY can be bigger than INSN_BOUNDARY (just like
FUNCTION_BOUNDARY can be even bigger, like on 390 :-) )

Either will work for your purposes afaics.

LABEL in RTL is always a CODE_LABEL I think?  Maybe CODE_LABEL_BOUNDARY
would make it clearer, it's not like a short name for this is useful
anyway.


Segher
Richard Sandiford July 3, 2019, 8:47 p.m. UTC | #7 
Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Tue, Jul 02, 2019 at 03:55:56PM +0200, Ilya Leoshkevich wrote:
>> > Am 02.07.2019 um 15:39 schrieb Jakub Jelinek <jakub@redhat.com>:
>> > On Tue, Jul 02, 2019 at 03:33:28PM +0200, Ilya Leoshkevich wrote:
>> >>> Am 02.07.2019 um 15:19 schrieb Segher Boessenkool <segher@kernel.crashing.org>:
>> >>> 
>> >>> On Tue, Jul 02, 2019 at 08:02:16AM -0500, Segher Boessenkool wrote:
>> >>>> On Tue, Jul 02, 2019 at 10:51:54AM +0200, Ilya Leoshkevich wrote:
>> >>>>> +#undef TARGET_INSN_ALIGNMENT
>> >>>>> +#define TARGET_INSN_ALIGNMENT 16
>> >>>> 
>> >>>> There already is FUNCTION_BOUNDARY for something similar, which fits in
>> >>>> well with STACK_BOUNDARY, PARM_BOUNDARY, many more *_BOUNDARY.  I realise
>> >>>> you may prefer a hook, but as long as we aren't getting rid of all the
>> >>>> other macros, what's the point?
>> >>> 
>> >>> And maybe LABEL_BOUNDARY is bettter for this than INSN_BOUNDARY as well?
>> >> 
>> >> Can’t we just use FUNCTION_BOUNDARY then?
>> >> I think .LASANPC is always emitted at the beginning of a function.
>> > 
>> > Isn't e.g. the hotpatch sequence emitted before it?
>> 
>> You are right, with -fpatchable-function-entry it’s moved.
>> 
>> So, I guess I should stick with the current approach.
>> I could change TARGET_INSN_ALIGNMENT hook to INSN_BOUNDARY macro if that
>> would better match the current design.  I would still call it INSN, and
>> not LABEL, because LABEL can refer to data.
>
> On some archs LABEL_BOUNDARY can be bigger than INSN_BOUNDARY (just like
> FUNCTION_BOUNDARY can be even bigger, like on 390 :-) )
>
> Either will work for your purposes afaics.
>
> LABEL in RTL is always a CODE_LABEL I think?  Maybe CODE_LABEL_BOUNDARY
> would make it clearer, it's not like a short name for this is useful
> anyway.

IIUC the new value is effectively a mandatory/guaranteed minimum value of
align_labels/LABEL_ALIGN that applies even in blocks optimized for size.
So IMO sticking with *_ALIGNMENT would be better.

Thanks,
Richard
Ilya Leoshkevich July 4, 2019, 11:01 a.m. UTC | #8 
> Am 03.07.2019 um 22:47 schrieb Richard Sandiford <richard.sandiford@arm.com>:
> 
> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> On Tue, Jul 02, 2019 at 03:55:56PM +0200, Ilya Leoshkevich wrote:
>>>> Am 02.07.2019 um 15:39 schrieb Jakub Jelinek <jakub@redhat.com>:
>>>> On Tue, Jul 02, 2019 at 03:33:28PM +0200, Ilya Leoshkevich wrote:
>>>>>> Am 02.07.2019 um 15:19 schrieb Segher Boessenkool <segher@kernel.crashing.org>:
>>>>>> 
>>>>>> On Tue, Jul 02, 2019 at 08:02:16AM -0500, Segher Boessenkool wrote:
>>>>>>> On Tue, Jul 02, 2019 at 10:51:54AM +0200, Ilya Leoshkevich wrote:
>>>>>>>> +#undef TARGET_INSN_ALIGNMENT
>>>>>>>> +#define TARGET_INSN_ALIGNMENT 16
>>>>>>> 
>>>>>>> There already is FUNCTION_BOUNDARY for something similar, which fits in
>>>>>>> well with STACK_BOUNDARY, PARM_BOUNDARY, many more *_BOUNDARY.  I realise
>>>>>>> you may prefer a hook, but as long as we aren't getting rid of all the
>>>>>>> other macros, what's the point?
>>>>>> 
>>>>>> And maybe LABEL_BOUNDARY is bettter for this than INSN_BOUNDARY as well?
>>>>> 
>>>>> Can’t we just use FUNCTION_BOUNDARY then?
>>>>> I think .LASANPC is always emitted at the beginning of a function.
>>>> 
>>>> Isn't e.g. the hotpatch sequence emitted before it?
>>> 
>>> You are right, with -fpatchable-function-entry it’s moved.
>>> 
>>> So, I guess I should stick with the current approach.
>>> I could change TARGET_INSN_ALIGNMENT hook to INSN_BOUNDARY macro if that
>>> would better match the current design.  I would still call it INSN, and
>>> not LABEL, because LABEL can refer to data.
>> 
>> On some archs LABEL_BOUNDARY can be bigger than INSN_BOUNDARY (just like
>> FUNCTION_BOUNDARY can be even bigger, like on 390 :-) )
>> 
>> Either will work for your purposes afaics.
>> 
>> LABEL in RTL is always a CODE_LABEL I think?  Maybe CODE_LABEL_BOUNDARY
>> would make it clearer, it's not like a short name for this is useful
>> anyway.
> 
> IIUC the new value is effectively a mandatory/guaranteed minimum value of
> align_labels/LABEL_ALIGN that applies even in blocks optimized for size.
> So IMO sticking with *_ALIGNMENT would be better.

The new value should definitely be consistent with LABEL_ALIGN.  Still,
it is supposed to be applied primarily to trees, not rtxes, and I think
that in this regard it groups nicer with *_BOUNDARY macros.

Best regards,
Ilya
diff mbox series

Patch

diff --git a/gcc/asan.c b/gcc/asan.c
index 605d04f87f7..c436f437375 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1523,6 +1523,7 @@  asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
   DECL_INITIAL (decl) = decl;
   TREE_ASM_WRITTEN (decl) = 1;
   TREE_ASM_WRITTEN (id) = 1;
+  SET_DECL_ALIGN (decl, targetm.insn_alignment);
   emit_move_insn (mem, expand_normal (build_fold_addr_expr (decl)));
   shadow_base = expand_binop (Pmode, lshr_optab, base,
 			      gen_int_shift_amount (Pmode, ASAN_SHADOW_SHIFT),
diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index 5ec26a0592b..7ac2c8bdf76 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -16651,6 +16651,9 @@  s390_sched_dependencies_evaluation (rtx_insn *head, rtx_insn *tail)
 #undef TARGET_MAX_ANCHOR_OFFSET
 #define TARGET_MAX_ANCHOR_OFFSET 0xfff
 
+#undef TARGET_INSN_ALIGNMENT
+#define TARGET_INSN_ALIGNMENT 16
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 
 #include "gt-s390.h"
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 14c1ea6a323..142c7c04c46 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -1464,6 +1464,13 @@  appropriate for a target that does not define any new fundamental
 types.
 @end deftypefn
 
+@deftypevr {Target Hook} HOST_WIDE_INT TARGET_INSN_ALIGNMENT
+Certain architectures require individual machine instructions to
+be aligned - e.g. on a 4-byte boundary on arm, mips and ppc, or
+on a 2-byte boundary on s390.  Define this to specify such
+instruction alignment in bits.  The default value is 8.
+@end deftypevr
+
 @node Type Layout
 @section Layout of Source Language Data Types
 
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index b4d57b86e2f..97578d6de27 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -1282,6 +1282,8 @@  pattern needs to support both a 32- and a 64-bit mode.
 
 @hook TARGET_MANGLE_TYPE
 
+@hook TARGET_INSN_ALIGNMENT
+
 @node Type Layout
 @section Layout of Source Language Data Types
 
diff --git a/gcc/target.def b/gcc/target.def
index 41654054ad8..1f6fac4b830 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -3185,6 +3185,15 @@  DEFHOOK
  bool, (struct ao_ref *ref),
  default_ref_may_alias_errno)
 
+/* Machine instruction alignment.  */
+DEFHOOKPOD
+(insn_alignment,
+ "Certain architectures require individual machine instructions to\n\
+be aligned - e.g. on a 4-byte boundary on arm, mips and ppc, or\n\
+on a 2-byte boundary on s390.  Define this to specify such\n\
+instruction alignment in bits.  The default value is 8.",
+ HOST_WIDE_INT, 8)
+
 /* Support for named address spaces.  */
 #undef HOOK_PREFIX
 #define HOOK_PREFIX "TARGET_ADDR_SPACE_"
diff --git a/gcc/testsuite/gcc.target/s390/asan-no-gotoff.c b/gcc/testsuite/gcc.target/s390/asan-no-gotoff.c
new file mode 100644
index 00000000000..f555e4e96f8
--- /dev/null
+++ b/gcc/testsuite/gcc.target/s390/asan-no-gotoff.c
@@ -0,0 +1,15 @@ 
+/* Test that ASAN labels are referenced without unnecessary indirections.  */
+
+/* { dg-do compile } */
+/* { dg-options "-fPIE -O2 -fsanitize=kernel-address --param asan-stack=1" } */
+
+extern void c (int *);
+
+void a ()
+{
+  int b;
+  c (&b);
+}
+
+/* { dg-final { scan-assembler {\tlarl\t%r\d+,\.LASANPC\d+} } } */
+/* { dg-final { scan-assembler-not {\.LASANPC\d+@GOTOFF} } } */