[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
Related show

Commit Message

Jakub Jelinek via Gcc-patches July 1, 2020, 12:29 p.m.
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(+)

-- 
2.25.4

Comments

Jakub Jelinek via Gcc-patches July 1, 2020, 5:57 p.m. | #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
>
Jakub Jelinek via Gcc-patches July 1, 2020, 7:48 p.m. | #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?".
Jakub Jelinek via Gcc-patches July 9, 2020, 12:07 p.m. | #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

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,