This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Come up with --param asan-stack-small-redzone (PR sanitizer/81715).
On 9/25/18 5:53 PM, Jakub Jelinek wrote:
> On Tue, Sep 25, 2018 at 05:26:44PM +0200, Martin Liška wrote:
>> The only missing piece is how to implement asan_emit_redzone_payload more smart.
>> It means doing memory stores with 8,4,2,1 sizes in order to reduce # of insns.
>> Do we have somewhere a similar code?
>
> Yeah, that is a very important optimization. I wasn't using DImode because
> at least on x86_64 64-bit constants are quite expensive and on several other
> targets even more so, so SImode was a compromise to get size of the prologue
> under control and not very slow. What I think we want is figure out ranges
Ah, some time ago, I remember you mentioned the 64-bit constants are expensive
(even on x86_64). Btw. it's what clang used for the red zone instrumentation.
> of shadow bytes we want to initialize and the values we want to store there,
> perhaps take also into account strict alignment vs. non-strict alignment,
> and perform kind of store merging for it. Given that 2 shadow bytes would
> be only used for the very small variables (<=4 bytes in size, so <= 0.5
> bytes of shadow), we'd just need a way to remember the 2 shadow bytes across
> handling adjacent vars and store it together.
Agree, it's implemented in next version of patch.
>
> I think we want to introduce some define for minimum red zone size and use
> it instead of the granularity (granularity is 8 bytes, but minimum red zone
> size if we count into it also the very small variable size is 16 bytes).
>
>> --- a/gcc/asan.h
>> +++ b/gcc/asan.h
>> @@ -102,6 +102,26 @@ asan_red_zone_size (unsigned int size)
>> return c ? 2 * ASAN_RED_ZONE_SIZE - c : ASAN_RED_ZONE_SIZE;
>> }
>>
>> +/* Return how much a stack variable occupy on a stack
>> + including a space for redzone. */
>> +
>> +static inline unsigned int
>> +asan_var_and_redzone_size (unsigned int size)
>
> The argument needs to be UHWI, otherwise you do a wrong thing for
> say 4GB + 4 bytes long variable. Ditto the result.
>
>> +{
>> + if (size <= 4)
>> + return 16;
>> + else if (size <= 16)
>> + return 32;
>> + else if (size <= 128)
>> + return 32 + size;
>> + else if (size <= 512)
>> + return 64 + size;
>> + else if (size <= 4096)
>> + return 128 + size;
>> + else
>> + return 256 + size;
>
> I'd prefer size + const instead of const + size operand order.
>
>> @@ -1125,13 +1125,13 @@ expand_stack_vars (bool (*pred) (size_t), struct stack_vars_data *data)
>> && stack_vars[i].size.is_constant ())
>> {
>> prev_offset = align_base (prev_offset,
>> - MAX (alignb, ASAN_RED_ZONE_SIZE),
>> + MAX (alignb, ASAN_SHADOW_GRANULARITY),
>
> Use that ASAN_MIN_RED_ZONE_SIZE (16) here.
>
>> !FRAME_GROWS_DOWNWARD);
>> tree repr_decl = NULL_TREE;
>> + poly_uint64 size = asan_var_and_redzone_size (stack_vars[i].size.to_constant ());
>
> Too long line. Two spaces instead of one. Why poly_uint64?
> Plus, perhaps if data->asan_vec is empty (i.e. when assigning the topmost
> automatic variable in a frame), we should ensure that size is at least
> 2 * ASAN_RED_ZONE_SIZE (or just 1 * ASAN_RED_ZONE_SIZE).
>
>> offset
>> - = alloc_stack_frame_space (stack_vars[i].size
>> - + ASAN_RED_ZONE_SIZE,
>> - MAX (alignb, ASAN_RED_ZONE_SIZE));
>> + = alloc_stack_frame_space (size,
>> + MAX (alignb, ASAN_SHADOW_GRANULARITY));
>
> Again, too long line and we want 16 instead of 8 here too.
>>
>> data->asan_vec.safe_push (prev_offset);
>> /* Allocating a constant amount of space from a constant
>> @@ -2254,7 +2254,7 @@ expand_used_vars (void)
>> & ~(data.asan_alignb - HOST_WIDE_INT_1)) - sz;
>> /* Allocating a constant amount of space from a constant
>> starting offset must give a constant result. */
>> - offset = (alloc_stack_frame_space (redzonesz, ASAN_RED_ZONE_SIZE)
>> + offset = (alloc_stack_frame_space (redzonesz, ASAN_SHADOW_GRANULARITY)
>
> and here too.
>
> Jakub
>
The rest is also implemented as requested. I'm testing Linux kernel now, will send
stats to the PR created for it.
Patch survives testing on x86_64-linux-gnu.
Martin
>From acbea3a2127a5eb19e23a202010847e098cf8ce8 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 25 Sep 2018 10:54:37 +0200
Subject: [PATCH] Make red zone size more flexible for stack variables (PR
sanitizer/81715).
gcc/ChangeLog:
2018-09-26 Martin Liska <mliska@suse.cz>
PR sanitizer/81715
* asan.c (asan_shadow_cst): Remove.
(asan_emit_redzone_payload): New.
(asan_emit_stack_protection): Make it more
flexible to support arbitrary size of red zones.
* asan.h (ASAN_MIN_RED_ZONE_SIZE): New.
(asan_var_and_redzone_size): Likewise.
* cfgexpand.c (expand_stack_vars): Reserve size
for stack vars according to asan_var_and_redzone_size.
(expand_used_vars): Make smaller offset based
on ASAN_SHADOW_GRANULARITY.
gcc/testsuite/ChangeLog:
2018-09-26 Martin Liska <mliska@suse.cz>
PR sanitizer/81715
* c-c++-common/asan/asan-stack-small.c: New test.
---
gcc/asan.c | 108 +++++++++++-------
gcc/asan.h | 25 ++++
gcc/cfgexpand.c | 16 ++-
.../c-c++-common/asan/asan-stack-small.c | 28 +++++
4 files changed, 132 insertions(+), 45 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/asan/asan-stack-small.c
diff --git a/gcc/asan.c b/gcc/asan.c
index 235e219479d..6552ca66132 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1155,18 +1155,34 @@ asan_pp_string (pretty_printer *pp)
return build1 (ADDR_EXPR, shadow_ptr_types[0], ret);
}
-/* Return a CONST_INT representing 4 subsequent shadow memory bytes. */
+/* Emit red zones payload that started at SHADOW_MEM address.
+ SHADOW_BYTES contains payload that should be stored. */
static rtx
-asan_shadow_cst (unsigned char shadow_bytes[4])
+asan_emit_redzone_payload (rtx shadow_mem,
+ auto_vec<unsigned char> &shadow_bytes)
{
- int i;
- unsigned HOST_WIDE_INT val = 0;
- gcc_assert (WORDS_BIG_ENDIAN == BYTES_BIG_ENDIAN);
- for (i = 0; i < 4; i++)
- val |= (unsigned HOST_WIDE_INT) shadow_bytes[BYTES_BIG_ENDIAN ? 3 - i : i]
- << (BITS_PER_UNIT * i);
- return gen_int_mode (val, SImode);
+ while (!shadow_bytes.is_empty ())
+ {
+ unsigned l = shadow_bytes.length ();
+ unsigned chunk = l >= 4 ? 4 : (l >= 2 ? 2 : 1);
+ machine_mode mode = chunk == 4 ? SImode : (chunk == 2 ? HImode : QImode);
+ shadow_mem = adjust_address (shadow_mem, mode, 0);
+
+ unsigned HOST_WIDE_INT val = 0;
+ gcc_assert (WORDS_BIG_ENDIAN == BYTES_BIG_ENDIAN);
+ for (unsigned i = 0; i < chunk; i++)
+ {
+ unsigned char v = shadow_bytes[BYTES_BIG_ENDIAN ? chunk - i : i];
+ val |= (unsigned HOST_WIDE_INT)v << (BITS_PER_UNIT * i);
+ }
+ rtx c = gen_int_mode (val, mode);
+ emit_move_insn (shadow_mem, c);
+ shadow_mem = adjust_address (shadow_mem, VOIDmode, chunk);
+ shadow_bytes.block_remove (0, chunk);
+ }
+
+ return shadow_mem;
}
/* Clear shadow memory at SHADOW_MEM, LEN bytes. Can't call a library call here
@@ -1256,7 +1272,6 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
rtx_code_label *lab;
rtx_insn *insns;
char buf[32];
- unsigned char shadow_bytes[4];
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;
@@ -1398,49 +1413,64 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
+ (base_align_bias >> ASAN_SHADOW_SHIFT));
gcc_assert (asan_shadow_set != -1
&& (ASAN_RED_ZONE_SIZE >> ASAN_SHADOW_SHIFT) == 4);
- shadow_mem = gen_rtx_MEM (SImode, shadow_base);
+ shadow_mem = gen_rtx_MEM (QImode, shadow_base);
set_mem_alias_set (shadow_mem, asan_shadow_set);
if (STRICT_ALIGNMENT)
set_mem_align (shadow_mem, (GET_MODE_ALIGNMENT (SImode)));
prev_offset = base_offset;
+
+ auto_vec<unsigned char> shadow_bytes (64);
for (l = length; l; l -= 2)
{
if (l == 2)
cur_shadow_byte = ASAN_STACK_MAGIC_RIGHT;
offset = offsets[l - 1];
- if ((offset - base_offset) & (ASAN_RED_ZONE_SIZE - 1))
+
+ bool merging_p = !shadow_bytes.is_empty ();
+ bool extra_byte = (offset - base_offset) & (ASAN_SHADOW_GRANULARITY - 1);
+ /* If a red-zone is not aligned to ASAN_SHADOW_GRANULARITY then
+ the previous stack variable has size % ASAN_SHADOW_GRANULARITY != 0.
+ In that case we have to emit one extra byte that will describe
+ how many bytes (our of ASAN_SHADOW_GRANULARITY) can be accessed. */
+ if (extra_byte)
{
- int i;
HOST_WIDE_INT aoff
= base_offset + ((offset - base_offset)
- & ~(ASAN_RED_ZONE_SIZE - HOST_WIDE_INT_1));
- shadow_mem = adjust_address (shadow_mem, VOIDmode,
- (aoff - prev_offset)
- >> ASAN_SHADOW_SHIFT);
- prev_offset = aoff;
- for (i = 0; i < 4; i++, aoff += ASAN_SHADOW_GRANULARITY)
- if (aoff < offset)
- {
- if (aoff < offset - (HOST_WIDE_INT)ASAN_SHADOW_GRANULARITY + 1)
- shadow_bytes[i] = 0;
- else
- shadow_bytes[i] = offset - aoff;
- }
- else
- shadow_bytes[i] = ASAN_STACK_MAGIC_MIDDLE;
- emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
+ & ~(ASAN_SHADOW_GRANULARITY - HOST_WIDE_INT_1));
+ shadow_bytes.quick_push (offset - aoff);
offset = aoff;
}
- while (offset <= offsets[l - 2] - ASAN_RED_ZONE_SIZE)
+
+ /* Adjust shadow memory to beginning of shadow memory
+ (or one byte earlier). */
+ if (!merging_p)
+ shadow_mem = adjust_address (shadow_mem, VOIDmode,
+ (offset - prev_offset)
+ >> ASAN_SHADOW_SHIFT);
+
+ if (extra_byte)
+ offset += ASAN_SHADOW_GRANULARITY;
+
+ /* Calculate size of red zone payload. */
+ while (offset < offsets[l - 2])
{
- shadow_mem = adjust_address (shadow_mem, VOIDmode,
- (offset - prev_offset)
- >> ASAN_SHADOW_SHIFT);
+ shadow_bytes.quick_push (cur_shadow_byte);
+ offset += ASAN_SHADOW_GRANULARITY;
+ }
+
+ /* Do simple store merging for 2 adjacent small variables
+ that will need 4 bytes in total to emit red zones. */
+ if (shadow_bytes.length () == 2
+ && l >= 3
+ && ((unsigned HOST_WIDE_INT)(offsets[l - 3] - offsets[l - 2])
+ < ASAN_SHADOW_GRANULARITY))
+ ;
+ else
+ {
+ shadow_mem = asan_emit_redzone_payload (shadow_mem, shadow_bytes);
prev_offset = offset;
- memset (shadow_bytes, cur_shadow_byte, 4);
- emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
- offset += ASAN_RED_ZONE_SIZE;
}
+
cur_shadow_byte = ASAN_STACK_MAGIC_MIDDLE;
}
do_pending_stack_adjust ();
@@ -1501,7 +1531,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
for (l = length; l; l -= 2)
{
offset = base_offset + ((offsets[l - 1] - base_offset)
- & ~(ASAN_RED_ZONE_SIZE - HOST_WIDE_INT_1));
+ & ~(ASAN_MIN_RED_ZONE_SIZE - HOST_WIDE_INT_1));
if (last_offset + last_size != offset)
{
shadow_mem = adjust_address (shadow_mem, VOIDmode,
@@ -1513,7 +1543,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
last_size = 0;
}
last_size += base_offset + ((offsets[l - 2] - base_offset)
- & ~(ASAN_RED_ZONE_SIZE - HOST_WIDE_INT_1))
+ & ~(ASAN_MIN_RED_ZONE_SIZE - HOST_WIDE_INT_1))
- offset;
/* Unpoison shadow memory that corresponds to a variable that is
@@ -1534,7 +1564,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
"%s (%" PRId64 " B)\n", n, size);
}
- last_size += size & ~(ASAN_RED_ZONE_SIZE - HOST_WIDE_INT_1);
+ last_size += size & ~(ASAN_MIN_RED_ZONE_SIZE - HOST_WIDE_INT_1);
}
}
}
diff --git a/gcc/asan.h b/gcc/asan.h
index 2f431b4f938..e1b9b491e67 100644
--- a/gcc/asan.h
+++ b/gcc/asan.h
@@ -53,6 +53,11 @@ extern hash_set <tree> *asan_used_labels;
up to 2 * ASAN_RED_ZONE_SIZE - 1 bytes. */
#define ASAN_RED_ZONE_SIZE 32
+/* Stack variable use more compact red zones. The size includes also
+ size of variable itself. */
+
+#define ASAN_MIN_RED_ZONE_SIZE 16
+
/* Shadow memory values for stack protection. Left is below protected vars,
the first pointer in stack corresponding to that offset contains
ASAN_STACK_FRAME_MAGIC word, the second pointer to a string describing
@@ -102,6 +107,26 @@ asan_red_zone_size (unsigned int size)
return c ? 2 * ASAN_RED_ZONE_SIZE - c : ASAN_RED_ZONE_SIZE;
}
+/* Return how much a stack variable occupis on a stack
+ including a space for red zone. */
+
+static inline unsigned HOST_WIDE_INT
+asan_var_and_redzone_size (unsigned HOST_WIDE_INT size)
+{
+ if (size <= 4)
+ return 16;
+ else if (size <= 16)
+ return 32;
+ else if (size <= 128)
+ return size + 32;
+ else if (size <= 512)
+ return size + 64;
+ else if (size <= 4096)
+ return size + 128;
+ else
+ return size + 256;
+}
+
extern bool set_asan_shadow_offset (const char *);
extern void set_sanitized_sections (const char *);
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 35ca276e4ad..1a1abe1f6a2 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1125,13 +1125,17 @@ expand_stack_vars (bool (*pred) (size_t), struct stack_vars_data *data)
&& stack_vars[i].size.is_constant ())
{
prev_offset = align_base (prev_offset,
- MAX (alignb, ASAN_RED_ZONE_SIZE),
+ MAX (alignb, ASAN_MIN_RED_ZONE_SIZE),
!FRAME_GROWS_DOWNWARD);
tree repr_decl = NULL_TREE;
- offset
- = alloc_stack_frame_space (stack_vars[i].size
- + ASAN_RED_ZONE_SIZE,
- MAX (alignb, ASAN_RED_ZONE_SIZE));
+ unsigned HOST_WIDE_INT size
+ = asan_var_and_redzone_size (stack_vars[i].size.to_constant ());
+ if (data->asan_vec.is_empty ())
+ size = MAX (size, ASAN_RED_ZONE_SIZE);
+
+ unsigned HOST_WIDE_INT alignment = MAX (alignb,
+ ASAN_MIN_RED_ZONE_SIZE);
+ offset = alloc_stack_frame_space (size, alignment);
data->asan_vec.safe_push (prev_offset);
/* Allocating a constant amount of space from a constant
@@ -2254,7 +2258,7 @@ expand_used_vars (void)
& ~(data.asan_alignb - HOST_WIDE_INT_1)) - sz;
/* Allocating a constant amount of space from a constant
starting offset must give a constant result. */
- offset = (alloc_stack_frame_space (redzonesz, ASAN_RED_ZONE_SIZE)
+ offset = (alloc_stack_frame_space (redzonesz, ASAN_SHADOW_GRANULARITY)
.to_constant ());
data.asan_vec.safe_push (prev_offset);
data.asan_vec.safe_push (offset);
diff --git a/gcc/testsuite/c-c++-common/asan/asan-stack-small.c b/gcc/testsuite/c-c++-common/asan/asan-stack-small.c
new file mode 100644
index 00000000000..11a56b8db4c
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/asan-stack-small.c
@@ -0,0 +1,28 @@
+/* { dg-do run } */
+
+char *pa;
+char *pb;
+char *pc;
+
+void access (volatile char *ptr)
+{
+ *ptr = 'x';
+}
+
+int main (int argc, char **argv)
+{
+ char a;
+ char b;
+ char c;
+
+ pa = &a;
+ pb = &b;
+ pc = &c;
+
+ access (pb);
+ access (pc);
+ // access 'b' here
+ access (pa + 32);
+
+ return 0;
+}
--
2.19.0