Re: [PATCH 6/X] libsanitizer: Add hwasan pass and associated gimple changes

[Richard Sandiford <richard.sandiford@arm.com>]

Matthew Malcomson <matthew.malcomson@arm.com> writes:
> @@ -133,6 +137,13 @@ enum asan_mark_flags
>  #undef DEF
>  };
>  
> +enum hwasan_mark_flags
> +{
> +#define DEF(X) HWASAN_MARK_##X
> +  IFN_ASAN_MARK_FLAGS
> +#undef DEF
> +};

Are these used anywhere?  It looks like expand_HWASAN_MARK uses the
plain asan versions.

> @@ -640,6 +684,85 @@ handle_builtin_alloca (gcall *call, gimple_stmt_iterator *iter)
>      = DECL_FUNCTION_CODE (callee) == BUILT_IN_ALLOCA
>        ? 0 : tree_to_uhwi (gimple_call_arg (call, 1));
>  
> +  if (hwasan_sanitize_allocas_p ())
> +    {
> +      /*
> +	 HWASAN needs a different expansion.
> +
> +	 addr = __builtin_alloca (size, align);
> +
> +	 should be replaced by
> +
> +	 new_size = size rounded up to HWASAN_TAG_GRANULE_SIZE byte alignment;
> +	 untagged_addr = __builtin_alloca (new_size, align);
> +	 tag = __hwasan_choose_alloca_tag ();
> +	 addr = __hwasan_tag_pointer (untagged_addr, tag);
> +	 __hwasan_tag_memory (untagged_addr, tag, new_size);
> +	*/
> +      /* Ensure alignment at least HWASAN_TAG_GRANULE_SIZE bytes so we start on
> +	 a tag granule.  */
> +      align = align > HWASAN_TAG_GRANULE_SIZE ? align : HWASAN_TAG_GRANULE_SIZE;
> +
> +      uint8_t tg_mask = HWASAN_TAG_GRANULE_SIZE - 1;
> +      /* tree new_size = (old_size + tg_mask) & ~tg_mask;  */
> +      tree old_size = gimple_call_arg (call, 0);
> +      tree tree_mask = build_int_cst (size_type_node, tg_mask);
> +      g = gimple_build_assign (make_ssa_name (size_type_node), PLUS_EXPR,
> +			       old_size, tree_mask);
> +      gsi_insert_before (iter, g, GSI_SAME_STMT);
> +      tree oversize = gimple_assign_lhs (g);
> +
> +      g = gimple_build_assign (make_ssa_name (size_type_node), BIT_NOT_EXPR,
> +			       tree_mask);
> +      tree mask = gimple_assign_lhs (g);
> +      gsi_insert_before (iter, g, GSI_SAME_STMT);

Seems simpler to use:

      tree mask = build_int_cst (size_type_node, -HWASAN_TAG_GRANULE_SIZE);

> +
> +      g = gimple_build_assign (make_ssa_name (size_type_node), BIT_AND_EXPR,
> +			       oversize, mask);
> +      gsi_insert_before (iter, g, GSI_SAME_STMT);
> +      tree new_size = gimple_assign_lhs (g);
> +
> +      /* emit the alloca call */
> +      tree fn = builtin_decl_implicit (BUILT_IN_ALLOCA_WITH_ALIGN);
> +      gg = gimple_build_call (fn, 2, new_size,
> +			      build_int_cst (size_type_node, align));
> +      tree untagged_addr = make_ssa_name (ptr_type, gg);
> +      gimple_call_set_lhs (gg, untagged_addr);
> +      gsi_insert_before (iter, gg, GSI_SAME_STMT);
> +
> +      /* Insert code choosing the tag.
> +	 Here we use an internal function so we can choose the tag at expand
> +	 time.  We need the decision to be made after stack variables have been
> +	 assigned their tag (i.e. once the tag_offset variable has been set to
> +	 one after the last stack variables tag).  */
> +
> +      gg = gimple_build_call_internal (IFN_HWASAN_CHOOSE_TAG, 0);
> +      tree tag = make_ssa_name (unsigned_char_type_node, gg);
> +      gimple_call_set_lhs (gg, tag);
> +      gsi_insert_before (iter, gg, GSI_SAME_STMT);
> +
> +      /* Insert code adding tag to pointer.  */
> +      fn = builtin_decl_implicit (BUILT_IN_HWASAN_TAG_PTR);
> +      gg = gimple_build_call (fn, 2, untagged_addr, tag);
> +      tree addr = make_ssa_name (ptr_type, gg);
> +      gimple_call_set_lhs (gg, addr);
> +      gsi_insert_before (iter, gg, GSI_SAME_STMT);
> +
> +      /* Insert code tagging shadow memory.
> +	 NOTE: require using `untagged_addr` here for libhwasan API.  */
> +      fn = builtin_decl_implicit (BUILT_IN_HWASAN_TAG_MEM);
> +      gg = gimple_build_call (fn, 3, untagged_addr, tag, new_size);
> +      gsi_insert_before (iter, gg, GSI_SAME_STMT);
> +
> +      /* Finally, replace old alloca ptr with NEW_ALLOCA.  */
> +      replace_call_with_value (iter, addr);
> +      return;
> +    }
> +
> +  tree last_alloca = get_last_alloca_addr ();
> +  const HOST_WIDE_INT redzone_mask = ASAN_RED_ZONE_SIZE - 1;
> +
> +
>    /* If ALIGN > ASAN_RED_ZONE_SIZE, we embed left redzone into first ALIGN
>       bytes of allocated space.  Otherwise, align alloca to ASAN_RED_ZONE_SIZE
>       manually.  */
> @@ -792,6 +915,31 @@ get_mem_refs_of_builtin_call (gcall *call,
>        break;
>  
>      case BUILT_IN_STRLEN:
> +      /* Special case strlen here since its length is taken from its return
> +	 value.
> +
> +	 The approach taken by the sanitizers is to check a memory access
> +	 before it's taken.  For ASAN strlen is intercepted by libasan, so no
> +	 check is inserted by the compiler.
> +
> +	 This function still returns `true` and provides a length to the rest
> +	 of the ASAN pass in order to record what areas have been checked,
> +	 avoiding superfluous checks later on.
> +
> +	 HWASAN does not intercept any of these internal functions.
> +	 This means that checks for memory accesses must be inserted by the
> +	 compiler.
> +	 strlen is a special case, because we can tell the length from the
> +	 return of the function, but that is not known until after the function
> +	 has returned.
> +
> +	 Hence we can't check the memory access before it happens.
> +	 We could check the memory access after it has already happened, but
> +	 for now I'm choosing to just ignore `strlen` calls.
> +	 This decision was simply made because that means the special case is
> +	 limited to this one case of this one function.  */
> +      if (hwasan_sanitize_p ())
> +	return false;

What does LLVM do here?

s/I'm choosing/we choose/

> @@ -1361,6 +1509,149 @@ asan_redzone_buffer::flush_if_full (void)
>      flush_redzone_payload ();
>  }
>  
> +
> +/* HWAddressSanitizer (hwasan) is a probabilistic method for detecting
> +   out-of-bounds and use-after-free bugs.
> +   Read more:
> +   http://code.google.com/p/address-sanitizer/
> +
> +   Similar to AddressSanitizer (asan) it consists of two parts: the
> +   instrumentation module in this file, and a run-time library.
> +
> +   The instrumentation module adds a run-time check before every memory insn in
> +   the same manner as asan (see the block comment for AddressSanitizer above).
> +   Currently, hwasan only adds out-of-line instrumentation, where each check is
> +   implemented as a function call to the run-time library.  Hence a check for a
> +   load of N bytes from address X would be implemented with a function call to
> +   __hwasan_loadN(X), and checking a store of N bytes from address X would be
> +   implemented with a function call to __hwasan_storeN(X).
> +
> +   The main difference between hwasan and asan is in the information stored to
> +   help this checking.  Both sanitizers use a shadow memory area which stores
> +   data recording the state of main memory at a corresponding address.
> +
> +   For hwasan, each 16 byte granule in main memory has a corresponding 1 byte
> +   in shadow memory.  This shadow address can be calculated with equation:
> +     (addr >> HWASAN_TAG_SHIFT_SIZE) + __hwasan_shadow_memory_dynamic_address;
> +   The conversion between real and shadow memory for asan is given in the block
> +   comment at the top of this file.
> +   The description of how this shadow memory is laid out for asan is in the

Maybe “is also in”?

> +   block comment at the top of this file, here we describe how this shadow
> +   memory is used for hwasan.
> +
> +   For hwasan, each variable is assigned a byte-sized 'tag'.  The extent of
> +   the shadow memory for that variable is filled with the assigned tag, and
> +   every pointer referencing that variable has its top byte set to the same
> +   tag.  The run-time library redefines malloc so that every allocation returns
> +   a tagged pointer and tags the corresponding shadow memory with the same tag.
> +
> +   On each pointer dereference the tag found in the pointer is compared to the
> +   tag found in the shadow memory corresponding to the accessed memory address.
> +   If these tags are found to differ then this memory access is judged to be
> +   invalid and a report is generated.
> +
> +   This method of bug detection is not perfect -- it can not catch every bad
> +   access -- but catches them probabilistically instead.  There is always the
> +   possibility that an invalid memory access will happen to access memory
> +   tagged with the same tag as the pointer that this access used.
> +   The chances of this are approx. 0.4% for any two uncorrelated objects.
> +
> +   Random tag generation can mitigate this problem by decreasing the
> +   probability that an invalid access will be missed in the same manner over
> +   multiple runs.  i.e. if two objects are tagged the same in one run of the
> +   binary they are unlikely to be tagged the same in the next run.
> +   Both heap and stack allocated objects have random tags by default.
> +
> +   [16 byte granule implications]
> +    Since the shadow memory only has a resolution on real memory of 16 bytes,
> +    invalid accesses that are within the same 16 byte granule as a valid
> +    address will not be caught.
> +
> +    There is a "short-granule" feature in the runtime library which does catch
> +    such accesses, but this feature is not implemented for stack objects (since
> +    stack objects are allocated and tagged by compiler instrumentation, and
> +    this feature has not yet been implemented in GCC instrumentation).
> +
> +    Another outcome of this 16 byte resolution is that each tagged object must
> +    be 16 byte aligned.  If two objects were to share any 16 byte granule in
> +    memory, then they both would have to be given the same tag, and invalid
> +    accesses to one using a pointer to the other would be undetectable.
> +
> +   [Compiler instrumentation]
> +    Compiler instrumentation ensures that two adjacent buffers on the stack are
> +    given different tags, this means an access to one buffer using a pointer
> +    generated from the other (e.g. through buffer overrun) will have mismatched
> +    tags and be caught by hwasan.
> +
> +    We don't randomly tag every object on the stack, since that would require
> +    keeping many registers to record each tag.  Instead we randomly generate a
> +    tag for each function frame, and each new stack object uses a tag offset
> +    from that frame tag.
> +    i.e. each object is tagged as RFT + offset, where RFT is the "random frame
> +    tag" generated for this frame.

So randomisation isn't effective at perturbing the pairs of mismatching
tags within a frame, but that is mitigated by objects with the same tag
within a frame being very far apart.  Is that right?

> +    As a demonstration, using the same example program as in the asan block
> +    comment above:
> +
> +     int
> +     foo ()
> +     {
> +       char a[23] = {0};
> +       int b[2] = {0};
> +
> +       a[5] = 1;
> +       b[1] = 2;
> +
> +       return a[5] + b[1];
> +     }
> +
> +    On AArch64 the stack will be ordered as follows for the above function:
> +
> +    Slot 1/ [24 bytes for variable 'a']
> +    Slot 2/ [8 bytes padding for alignment]
> +    Slot 3/ [8 bytes for variable 'b']
> +    Slot 4/ [8 bytes padding for alignment]
> +
> +    (The padding is there to ensure 16 byte alignment as described in the 16
> +     byte granule implications).
> +
> +    While the shadow memory will be ordered as follows:
> +
> +    - 2 bytes (representing 32 bytes in real memory) tagged with RFT + 1.
> +    - 1 byte (representing 16 bytes in real memory) tagged with RFT + 2.
> +
> +    And any pointer to "a" will have the tag RFT + 1, and any pointer to "b"
> +    will have the tag RFT + 2.
> +
> +   [Top Byte Ignore requirements]
> +    Hwasan requires the ability to store an 8 bit tag in every pointer.  There
> +    is no instrumentation done to remove this tag from pointers before
> +    dereferencing, which means the hardware must ignore this tag during memory
> +    accesses.
> +
> +    One architecture that provides this feature is the AArch64 architecture.
> +    This is the only architecture that hwasan is currently implemented for.

This could easily get out of date.  Maybe it would be better to refer to
the target hook instead.

> +
> +   [Stack requires cleanup on unwinding]
> +    During normal operation of a hwasan sanitized program more space in the
> +    shadow memory becomes tagged as the stack grows.  As the stack shrinks this
> +    shadow memory space must become untagged.  If it is not untagged then when
> +    the stack grows again (during other function calls later on in the program)
> +    objects on the stack that are usually not tagged (e.g. parameters passed on
> +    the stack) can be placed in memory whose shadow space is tagged with
> +    something else, and accesses can cause false positive reports.
> +
> +    Hence we place untagging code on every epilogue of functions which tag some
> +    stack objects.
> +
> +    Moreover, the run-time library intercepts longjmp & setjmp to uncolour

uncolor

> +    when the stack is unwound this way.
> +
> +    C++ exceptions are not yet handled, which means this sanitizer can not
> +    handle C++ code that throws exceptions -- it will give false positives
> +    after an exception has been thrown.  */

Is this a limitation in the library as well as the compiler side?
Might be worth clarifying in the comment.

But thanks for this, it's a really nice write-up.

> @@ -2301,7 +2620,8 @@ instrument_derefs (gimple_stmt_iterator *iter, tree t,
>      {
>        if (DECL_THREAD_LOCAL_P (inner))
>  	return;
> -      if (!param_asan_globals && is_global_var (inner))
> +      if ((hwasan_sanitize_p () || !param_asan_globals)
> +	  && is_global_var (inner))
>          return;
>        if (!TREE_STATIC (inner))
>  	{

Could you expand on this?

> @@ -2530,10 +2850,13 @@ maybe_instrument_call (gimple_stmt_iterator *iter)
>  	      break;
>  	    }
>  	}
> -      tree decl = builtin_decl_implicit (BUILT_IN_ASAN_HANDLE_NO_RETURN);
> -      gimple *g = gimple_build_call (decl, 0);
> -      gimple_set_location (g, gimple_location (stmt));
> -      gsi_insert_before (iter, g, GSI_SAME_STMT);
> +      if (! hwasan_sanitize_p ())
> +	{
> +	  tree decl = builtin_decl_implicit (BUILT_IN_ASAN_HANDLE_NO_RETURN);
> +	  gimple *g = gimple_build_call (decl, 0);
> +	  gimple_set_location (g, gimple_location (stmt));
> +	  gsi_insert_before (iter, g, GSI_SAME_STMT);
> +	}

Why is this only needed for asan?  (Just curious.)

> @@ -3255,18 +3581,34 @@ asan_expand_mark_ifn (gimple_stmt_iterator *iter)
>  
>    gcc_checking_assert (TREE_CODE (decl) == VAR_DECL);
>  
> +  tree len = gimple_call_arg (g, 2);
> +  gcc_assert (tree_fits_shwi_p (len));
> +  unsigned HOST_WIDE_INT size_in_bytes = tree_to_shwi (len);
> +  gcc_assert (size_in_bytes);
> +
> +  if (hwasan_sanitize_p ())
> +    {
> +      gcc_assert (param_hwasan_instrument_stack);
> +      /* Here we swap ASAN_MARK calls for HWASAN_MARK.
> +	 This is because we are using the approach of using ASAN_MARK as a
> +	 synonym until here.
> +	 That approach means we don't yet have to duplicate all the special
> +	 cases for ASAN_MARK and ASAN_POISON with the exact same handling but
> +	 called HWASAN_MARK etc.  */

Definitely in favour of that :-)

> +      gimple *hw_poison_call
> +	= gimple_build_call_internal (IFN_HWASAN_MARK, 3,
> +				      gimple_call_arg (g, 0),
> +				      base, len);
> +      gsi_replace (iter, hw_poison_call, false);
> +      return false;
> +    }
> +
>    if (is_poison)
>      {
>        if (asan_handled_variables == NULL)
>  	asan_handled_variables = new hash_set<tree> (16);
>        asan_handled_variables->add (decl);
>      }
> -  tree len = gimple_call_arg (g, 2);
> -
> -  gcc_assert (tree_fits_shwi_p (len));
> -  unsigned HOST_WIDE_INT size_in_bytes = tree_to_shwi (len);
> -  gcc_assert (size_in_bytes);
> -
>    g = gimple_build_assign (make_ssa_name (pointer_sized_int_node),
>  			   NOP_EXPR, base);
>    gimple_set_location (g, loc);
> @@ -3329,6 +3671,7 @@ asan_expand_mark_ifn (gimple_stmt_iterator *iter)
>  bool
>  asan_expand_check_ifn (gimple_stmt_iterator *iter, bool use_calls)
>  {
> +  gcc_assert (!hwasan_sanitize_p ());
>    gimple *g = gsi_stmt (*iter);
>    location_t loc = gimple_location (g);
>    bool recover_p;
> @@ -3602,11 +3945,66 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter,
>  
>        int nargs;
>        bool store_p = gimple_call_internal_p (use, IFN_ASAN_POISON_USE);
> -      tree fun = report_error_func (store_p, recover_p, tree_to_uhwi (size),
> -				    &nargs);
> -
> -      gcall *call = gimple_build_call (fun, 1,
> -				       build_fold_addr_expr (shadow_var));
> +      gcall *call;
> +      if (hwasan_sanitize_p ())
> +	{
> +	  tree fun = builtin_decl_implicit (BUILT_IN_HWASAN_TAG_MISMATCH4);
> +	  /* NOTE: hwasan has no __hwasan_report_* functions like asan does.
> +		We use __hwasan_tag_mismatch4 with arguments that tell it the
> +		size of access and load to report all tag mismatches.
> +
> +		The arguments to this function are:
> +		  Address of invalid access.
> +		  Bitfield containing information about the access
> +		    (access_info)
> +		  Pointer to a frame of registers
> +		    (for use in printing the contents of registers in a dump)
> +		    Not used yet -- to be used by inline instrumentation.
> +		  Size of access (only if size is not representable in the
> +		    access_info argument).
> +
> +		The access_info bitfield encodes the following pieces of
> +		information:
> +		  - Is this a store or load?
> +		    access_info & 0x10  =>  store
> +		  - Should the program continue after reporting the error?
> +		    access_info & 0x20  =>  recover
> +		  - What size access is this (if size is less than 16 bytes)
> +		    if (access_info & 0xf == 0xf)
> +		      size is taken from last argument.
> +		    else
> +		      size == 1 << (access_info & 0xf)
> +
> +		The last argument contains the size of the access iff the
> +		access was of a size greater than or equal to 16 bytes.
> +
> +		See the function definition `__hwasan_tag_mismatch4` in
> +		libsanitizer/hwasan for the full definition.
> +		*/
> +	  unsigned HOST_WIDE_INT size_in_bytes = tree_to_uhwi (size);
> +	  unsigned size_indicator = (size_in_bytes > 16)
> +	    ? 0xf
> +	    : exact_log2 (size_in_bytes);

The parts about “16 bytes” don't seem to match the equation;
seems like it's a power of 2 in the range [1, 16384].

Is there any benefit to using the size field encoded in the access_info here?
AIUI it was retained for compatibility with pre-__hwasan_tag_mismatch4
versions of the library.  Seems like we could always use 0xf instead.

> +	  unsigned access_info = (0x20 * recover_p)
> +	    + (0x10 * store_p)
> +	    + (size_indicator);
> +	  tree long_pointer_type
> +	    = build_pointer_type (long_unsigned_type_node);

pointer_sized_int_type seems more appropriate, given that the second
argument and the target of the third argument are uptrs.

> +	  call = gimple_build_call (fun, 3,
> +				    build_fold_addr_expr (shadow_var),
> +				    build_int_cst (long_unsigned_type_node,
> +						   access_info),
> +				    build_int_cst (long_pointer_type,
> +						   0),
> +				    size);
> +	}
> +      else
> +	{
> +	  tree fun = report_error_func (store_p, recover_p, tree_to_uhwi (size),
> +					&nargs);
> +	  call = gimple_build_call (fun, 1,
> +				    build_fold_addr_expr (shadow_var));
> +	}
>        gimple_set_location (call, gimple_location (use));
>        gimple *call_to_insert = call;
>  
> @@ -4005,4 +4425,216 @@ hwasan_finish_file (void)
>    flag_sanitize |= SANITIZE_HWADDRESS;
>  }
>  
> +/* Construct a function tree for __hwasan_{load,store}{1,2,4,8,16,_n}.
> +   IS_STORE is either 1 (for a store) or 0 (for a load).  */
> +static tree
> +hwasan_check_func (bool is_store, bool recover_p, HOST_WIDE_INT size_in_bytes,
> +		   int *nargs)
> +{
> +  static enum built_in_function check[2][2][6]
> +    = { { { BUILT_IN_HWASAN_LOAD1, BUILT_IN_HWASAN_LOAD2,
> +	    BUILT_IN_HWASAN_LOAD4, BUILT_IN_HWASAN_LOAD8,
> +	    BUILT_IN_HWASAN_LOAD16, BUILT_IN_HWASAN_LOADN },
> +	  { BUILT_IN_HWASAN_STORE1, BUILT_IN_HWASAN_STORE2,
> +	    BUILT_IN_HWASAN_STORE4, BUILT_IN_HWASAN_STORE8,
> +	    BUILT_IN_HWASAN_STORE16, BUILT_IN_HWASAN_STOREN } },
> +	{ { BUILT_IN_HWASAN_LOAD1_NOABORT,
> +	    BUILT_IN_HWASAN_LOAD2_NOABORT,
> +	    BUILT_IN_HWASAN_LOAD4_NOABORT,
> +	    BUILT_IN_HWASAN_LOAD8_NOABORT,
> +	    BUILT_IN_HWASAN_LOAD16_NOABORT,
> +	    BUILT_IN_HWASAN_LOADN_NOABORT },
> +	  { BUILT_IN_HWASAN_STORE1_NOABORT,
> +	    BUILT_IN_HWASAN_STORE2_NOABORT,
> +	    BUILT_IN_HWASAN_STORE4_NOABORT,
> +	    BUILT_IN_HWASAN_STORE8_NOABORT,
> +	    BUILT_IN_HWASAN_STORE16_NOABORT,
> +	    BUILT_IN_HWASAN_STOREN_NOABORT } } };
> +  if (size_in_bytes == -1)
> +    {
> +      *nargs = 2;
> +      return builtin_decl_implicit (check[recover_p][is_store][5]);
> +    }
> +  *nargs = 1;
> +  int size_log2 = exact_log2 (size_in_bytes);
> +  return builtin_decl_implicit (check[recover_p][is_store][size_log2]);

I don't follow the logic here.  Why is it the case that we have to
handle size_in_bytes == -1, but are otherwise guaranteed that
size_in_bytes is in [1, 2, 4, 8, 16]?  I.e. why is the tree_fits_shwi_p
call needed here:

> +  unsigned HOST_WIDE_INT size_in_bytes
> +    = is_scalar_access && tree_fits_shwi_p (len) ? tree_to_shwi (len) : -1;

?  If is_scalar_access guarantees a valid value then I think we should
drop the tree_fits_shwi_p check above and instead assert that size_log2
is in range.  If is_scalar_access doesn't guarantee a valid value then
I think we should check size_log2 and set *nargs to 2 for out-of-range
values.

> +}
> +
> +/* Expand the HWASAN_{LOAD,STORE} builtins.  */
> +bool
> +hwasan_expand_check_ifn (gimple_stmt_iterator *iter, bool)
> +{
> +  gimple *g = gsi_stmt (*iter);
> +  location_t loc = gimple_location (g);
> +  bool recover_p;
> +  if (flag_sanitize & SANITIZE_USER_HWADDRESS)
> +    recover_p = (flag_sanitize_recover & SANITIZE_USER_HWADDRESS) != 0;
> +  else
> +    recover_p = (flag_sanitize_recover & SANITIZE_KERNEL_HWADDRESS) != 0;
> +
> +  HOST_WIDE_INT flags = tree_to_shwi (gimple_call_arg (g, 0));
> +  gcc_assert (flags < ASAN_CHECK_LAST);
> +  bool is_scalar_access = (flags & ASAN_CHECK_SCALAR_ACCESS) != 0;
> +  bool is_store = (flags & ASAN_CHECK_STORE) != 0;
> +  bool is_non_zero_len = (flags & ASAN_CHECK_NON_ZERO_LEN) != 0;
> +
> +  tree base = gimple_call_arg (g, 1);
> +  tree len = gimple_call_arg (g, 2);
> +
> +  /* `align` is unused for HWASAN_CHECK, but I pass the argument anyway

s/I/we/.

> +     since that way the arguments match ASAN_CHECK.  */
> +  /* HOST_WIDE_INT align = tree_to_shwi (gimple_call_arg (g, 3));  */
> +
> +  unsigned HOST_WIDE_INT size_in_bytes
> +    = is_scalar_access && tree_fits_shwi_p (len) ? tree_to_shwi (len) : -1;
> +
> +  gimple_stmt_iterator gsi = *iter;
> +
> +  if (!is_non_zero_len)
> +    {
> +      /* So, the length of the memory area to hwasan-protect is
> +	 non-constant.  Let's guard the generated instrumentation code
> +	 like:
> +
> +	 if (len != 0)
> +	   {
> +	     // hwasan instrumentation code goes here.
> +	   }
> +	 // falltrough instructions, starting with *ITER.  */
> +
> +      g = gimple_build_cond (NE_EXPR,
> +			    len,
> +			    build_int_cst (TREE_TYPE (len), 0),
> +			    NULL_TREE, NULL_TREE);
> +      gimple_set_location (g, loc);
> +
> +      basic_block then_bb, fallthrough_bb;
> +      insert_if_then_before_iter (as_a <gcond *> (g), iter,
> +				  /*then_more_likely_p=*/true,
> +				  &then_bb, &fallthrough_bb);
> +      /* Note that fallthrough_bb starts with the statement that was
> +	pointed to by ITER.  */
> +
> +      /* The 'then block' of the 'if (len != 0) condition is where
> +	we'll generate the hwasan instrumentation code now.  */
> +      gsi = gsi_last_bb (then_bb);
> +    }
> +
> +  g = gimple_build_assign (make_ssa_name (pointer_sized_int_node),
> +			   NOP_EXPR, base);
> +  gimple_set_location (g, loc);
> +  gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +  tree base_addr = gimple_assign_lhs (g);
> +
> +  int nargs = 0;
> +  tree fun = hwasan_check_func (is_store, recover_p, size_in_bytes, &nargs);
> +  if (nargs == 1)
> +    g = gimple_build_call (fun, 1, base_addr);
> +  else
> +    {
> +      gcc_assert (nargs == 2);
> +      g = gimple_build_assign (make_ssa_name (pointer_sized_int_node),
> +			       NOP_EXPR, len);
> +      gimple_set_location (g, loc);
> +      gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +      tree sz_arg = gimple_assign_lhs (g);
> +      g = gimple_build_call (fun, nargs, base_addr, sz_arg);
> +    }
> +
> +  gimple_set_location (g, loc);
> +  gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> +  gsi_remove (iter, true);
> +  *iter = gsi;
> +  return false;
> +}
> +
> +/* For stack tagging:
> +     Dummy: the HWASAN_MARK internal function should only ever be in the code
> +     after the sanopt pass.  */

Same nit as the previous part about this style of comment formatting.

> +bool
> +hwasan_expand_mark_ifn (gimple_stmt_iterator *)
> +{
> +  gcc_unreachable ();
> +}
> +
> +bool
> +gate_hwasan ()
> +{
> +  return hwasan_sanitize_p ();
> +}
> +
> +namespace {
> +
> +const pass_data pass_data_hwasan =
> +{
> +  GIMPLE_PASS, /* type */
> +  "hwasan", /* name */
> +  OPTGROUP_NONE, /* optinfo_flags */
> +  TV_NONE, /* tv_id */
> +  ( PROP_ssa | PROP_cfg | PROP_gimple_leh ), /* properties_required */
> +  0, /* properties_provided */
> +  0, /* properties_destroyed */
> +  0, /* todo_flags_start */
> +  TODO_update_ssa, /* todo_flags_finish */
> +};
> +
> +class pass_hwasan : public gimple_opt_pass
> +{
> +public:
> +  pass_hwasan (gcc::context *ctxt)
> +    : gimple_opt_pass (pass_data_hwasan, ctxt)
> +  {}
> +
> +  /* opt_pass methods: */
> +  opt_pass * clone () { return new pass_hwasan (m_ctxt); }
> +  virtual bool gate (function *) { return gate_hwasan (); }
> +  virtual unsigned int execute (function *) { return hwasan_instrument (); }
> +
> +}; /* class pass_hwasan  */
> +
> +} /* anon namespace  */
> +
> +gimple_opt_pass *
> +make_pass_hwasan (gcc::context *ctxt)
> +{
> +  return new pass_hwasan (ctxt);
> +}
> +
> +namespace {
> +
> +const pass_data pass_data_hwasan_O0 =
> +{
> +  GIMPLE_PASS, /* type */
> +  "hwasan_O0", /* name */
> +  OPTGROUP_NONE, /* optinfo_flags */
> +  TV_NONE, /* tv_id */
> +  ( PROP_ssa | PROP_cfg | PROP_gimple_leh ), /* properties_required */
> +  0, /* properties_provided */
> +  0, /* properties_destroyed */
> +  0, /* todo_flags_start */
> +  TODO_update_ssa, /* todo_flags_finish */
> +};
> +
> +class pass_hwasan_O0 : public gimple_opt_pass
> +{
> +public:
> +  pass_hwasan_O0 (gcc::context *ctxt)
> +    : gimple_opt_pass (pass_data_hwasan_O0, ctxt)
> +  {}
> +
> +  /* opt_pass methods: */
> +  opt_pass * clone () { return new pass_hwasan_O0 (m_ctxt); }
> +  virtual bool gate (function *) { return !optimize && gate_hwasan (); }
> +  virtual unsigned int execute (function *) { return hwasan_instrument (); }
> +
> +}; /* class pass_hwasan */
> +
> +} /* anon namespace */
> +
> +gimple_opt_pass *
> +make_pass_hwasan_O0 (gcc::context *ctxt)
> +{
> +  return new pass_hwasan_O0 (ctxt);
> +}

Is it worth creating separate passes for this?  Seems like we could
just fork the implementation of the asan ones based on whether we're
using hwasan or not.

> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 15dfee903ab298f6a02e45d1affcc2260f3c911d..24ebedd490aea4ad634a92aa5742a83b1b0c0bb7 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1234,8 +1234,11 @@ asan_poison_variable (tree decl, bool poison, gimple_stmt_iterator *it,
>  
>    /* It's necessary to have all stack variables aligned to ASAN granularity
>       bytes.  */
> -  if (DECL_ALIGN_UNIT (decl) <= ASAN_SHADOW_GRANULARITY)
> -    SET_DECL_ALIGN (decl, BITS_PER_UNIT * ASAN_SHADOW_GRANULARITY);
> +  gcc_assert (!hwasan_sanitize_p () || hwasan_sanitize_stack_p ());
> +  unsigned shadow_granularity
> +    = hwasan_sanitize_p () ? HWASAN_TAG_GRANULE_SIZE : ASAN_SHADOW_GRANULARITY;
> +  if (DECL_ALIGN_UNIT (decl) <= shadow_granularity)
> +    SET_DECL_ALIGN (decl, BITS_PER_UNIT * shadow_granularity);
>  
>    HOST_WIDE_INT flags = poison ? ASAN_MARK_POISON : ASAN_MARK_UNPOISON;
>  
> @@ -15023,7 +15026,7 @@ gimplify_function_tree (tree fndecl)
>       if necessary.  */
>    cfun->curr_properties |= PROP_gimple_lva;
>  
> -  if (asan_sanitize_use_after_scope () && sanitize_flags_p (SANITIZE_ADDRESS))
> +  if (asan_sanitize_use_after_scope ())
>      asan_poisoned_variables = new hash_set<tree> ();
>    bind = gimplify_body (fndecl, true);
>    if (asan_poisoned_variables)
> diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c
> index 8efc77d986b927dc4a37e396e6c710ffeda663ff..3397fa355f373f3ead5a02a07838a467f08f03c4 100644
> --- a/gcc/internal-fn.c
> +++ b/gcc/internal-fn.c
> @@ -467,6 +467,105 @@ expand_UBSAN_OBJECT_SIZE (internal_fn, gcall *)
>  /* This should get expanded in the sanopt pass.  */
>  
>  static void
> +expand_HWASAN_CHECK (internal_fn, gcall *)
> +{
> +  gcc_unreachable ();
> +}
> +
> +/* For hwasan stack tagging:
> +     Clear tags on the dynamically allocated space.
> +     For use after an object dynamically allocated on the stack goes out of
> +     scope.  */
> +static void
> +expand_HWASAN_ALLOCA_UNPOISON (internal_fn, gcall *gc)
> +{
> +  tree restored_position = gimple_call_arg (gc, 0);
> +  rtx restored_rtx = expand_expr (restored_position, NULL_RTX, VOIDmode,
> +				  EXPAND_NORMAL);
> +  rtx func = init_one_libfunc ("__hwasan_tag_memory");
> +  rtx off = expand_simple_binop (ptr_mode, MINUS, restored_rtx,
> +				 stack_pointer_rtx, NULL_RTX, 0,
> +				 OPTAB_LIB_WIDEN);
> +  rtx dynamic = convert_memory_address (ptr_mode, virtual_stack_dynamic_rtx);
> +  emit_library_call_value (func, NULL_RTX, LCT_NORMAL, VOIDmode,
> +			   dynamic, ptr_mode,
> +			   const0_rtx, QImode,
> +			   off, ptr_mode);
> +}
> +
> +/* For hwasan stack tagging:
> +     Return a tag to be used for a dynamic allocation.  */
> +static void
> +expand_HWASAN_CHOOSE_TAG (internal_fn, gcall *gc)
> +{
> +  tree tag = gimple_call_lhs (gc);
> +  rtx target = expand_expr (tag, NULL_RTX, VOIDmode, EXPAND_NORMAL);
> +  machine_mode mode = GET_MODE (target);
> +  gcc_assert (mode == QImode);
> +
> +  rtx base_tag = hwasan_extract_tag (hwasan_base ());
> +  gcc_assert (base_tag);
> +  rtx tag_offset = GEN_INT (hwasan_current_tag ());
> +  rtx chosen_tag = expand_simple_binop (QImode, PLUS, base_tag, tag_offset,
> +					target, /* unsignedp = */1,
> +					OPTAB_WIDEN);
> +
> +  gcc_assert (chosen_tag);
> +  /* Really need to put the tag into the `target` RTX.  */
> +  if (chosen_tag != target)
> +    {
> +      rtx temp = chosen_tag;
> +      machine_mode ret_mode = GET_MODE (chosen_tag);
> +      if (ret_mode != mode)
> +	temp = simplify_gen_unary (TRUNCATE, mode, chosen_tag, ret_mode);

Is this truncation necessary?  I'd have expected:

  expand_simple_binop (QImode, …)

to always return a QImode value, even for the widening path.

> +/* For hwasan stack tagging:
> +     Tag a region of space in the shadow stack according to the base pointer of
> +     an object on the stack.  */
> +static void
> +expand_HWASAN_MARK (internal_fn, gcall *gc)
> +{
> +  HOST_WIDE_INT flag = tree_to_shwi (gimple_call_arg (gc, 0));
> +  bool is_poison = ((asan_mark_flags)flag) == ASAN_MARK_POISON;
> +
> +  tree base = gimple_call_arg (gc, 1);
> +  gcc_checking_assert (TREE_CODE (base) == ADDR_EXPR);
> +  /* base is a pointer argument, hence in ptr_mode.
> +     We convert to Pmode for use in the hwasan_extract_tag and
> +     hwasan_create_untagged_base functions.
> +     We then convert the result to ptr_mode for the emit_library_call.  */

How does this ptr_mode<->Pmode stuff work for ILP32?  Wouldn't the tag
get lost by the Pmode->ptr_mode conversion?  Or does something ensure
that the tag is moved from bits 24+ to bits 56+ and back?

> +  rtx base_rtx = convert_memory_address (Pmode, expand_normal (base));
> +
> +  rtx tag = is_poison ? const0_rtx : hwasan_extract_tag (base_rtx);
> +  rtx address = hwasan_create_untagged_base (base_rtx);
> +  address = convert_memory_address (ptr_mode, address);
> +
> +  tree len = gimple_call_arg (gc, 2);
> +  gcc_assert (tree_fits_shwi_p (len));

Why's this guaranteed to be a SHWI?  For example, I couldn't see
anything that explicitly stopped calls to build_asan_poison_call_expr
with poly_int- or variable-sized decls.

Could we expand this to an aligned size in gimple, when generating
the HWASAN_MARK call?  E.g. we could reuse the alloca code for
computing an aligned size.

It'd probably be worth using the gimple_build routines in
gimple-fold.h to build the calculation, instead of calling
gimple_build_assign directly.  The operations will then get
folded on the fly.

> +  unsigned HOST_WIDE_INT size_in_bytes = tree_to_shwi (len);
> +  uint8_t tg_mask = HWASAN_TAG_GRANULE_SIZE - 1;
> +  gcc_assert (size_in_bytes);
> +  size_in_bytes = (size_in_bytes + tg_mask) & ~tg_mask;
> +  rtx size = gen_int_mode (size_in_bytes, ptr_mode);
> +
> +  rtx func = init_one_libfunc ("__hwasan_tag_memory");
> +  emit_library_call (func,
> +      LCT_NORMAL,
> +      VOIDmode,
> +      address, ptr_mode,
> +      tag, QImode,
> +      size, ptr_mode);

Formatting nit: should be indented under “func”.  Seems at least
LCT_NORMAL and VOIDmode could go on the same line as “func”.

Generally looks really good though (this and previous patches).

Thanks,
Richard