| Message ID | 20240102194511.3171559-3-iii@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | asan: Align .LASANPC on function boundary | expand |
On 1/2/24 12:41, Ilya Leoshkevich wrote: > GCC can emit code between the function label and the .LASANPC label, > making the latter unaligned. Some architectures cannot load unaligned > labels directly and require literal pool entries, which is inefficient. > > Move the invocation of asan_function_start to > ASM_OUTPUT_FUNCTION_LABEL, which guarantees that no additional code is > emitted. This allows setting the .LASANPC label alignment to the > respective function alignment. > --- > gcc/asan.cc | 6 ++---- > gcc/config/i386/i386.cc | 2 +- > gcc/config/s390/s390.cc | 2 +- > gcc/defaults.h | 2 +- > gcc/final.cc | 3 --- > gcc/output.h | 4 ++++ > gcc/varasm.cc | 14 ++++++++++++++ > 7 files changed, 23 insertions(+), 10 deletions(-) So this needs a ChangeLog obviously. I assume you've tested on s390[x]. It should also be tested on x86 since it's the only other platform that redefined ASM_OUTPUT_FUNCTION_LABEL. Assuming those tests pass without regression, then this is fine for the trunk. Thanks, Jeff
On Tue, 2024-01-09 at 11:55 -0700, Jeff Law wrote: > > > On 1/2/24 12:41, Ilya Leoshkevich wrote: > > GCC can emit code between the function label and the .LASANPC > > label, > > making the latter unaligned. Some architectures cannot load > > unaligned > > labels directly and require literal pool entries, which is > > inefficient. > > > > Move the invocation of asan_function_start to > > ASM_OUTPUT_FUNCTION_LABEL, which guarantees that no additional code > > is > > emitted. This allows setting the .LASANPC label alignment to the > > respective function alignment. > > --- > > gcc/asan.cc | 6 ++---- > > gcc/config/i386/i386.cc | 2 +- > > gcc/config/s390/s390.cc | 2 +- > > gcc/defaults.h | 2 +- > > gcc/final.cc | 3 --- > > gcc/output.h | 4 ++++ > > gcc/varasm.cc | 14 ++++++++++++++ > > 7 files changed, 23 insertions(+), 10 deletions(-) > So this needs a ChangeLog obviously. I assume you've tested on > s390[x]. > It should also be tested on x86 since it's the only other platform > that redefined ASM_OUTPUT_FUNCTION_LABEL. > > Assuming those tests pass without regression, then this is fine for > the > trunk. > > Thanks, > Jeff Hi Jeff, Since Jakub already approved this 2/2, you approved 1/2, and x86_64/ppc64le/s390x regtests were successful, I've already pushed this series (with ChangeLogs). Unfortunately people discovered two regressions on i686 [1] and ppc64be [2]. The first one is already sorted out, I'm currently regtesting the fix for the second one and will push it as soon as it's done. Best regards, Ilya [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113251 [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113284
diff --git a/gcc/asan.cc b/gcc/asan.cc index 8d0ffb497cc..48738244aba 100644 --- a/gcc/asan.cc +++ b/gcc/asan.cc @@ -1481,10 +1481,7 @@ asan_clear_shadow (rtx shadow_mem, HOST_WIDE_INT len) void asan_function_start (void) { - section *fnsec = function_section (current_function_decl); - switch_to_section (fnsec); - ASM_OUTPUT_DEBUG_LABEL (asm_out_file, "LASANPC", - current_function_funcdef_no); + ASM_OUTPUT_DEBUG_LABEL (asm_out_file, "LASANPC", current_function_funcdef_no); } /* Return number of shadow bytes that are occupied by a local variable @@ -2006,6 +2003,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; + DECL_ALIGN_RAW (decl) = DECL_ALIGN_RAW (current_function_decl); 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/i386/i386.cc b/gcc/config/i386/i386.cc index 38d515dac04..09fc2b63ee3 100644 --- a/gcc/config/i386/i386.cc +++ b/gcc/config/i386/i386.cc @@ -1640,7 +1640,7 @@ ix86_asm_output_function_label (FILE *out_file, const char *fname, SUBTARGET_ASM_UNWIND_INIT (out_file); #endif - ASM_OUTPUT_LABEL (out_file, fname); + assemble_function_label_raw (out_file, fname); /* Output magic byte marker, if hot-patch attribute is set. */ if (is_ms_hook) diff --git a/gcc/config/s390/s390.cc b/gcc/config/s390/s390.cc index a5c36b43972..c871a10506a 100644 --- a/gcc/config/s390/s390.cc +++ b/gcc/config/s390/s390.cc @@ -8323,7 +8323,7 @@ s390_asm_output_function_label (FILE *out_file, const char *fname, asm_fprintf (out_file, "\t# fn:%s wd%d\n", fname, s390_warn_dynamicstack_p); } - ASM_OUTPUT_LABEL (out_file, fname); + assemble_function_label_raw (out_file, fname); if (hw_after > 0) asm_fprintf (out_file, "\t# post-label NOPs for hotpatch (%d halfwords)\n", diff --git a/gcc/defaults.h b/gcc/defaults.h index 6f095969410..b76734908cd 100644 --- a/gcc/defaults.h +++ b/gcc/defaults.h @@ -150,7 +150,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see #ifndef ASM_OUTPUT_FUNCTION_LABEL #define ASM_OUTPUT_FUNCTION_LABEL(FILE, NAME, DECL) \ - ASM_OUTPUT_LABEL ((FILE), (NAME)) + assemble_function_label_raw ((FILE), (NAME)) #endif /* Output the definition of a compiler-generated label named NAME. */ diff --git a/gcc/final.cc b/gcc/final.cc index e6f1b1e166b..5e21aedf8ed 100644 --- a/gcc/final.cc +++ b/gcc/final.cc @@ -1686,9 +1686,6 @@ final_start_function_1 (rtx_insn **firstp, FILE *file, int *seen, high_block_linenum = high_function_linenum = last_linenum; - if (flag_sanitize & SANITIZE_ADDRESS) - asan_function_start (); - rtx_insn *first = *firstp; if (in_initial_view_p (first)) { diff --git a/gcc/output.h b/gcc/output.h index 76cfd58c1e6..bfdecc5ea74 100644 --- a/gcc/output.h +++ b/gcc/output.h @@ -178,6 +178,10 @@ extern void assemble_asm (tree); /* Get the function's name from a decl, as described by its RTL. */ extern const char *get_fnname_from_decl (tree); +/* Output function label, possibly with accompanying metadata. No additional + code or data is output after the label. */ +extern void assemble_function_label_raw (FILE *, const char *); + /* Output assembler code for the constant pool of a function and associated with defining the name of the function. DECL describes the function. NAME is the function's name. For the constant pool, we use the current diff --git a/gcc/varasm.cc b/gcc/varasm.cc index 69f8f8ee018..d0d670d009c 100644 --- a/gcc/varasm.cc +++ b/gcc/varasm.cc @@ -61,6 +61,7 @@ along with GCC; see the file COPYING3. If not see #include "alloc-pool.h" #include "toplev.h" #include "opts.h" +#include "asan.h" /* The (assembler) name of the first globally-visible object output. */ extern GTY(()) const char *first_global_object_name; @@ -1835,6 +1836,19 @@ get_fnname_from_decl (tree decl) return XSTR (x, 0); } +/* Output function label, possibly with accompanying metadata. No additional + code or data is output after the label. */ + +void +assemble_function_label_raw (FILE *file, const char *name) +{ + ASM_OUTPUT_LABEL (file, name); + if ((flag_sanitize & SANITIZE_ADDRESS) + /* Notify ASAN only about the first function label. */ + && (in_cold_section_p == first_function_block_is_cold)) + asan_function_start (); +} + /* Output assembler code for the constant pool of a function and associated with defining the name of the function. DECL describes the function. NAME is the function's name. For the constant pool, we use the current