On 10/21/2016 04:26 PM, Jakub Jelinek wrote:
> On Wed, Oct 12, 2016 at 04:07:53PM +0200, Martin Liška wrote:
>>> Ok, first let me list some needed follow-ups that don't need to be handled
>>> right away:
>>> - r237814-like changes for ASAN_MARK

I've spent quite some on that and that's what I begin (use-after-scope-addressable.patch).
Problem is that as I ignore all ASAN_MARK internal fns, the code does not detect having address
taken in:

_2 = MEM[(char *)&my_char + 8B];

  char *ptr;
  {
    char my_char[9];
    ptr = &my_char[0];
  }

  return *(ptr+8);

and thus the code in tree-ssa.c (maybe_optimize_var) sets TREE_ADDRESSABLE (var) = 0.

Second question I have is whether we want to handle just TREE_ADDRESSABLE stuff during gimplification?
Basically in a way that the current patch is doing?

>>
>> Yes, that's missing. I'm wondering whether the same approach would be viable as
>> the {un}poisoning happens during gimplification. Thus it generates &var expressions
>> which verifier won't be happy about?
> 
> Sure, it uses &var.  The trick is that the addressable sub-pass then ignores
> the taking of the address just in ASAN_MARK, and if some var is determined
> to be addressable solely because of ASAN_MARK &var uses and no other reason,
> the ASAN_MARK would be dropped and variable rewritten into SSA form.
> 
>>> - optimization to remove ASAN_MARK unpoisoning at the start of the function
>>
>> As current implementation does not poison variables at the very beginning of
>> a functions (RTL stack frame emission), it won't be needed.
> 
> But you still ASAN_MARK unpoison the vars when they get into scope, right?
> And those can be removed if the optimizers could prove that the area has not
> been poisoned yet since the beginning of the function.
> 
>>> - optimization to remove ASAN_MARK poisoning at the end of function (and
>>>   remove epilogue unpoisoning)
>>> - optimization to remove ASAN_MARK unpoisoning followed by ASAN_MARK poisoning
>>>   or vice versa if there are no memory accesses in between
>>
>> Yep, both are not handled and are very similar from my perspective: pairing
>> poisoning and unpoisoning pair which are not interfered by a memory operation
>> in between (in dominator meaning of word).
>> I'm wondering whether can be done effectively as we would need to visit all BBs
>> in a dominance domain (get_all_dominated_blocks) and check for the memory operations.
>> One improvement can be set of BBs that do not have any memory operations (excluding
>> ASAN_CHECK, ASAN_MARK builtins), but it can be still expensive to simplify poisoning
>> for all local variables. Or am I wrong?
> 
> My memory is weak, but isn't this something the sanopt pass
> (sanopt_optimize) is already doing similarly for e.g. ASAN_CHECK, UBSAN_NULL
> and UBSAN_VPTR checks?  For ASAN_MARK, you actually don't care about any
> calls, those shouldn't unpoison or poison again the vars under compiler's
> back.
> 
>>> - try to improve the goto handling
>>
>> Works for me to be target for stage3.
> 
> Sure.
> 
>> 2016-09-27  Martin Liska  <mliska@suse.cz>
> 
> Likely newer date :)
> 
>> 	* c-common.c (warn_for_unused_label): Save all labels used
>> 	in goto or in &label;
> 
> &label.
> instead?
> 
>> +	      if (dump_file && (dump_flags & TDF_DETAILS))
>> +		{
>> +		  const char *n = DECL_NAME (decl)
>> +		    ? IDENTIFIER_POINTER (DECL_NAME (decl)) : "<unknown>";
> 
> Bad formatting.
> 
> 		  const char *n = (DECL_NAME (decl)
> 				   ? IDENTIFIER_POINTER (DECL_NAME (decl))
> 				   : "<unknown>");
> or
> 		  const char *n
> 		    = (DECL_NAME (decl)
> 		       ? IDENTIFIER_POINTER (DECL_NAME (decl)) : "<unknown>");
> 
>>  /* Return true if DECL should be guarded on the stack.  */
>>  
>>  static inline bool
>>  asan_protect_stack_decl (tree decl)
>>  {
>> -  return DECL_P (decl) && !DECL_ARTIFICIAL (decl);
>> +  return DECL_P (decl) && TREE_ADDRESSABLE (decl);
>>  }
> 
> Can you explain this change?  It won't affect just
> -fsanitize-use-after-scope, and goes in both directions, adds some
> decls that weren't previously protected and removes others that were
> previously protected.
> 
> Is the removal of !DECL_ARTIFICIAL so that you can use-after-scope
> verify the C++ TARGET_EXPR temporaries?  Do we need to stack protect
> those even for -fno-sanitize-use-after-scope?
> I'm not 100% sure if we keep TREE_ADDRESSABLE meaningful for variables that
> need to live in memory for other reasons until expansion.


Yes, it's connected to C++ temporaries that are DECL_ARTIFICIAL.

> 
> So, I wonder if we don't want && (TREE_ADDRESSABLE (decl) || !DECL_ARTIFICAL (decl))
> or just DECL_P (decl); or conditionalize something on
> -fsanitize-use-after-scope.  Perhaps the change is good as is, just want to
> point out that it affects also other -fsanitize=address modes in both
> ways (instruments something that hasn't been before, and stops instrumenting
> something that has been before).

Ok, I'm suggesting to use:

static inline bool
asan_protect_stack_decl (tree decl)
{
  return DECL_P (decl)
    && (!DECL_ARTIFICIAL (decl)
	|| (asan_sanitize_use_after_scope () && TREE_ADDRESSABLE (decl)));
}

Apart from that, all nits you pointed out are fixed and the patch should be
in an acceptable shape. I'll run regression tests and we can focus on the
sanopt side of the patch.

Martin

> 
>> @@ -1514,7 +1503,8 @@ defer_stack_allocation (tree var, bool toplevel)
>>    /* If stack protection is enabled, *all* stack variables must be deferred,
>>       so that we can re-order the strings to the top of the frame.
>>       Similarly for Address Sanitizer.  */
>> -  if (flag_stack_protect || asan_sanitize_stack_p ())
>> +  if (flag_stack_protect
>> +      || asan_sanitize_stack_p ())
>>      return true;
> 
> This hunk isn't needed, if all the conditions fit on one line,
> one line is better.
> 
> 	Jakub
>