Re: Patch ping (stage1-ish patches)
On 11/28/13 00:17, Jakub Jelinek wrote: On Wed, Nov 27, 2013 at 01:11:59PM -0700, Jeff Law wrote: On 11/27/13 00:36, Jakub Jelinek wrote: Use libbacktrace for libsanitizer's symbolization (will need tweaking, depending on next libsanitizer merge, whether the corresponding sanitizer_common changes are upstreamed or not, and perhaps to compile libbacktrace sources again with renamed function names and other tweaks - different allocator, only subset of files, etc.; but, there is a P1 bug for this anyway): http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02055.html Isn't libsanitizer maintained outside GCC? In which case making significant changes of this nature ought to be avoided. libsanitizer contains some files imported from upstream (pretty much all of *.cc and *.h) and the rest (configury/Makefiles etc.) is owned by GCC, as the LLVM buildsystem is very different. OK. I actually kindof came to the same conclusion while looking at other sanitizer library patches. The changes to the *.cc/*.h files actally have been committed to upstream, so a next merge from upstream will bring those changes automatically and we'll just need the build system etc. changes. When that happens (I think Kostya said he'll work on that), I'll update the patch accordingly. OK. Go ahead and check it in then. Thanks for clarifying things, jeff
Re: Patch ping (stage1-ish patches)
On Wed, Nov 27, 2013 at 01:06:06PM -0700, Jeff Law wrote:
+ HOST_WIDE_INT offset, sz;
+ sz = ASAN_RED_ZONE_SIZE;
+ sz = data.asan_vec[0] - prev_offset;
Seems to me like the first assignment to sz is dead. Clearly
something isn't right here.
Thanks for catching that out, yeah, the above is from reusing
the sz variable for both red zone size (what is being computed)
and a helper temporary for the total size of asan stack frame so far
that is needed 3 times in the computation.
I've used new redzonesz var for the former to make it clearer.
Here is what I've committed in the end after retesting it on x86_64-linux.
2013-11-28 Jakub Jelinek ja...@redhat.com
* cfgexpand.c (struct stack_vars_data): Add asan_base and asan_alignb
fields.
(expand_stack_vars): For -fsanitize=address, use (and set initially)
data-asan_base as base for vars and update asan_alignb.
(expand_used_vars): Initialize data.asan_base and data.asan_alignb.
Pass them to asan_emit_stack_protection.
* asan.c (asan_detect_stack_use_after_return): New variable.
(asan_emit_stack_protection): Add pbase and alignb arguments.
Implement use after return sanitization.
* asan.h (asan_emit_stack_protection): Adjust prototype.
(ASAN_STACK_MAGIC_USE_AFTER_RET, ASAN_STACK_RETIRED_MAGIC): Define.
--- gcc/asan.c.jj 2013-11-27 18:02:47.984814523 +0100
+++ gcc/asan.c 2013-11-28 08:36:28.740704722 +0100
@@ -237,6 +237,9 @@ alias_set_type asan_shadow_set = -1;
alias set is used for all shadow memory accesses. */
static GTY(()) tree shadow_ptr_types[2];
+/* Decl for __asan_option_detect_stack_use_after_return. */
+static GTY(()) tree asan_detect_stack_use_after_return;
+
/* Hashtable support for memory references used by gimple
statements. */
@@ -950,20 +953,26 @@ asan_function_start (void)
and DECLS is an array of representative decls for each var partition.
LENGTH is the length of the OFFSETS array, DECLS array is LENGTH / 2 - 1
elements long (OFFSETS include gap before the first variable as well
- as gaps after each stack variable). */
+ as gaps after each stack variable). PBASE is, if non-NULL, some pseudo
+ register which stack vars DECL_RTLs are based on. Either BASE should be
+ assigned to PBASE, when not doing use after return protection, or
+ corresponding address based on __asan_stack_malloc* return value. */
rtx
-asan_emit_stack_protection (rtx base, HOST_WIDE_INT *offsets, tree *decls,
- int length)
+asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
+ HOST_WIDE_INT *offsets, tree *decls, int length)
{
- rtx shadow_base, shadow_mem, ret, mem;
+ rtx shadow_base, shadow_mem, ret, mem, orig_base, lab;
char buf[30];
unsigned char shadow_bytes[4];
- HOST_WIDE_INT base_offset = offsets[length - 1], offset, prev_offset;
+ HOST_WIDE_INT base_offset = offsets[length - 1];
+ HOST_WIDE_INT base_align_bias = 0, offset, prev_offset;
+ HOST_WIDE_INT asan_frame_size = offsets[0] - base_offset;
HOST_WIDE_INT last_offset, last_size;
int l;
unsigned char cur_shadow_byte = ASAN_STACK_MAGIC_LEFT;
tree str_cst, decl, id;
+ int use_after_return_class = -1;
if (shadow_ptr_types[0] == NULL_TREE)
asan_init_shadow_ptr_types ();
@@ -993,10 +1002,67 @@ asan_emit_stack_protection (rtx base, HO
str_cst = asan_pp_string (asan_pp);
/* Emit the prologue sequence. */
+ if (asan_frame_size 32 asan_frame_size = 65536 pbase)
+{
+ use_after_return_class = floor_log2 (asan_frame_size - 1) - 5;
+ /* __asan_stack_malloc_N guarantees alignment
+ N 6 ? (64 N) : 4096 bytes. */
+ if (alignb (use_after_return_class 6
+ ? (64U use_after_return_class) : 4096U))
+ use_after_return_class = -1;
+ else if (alignb ASAN_RED_ZONE_SIZE (asan_frame_size (alignb - 1)))
+ base_align_bias = ((asan_frame_size + alignb - 1)
+ ~(alignb - HOST_WIDE_INT_1)) - asan_frame_size;
+}
+ if (use_after_return_class == -1 pbase)
+emit_move_insn (pbase, base);
base = expand_binop (Pmode, add_optab, base,
- gen_int_mode (base_offset, Pmode),
+ gen_int_mode (base_offset - base_align_bias, Pmode),
NULL_RTX, 1, OPTAB_DIRECT);
+ orig_base = NULL_RTX;
+ if (use_after_return_class != -1)
+{
+ if (asan_detect_stack_use_after_return == NULL_TREE)
+ {
+ id = get_identifier (__asan_option_detect_stack_use_after_return);
+ decl = build_decl (BUILTINS_LOCATION, VAR_DECL, id,
+integer_type_node);
+ SET_DECL_ASSEMBLER_NAME (decl, id);
+ TREE_ADDRESSABLE (decl) = 1;
+ DECL_ARTIFICIAL (decl) = 1;
+ DECL_IGNORED_P (decl) = 1;
+ DECL_EXTERNAL (decl) = 1;
+ TREE_STATIC (decl) = 1;
+
Re: Patch ping (stage1-ish patches)
Hi Jeff, On my side, there's [c++, driver] Add -lrt on Solaris http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01488.html resubmitted as http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00412.html It's unclear if the more intrusive solution outlined in the second message (introduce libstdc++.spec) were acceptable in stage3, and I'm uncertain if I can get it ready in time. Well, the short-term hack to g++spec.c along with the corresponding change to sol2.h is, OK for the trunk. thanks, I've just installed it as a stopgap measure. As for the more invasive change, I'd let the C++ runtime guys decide if its too invasive for stage3. If you go that route, worst case is it's considered too invasive and it goes in during stage1 and you can remove the hack-ish solution from this patch. Right. I just remembered that something along this line will be needed for Solaris 10, too, which unlike Solaris 9 won't be removed in GCC 4.10. I'll see how far I get with a libstdc++.spec patch and than let the C++ maintainers decide what to do for 4.9. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
Re: Patch ping (stage1-ish patches)
Here is the patch series that had been posted in Sep that is aimed to isolate the Android support from targets that actually don't have that support (We discussed the need of it with Jakub here http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00185.html): http://gcc.gnu.org/ml/gcc-patches/2013-09/msg01420.html thanks, Alexander 2013/11/27 Jakub Jelinek ja...@redhat.com: On Fri, Nov 22, 2013 at 09:36:18AM -0700, Jeff Law wrote: In fact, I would suggest that anyone with a pending patch from prior to stage1 close that hasn't gotten feedback by midnight Tuesday ping their patch. I'd like to have a sense of everything that is outstanding sooner rather than later and wrap up any loose ends as quickly as possible. Ok, doing that now for my pending patches: Masked load/store vectorization: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01268.html http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01437.html http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01550.html Elemental function support (updated version of the earlier patch): http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03197.html AddressSanitizer use-after-return instrumentation: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02070.html Use libbacktrace for libsanitizer's symbolization (will need tweaking, depending on next libsanitizer merge, whether the corresponding sanitizer_common changes are upstreamed or not, and perhaps to compile libbacktrace sources again with renamed function names and other tweaks - different allocator, only subset of files, etc.; but, there is a P1 bug for this anyway): http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02055.html Jakub
Re: Patch ping (stage1-ish patches)
Jakub Jelinek ja...@redhat.com writes: On Fri, Nov 22, 2013 at 09:36:18AM -0700, Jeff Law wrote: In fact, I would suggest that anyone with a pending patch from prior to stage1 close that hasn't gotten feedback by midnight Tuesday ping their patch. I'd like to have a sense of everything that is outstanding sooner rather than later and wrap up any loose ends as quickly as possible. On my side, there's [c++, driver] Add -lrt on Solaris http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01488.html resubmitted as http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00412.html It's unclear if the more intrusive solution outlined in the second message (introduce libstdc++.spec) were acceptable in stage3, and I'm uncertain if I can get it ready in time. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
Patch ping (stage1-ish patches)
In fact, I would suggest that anyone with a pending patch from prior to stage1 close that hasn't gotten feedback by midnight Tuesday ping their patch. I'd like to have a sense of everything that is outstanding sooner rather than later and wrap up any loose ends as quickly as possible. Improve debug info for small structures (2) http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02007.html -- Eric Botcazou
Re: Patch ping (stage1-ish patches)
On 11/27/13 05:30, Eric Botcazou wrote: In fact, I would suggest that anyone with a pending patch from prior to stage1 close that hasn't gotten feedback by midnight Tuesday ping their patch. I'd like to have a sense of everything that is outstanding sooner rather than later and wrap up any loose ends as quickly as possible. Improve debug info for small structures (2) http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02007.html This is fine. Sorry about the delay. jeff
Re: Patch ping (stage1-ish patches)
On 11/27/13 04:48, Rainer Orth wrote: Jakub Jelinek ja...@redhat.com writes: On Fri, Nov 22, 2013 at 09:36:18AM -0700, Jeff Law wrote: In fact, I would suggest that anyone with a pending patch from prior to stage1 close that hasn't gotten feedback by midnight Tuesday ping their patch. I'd like to have a sense of everything that is outstanding sooner rather than later and wrap up any loose ends as quickly as possible. On my side, there's [c++, driver] Add -lrt on Solaris http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01488.html resubmitted as http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00412.html It's unclear if the more intrusive solution outlined in the second message (introduce libstdc++.spec) were acceptable in stage3, and I'm uncertain if I can get it ready in time. Well, the short-term hack to g++spec.c along with the corresponding change to sol2.h is, OK for the trunk. As for the more invasive change, I'd let the C++ runtime guys decide if its too invasive for stage3. If you go that route, worst case is it's considered too invasive and it goes in during stage1 and you can remove the hack-ish solution from this patch. jeff
Re: Patch ping (stage1-ish patches)
On 11/27/13 01:28, Alexander Ivchenko wrote: Here is the patch series that had been posted in Sep that is aimed to isolate the Android support from targets that actually don't have that support (We discussed the need of it with Jakub here http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00185.html): http://gcc.gnu.org/ml/gcc-patches/2013-09/msg01420.html This patch series is fine. Just a minor typo in patch #4: +/* IFUNCs are supportted by glibc, but not by uClibc or Bionic. */ s/supportted/supported/ jeff
Re: Patch ping (stage1-ish patches)
On 11/27/13 00:36, Jakub Jelinek wrote:
AddressSanitizer use-after-return instrumentation:
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02070.html
+ HOST_WIDE_INT offset, sz;
+ sz = ASAN_RED_ZONE_SIZE;
+ sz = data.asan_vec[0] - prev_offset;
Seems to me like the first assignment to sz is dead. Clearly something
isn't right here.
In fact, the whole fragment seems a bit wonky in that you set sz prior
to the conditional, use it in the conditional, then set it in both arms.
I'm guessing that structure is to simplify the conditional, which is
fine. In fact, I would hazard a guess the dead assignment is a result
of trying to clean things up in the conditional.
+ HOST_WIDE_INT offset, sz;
+ sz = ASAN_RED_ZONE_SIZE;
+ sz = data.asan_vec[0] - prev_offset;
+ if (data.asan_alignb ASAN_RED_ZONE_SIZE
+ data.asan_alignb = 4096
+ sz + ASAN_RED_ZONE_SIZE = data.asan_alignb)
+ {
+ sz = ((sz + ASAN_RED_ZONE_SIZE + data.asan_alignb - 1)
+~(data.asan_alignb - HOST_WIDE_INT_1)) - sz;
+ }
+ else
+ sz = ASAN_RED_ZONE_SIZE;
+ offset
+ = alloc_stack_frame_space (sz, ASAN_RED_ZONE_SIZE);
I'm assuming that the code you're generating to interface with the ubsan
libraries is sane -- I don't know those APIs at all. I trust that if
there's an issue you'll address is appropriately.
With the fragment above fixed, this is OK.
jeff
Re: Patch ping (stage1-ish patches)
On 11/27/13 00:36, Jakub Jelinek wrote: Use libbacktrace for libsanitizer's symbolization (will need tweaking, depending on next libsanitizer merge, whether the corresponding sanitizer_common changes are upstreamed or not, and perhaps to compile libbacktrace sources again with renamed function names and other tweaks - different allocator, only subset of files, etc.; but, there is a P1 bug for this anyway): http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02055.html Isn't libsanitizer maintained outside GCC? In which case making significant changes of this nature ought to be avoided. While I see the benefit in what you're doing, I question if we want to go down this road. Or is it the case that the build stuff in libsanitizer is ours and the only shared bits you'r hacking up are sanitizer_symbolizer_posix_libcdep.cc? jeff
Re: Patch ping (stage1-ish patches)
On 11/27/13 00:36, Jakub Jelinek wrote: Elemental function support (updated version of the earlier patch): http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03197.html Richi OK's this earlier today. So it's good to go, right? I thought I saw some follow-up items that Richi agreed could be done later, right? Jeff
Re: Patch ping (stage1-ish patches)
On Wed, Nov 27, 2013 at 01:26:54PM -0700, Jeff Law wrote: On 11/27/13 00:36, Jakub Jelinek wrote: Elemental function support (updated version of the earlier patch): http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03197.html Richi OK's this earlier today. So it's good to go, right? I thought I saw some follow-up items that Richi agreed could be done later, right? I've committed it already, and Richard has committed some related LTO fixes, will need to add omp clause LTO streaming now. Jakub
Re: Patch ping (stage1-ish patches)
On Wed, Nov 27, 2013 at 01:11:59PM -0700, Jeff Law wrote: On 11/27/13 00:36, Jakub Jelinek wrote: Use libbacktrace for libsanitizer's symbolization (will need tweaking, depending on next libsanitizer merge, whether the corresponding sanitizer_common changes are upstreamed or not, and perhaps to compile libbacktrace sources again with renamed function names and other tweaks - different allocator, only subset of files, etc.; but, there is a P1 bug for this anyway): http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02055.html Isn't libsanitizer maintained outside GCC? In which case making significant changes of this nature ought to be avoided. libsanitizer contains some files imported from upstream (pretty much all of *.cc and *.h) and the rest (configury/Makefiles etc.) is owned by GCC, as the LLVM buildsystem is very different. While I see the benefit in what you're doing, I question if we want to go down this road. Or is it the case that the build stuff in libsanitizer is ours and the only shared bits you'r hacking up are sanitizer_symbolizer_posix_libcdep.cc? The changes to the *.cc/*.h files actally have been committed to upstream, so a next merge from upstream will bring those changes automatically and we'll just need the build system etc. changes. When that happens (I think Kostya said he'll work on that), I'll update the patch accordingly. Jakub
Patch ping (stage1-ish patches)
On Fri, Nov 22, 2013 at 09:36:18AM -0700, Jeff Law wrote: In fact, I would suggest that anyone with a pending patch from prior to stage1 close that hasn't gotten feedback by midnight Tuesday ping their patch. I'd like to have a sense of everything that is outstanding sooner rather than later and wrap up any loose ends as quickly as possible. Ok, doing that now for my pending patches: Masked load/store vectorization: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01268.html http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01437.html http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01550.html Elemental function support (updated version of the earlier patch): http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03197.html AddressSanitizer use-after-return instrumentation: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02070.html Use libbacktrace for libsanitizer's symbolization (will need tweaking, depending on next libsanitizer merge, whether the corresponding sanitizer_common changes are upstreamed or not, and perhaps to compile libbacktrace sources again with renamed function names and other tweaks - different allocator, only subset of files, etc.; but, there is a P1 bug for this anyway): http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02055.html Jakub
