Re: [PATCH] Come up with --param asan-stack-small-redzone (PR sanitizer/81715).

[Martin Liška <mliska at suse dot cz>]

PING^1

On 9/26/18 11:33 AM, Martin Liška wrote:
> 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
>