diff mbox series

[v4,1/2] asan: specify alignment for LASANPC labels

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

Commit Message

Ilya Leoshkevich July 1, 2020, 12:29 p.m. UTC 
gcc/ChangeLog:

2020-06-30  Ilya Leoshkevich  <iii@linux.ibm.com>

	* asan.c (asan_emit_stack_protection): Use CODE_LABEL_BOUNDARY.
	* defaults.h (CODE_LABEL_BOUNDARY): New macro.
	* doc/tm.texi: Document CODE_LABEL_BOUNDARY.
	* doc/tm.texi.in: Likewise.
---
 gcc/asan.c         | 1 +
 gcc/defaults.h     | 5 +++++
 gcc/doc/tm.texi    | 4 ++++
 gcc/doc/tm.texi.in | 4 ++++
 4 files changed, 14 insertions(+)

Comments

Li, Pan2 via Gcc-patches July 1, 2020, 5:57 p.m. UTC | #1 
On Wed, 2020-07-01 at 14:29 +0200, Ilya Leoshkevich via Gcc-patches wrote:
> gcc/ChangeLog:
> 
> 2020-06-30  Ilya Leoshkevich  <iii@linux.ibm.com>
> 
> 	* asan.c (asan_emit_stack_protection): Use CODE_LABEL_BOUNDARY.
> 	* defaults.h (CODE_LABEL_BOUNDARY): New macro.
> 	* doc/tm.texi: Document CODE_LABEL_BOUNDARY.
> 	* doc/tm.texi.in: Likewise.
Don't we already have the ability to set label alignments?  See LABEL_ALIGN.



jeff
>
Ilya Leoshkevich July 1, 2020, 7:48 p.m. UTC | #2 
On Wed, 2020-07-01 at 11:57 -0600, Jeff Law wrote:
> On Wed, 2020-07-01 at 14:29 +0200, Ilya Leoshkevich via Gcc-patches
> wrote:
> > gcc/ChangeLog:
> > 
> > 2020-06-30  Ilya Leoshkevich  <iii@linux.ibm.com>
> > 
> > 	* asan.c (asan_emit_stack_protection): Use CODE_LABEL_BOUNDARY.
> > 	* defaults.h (CODE_LABEL_BOUNDARY): New macro.
> > 	* doc/tm.texi: Document CODE_LABEL_BOUNDARY.
> > 	* doc/tm.texi.in: Likewise.
> Don't we already have the ability to set label alignments?  See
> LABEL_ALIGN.

The following works with -falign-labels=2:

--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1524,7 +1524,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, CODE_LABEL_BOUNDARY);
+  SET_DECL_ALIGN (decl, (1 << LABEL_ALIGN (gen_label_rtx ())) *
BITS_PER_UNIT);
   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),

In order to go this way, we would need to raise `-falign-labels=`
default to 2 for s390, which is not incorrect, but would unnecessarily
clutter asm with `.align 2` before each label.  So IMHO it would be
nicer to simply ask the backend "what is your target's instruction
alignment?".
Ilya Leoshkevich July 9, 2020, 12:07 p.m. UTC | #3 
On Wed, 2020-07-01 at 21:48 +0200, Ilya Leoshkevich wrote:
> On Wed, 2020-07-01 at 11:57 -0600, Jeff Law wrote:
> > On Wed, 2020-07-01 at 14:29 +0200, Ilya Leoshkevich via Gcc-patches
> > wrote:
> > > gcc/ChangeLog:
> > > 
> > > 2020-06-30  Ilya Leoshkevich  <iii@linux.ibm.com>
> > > 
> > > 	* asan.c (asan_emit_stack_protection): Use CODE_LABEL_BOUNDARY.
> > > 	* defaults.h (CODE_LABEL_BOUNDARY): New macro.
> > > 	* doc/tm.texi: Document CODE_LABEL_BOUNDARY.
> > > 	* doc/tm.texi.in: Likewise.
> > Don't we already have the ability to set label alignments?  See
> > LABEL_ALIGN.
> 
> The following works with -falign-labels=2:
> 
> --- a/gcc/asan.c
> +++ b/gcc/asan.c
> @@ -1524,7 +1524,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, CODE_LABEL_BOUNDARY);
> +  SET_DECL_ALIGN (decl, (1 << LABEL_ALIGN (gen_label_rtx ())) *
> BITS_PER_UNIT);
>    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),
> 
> In order to go this way, we would need to raise `-falign-labels=`
> default to 2 for s390, which is not incorrect, but would
> unnecessarily
> clutter asm with `.align 2` before each label.  So IMHO it would be
> nicer to simply ask the backend "what is your target's instruction
> alignment?".

Besides that it would clutter asm with .align 2, another argument
against using LABEL_ALIGN here is that it's semantically different from
what is needed: -falign-labels value, which it returns, is specified by
user for optimization purposes, whereas here we need to query the
architecture's property.

In practical terms, if user specifies -falign-labels=4096, this would
affect how the code is generated here. However, this would be
completely unnecessary: we never jump to decl, its address is only
saved for reporting.

Best regards,
Ilya
Ilya Leoshkevich Dec. 9, 2020, 1:44 a.m. UTC | #4 
On Thu, 2020-07-09 at 14:07 +0200, Ilya Leoshkevich wrote:
> On Wed, 2020-07-01 at 21:48 +0200, Ilya Leoshkevich wrote:
> > On Wed, 2020-07-01 at 11:57 -0600, Jeff Law wrote:
> > > On Wed, 2020-07-01 at 14:29 +0200, Ilya Leoshkevich via Gcc-
> > > patches
> > > wrote:
> > > > gcc/ChangeLog:
> > > > 
> > > > 2020-06-30  Ilya Leoshkevich  <iii@linux.ibm.com>
> > > > 
> > > > 	* asan.c (asan_emit_stack_protection): Use
> > > > CODE_LABEL_BOUNDARY.
> > > > 	* defaults.h (CODE_LABEL_BOUNDARY): New macro.
> > > > 	* doc/tm.texi: Document CODE_LABEL_BOUNDARY.
> > > > 	* doc/tm.texi.in: Likewise.
> > > Don't we already have the ability to set label alignments?  See
> > > LABEL_ALIGN.
> > 
> > The following works with -falign-labels=2:
> > 
> > --- a/gcc/asan.c
> > +++ b/gcc/asan.c
> > @@ -1524,7 +1524,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, CODE_LABEL_BOUNDARY);
> > +  SET_DECL_ALIGN (decl, (1 << LABEL_ALIGN (gen_label_rtx ())) *
> > BITS_PER_UNIT);
> >    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),
> > 
> > In order to go this way, we would need to raise `-falign-labels=`
> > default to 2 for s390, which is not incorrect, but would
> > unnecessarily
> > clutter asm with `.align 2` before each label.  So IMHO it would be
> > nicer to simply ask the backend "what is your target's instruction
> > alignment?".
> 
> Besides that it would clutter asm with .align 2, another argument
> against using LABEL_ALIGN here is that it's semantically different
> from
> what is needed: -falign-labels value, which it returns, is specified
> by
> user for optimization purposes, whereas here we need to query the
> architecture's property.
> 
> In practical terms, if user specifies -falign-labels=4096, this would
> affect how the code is generated here. However, this would be
> completely unnecessary: we never jump to decl, its address is only
> saved for reporting.

Hi Jeff,

Could you please have another look at this one?

Best regards,
Ilya
diff mbox series

Patch

diff --git a/gcc/asan.c b/gcc/asan.c
index 9c9aa4cae35..cc06afb9ddc 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1524,6 +1524,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, CODE_LABEL_BOUNDARY);
   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/defaults.h b/gcc/defaults.h
index f1a38626624..e5a9139bbbe 100644
--- a/gcc/defaults.h
+++ b/gcc/defaults.h
@@ -1473,4 +1473,9 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 typedef TARGET_UNIT target_unit;
 #endif
 
+/* Alignment required for a code label, in bits.  */
+#ifndef CODE_LABEL_BOUNDARY
+#define CODE_LABEL_BOUNDARY BITS_PER_UNIT
+#endif
+
 #endif  /* ! GCC_DEFAULTS_H */
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 6e7d9dc54a9..16e48ce59d8 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -1011,6 +1011,10 @@  to a value equal to or larger than @code{STACK_BOUNDARY}.
 Alignment required for a function entry point, in bits.
 @end defmac
 
+@defmac CODE_LABEL_BOUNDARY
+Alignment required for a code label, in bits.
+@end defmac
+
 @defmac BIGGEST_ALIGNMENT
 Biggest alignment that any data type can require on this machine, in
 bits.  Note that this is not the biggest alignment that is supported,
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 3be984bbd5c..12f8c05f5dd 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -965,6 +965,10 @@  to a value equal to or larger than @code{STACK_BOUNDARY}.
 Alignment required for a function entry point, in bits.
 @end defmac
 
+@defmac CODE_LABEL_BOUNDARY
+Alignment required for a code label, in bits.
+@end defmac
+
 @defmac BIGGEST_ALIGNMENT
 Biggest alignment that any data type can require on this machine, in
 bits.  Note that this is not the biggest alignment that is supported,