Message ID | 20190702085154.26981-1-iii@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | S/390: Improve storing asan frame_pc | expand |
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
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
> 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
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
> 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 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
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
> 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 --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} } } */