[PATCH v4 5/7] Write SEH records to pdata/xdata

[Evgeny Karpov]

On Fri, Oct 17, 2025, Jan Beulich wrote:
> On 15.08.2025 00:50, Evgeny Karpov wrote:
> > --- a/gas/config/obj-coff-seh.c
> > +++ b/gas/config/obj-coff-seh.c
> > @@ -1256,7 +1256,86 @@ seh_x64_write_prologue_data (const seh_context *c)
> >  	}
> >      }
> >  }
> > +#endif
> > +
> > +#if defined (COFFAARCH64)
> > +static void
> > +seh_aarch64_emit_epilog_scopes (const seh_context *seh_ctx,
> > +				uint64_t fragment_offset,
> > +				uint32_t prolog_size,
> > +				unsigned int first_fragment_scope,
> > +				unsigned int last_fragment_scope,
> > +				bool has_phantom_prolog)
> > +{
> > +  int32_t start_index_offset = 0;
> 
> With this being a signed type, ...
> 
> > +  const
> > +  seh_aarch64_epilogue_scope *scopes = seh_ctx->aarch64_ctx.epilogue_scopes;
> > +  if (first_fragment_scope < seh_ctx->aarch64_ctx.epilogue_scopes_count)
> > +    start_index_offset = scopes[first_fragment_scope].epilogue_start_index
> > +			 - prolog_size;
> > +  if (has_phantom_prolog)
> > +    start_index_offset -= 1;
> 
> ... the variable can end up holding -1 here. What does ...
> 
> > +  for (unsigned int i = first_fragment_scope; i < last_fragment_scope; ++i)
> > +    {
> > +      seh_aarch64_epilogue_scope scope;
> > +      scope = seh_ctx->aarch64_ctx.epilogue_scopes[i];
> > +      scope.epilogue_start_offset_reduced = (scope.epilogue_start_offset
> > +					    - fragment_offset) >> 2;
> > +      scope.epilogue_start_index -= start_index_offset;
> 
> ... this mean here? You'd then _increment_ the destination by one. If all
> of this is intentional, some commentary would help. Otherwise please use
> an unsigned type and make clear why there cannot be any underflow.
> 
> Also - why is that variable of a fixed width type? Plain (signed or unsigned)
> int would apparently do, wouldn't it?

Yes, it makes sense to change to unsigned and add a condition for the decrement on
has_phantom_prolog.

> > +      uint32_t scope_code;
> > +      memcpy (&scope_code, &scope, sizeof (scope_code));
> > +      out_four (scope_code);
> > +    }
> > +}
> > +
> > +static void
> > +seh_aarch64_emit_unwind_codes (const seh_context *seh_ctx,
> > +			       uint32_t prolog_size,
> > +			       uint32_t first_epilog_index,
> > +			       uint32_t last_epilog_index,
> > +			       bool has_phantom_prolog)
> > +{
> > +  uint32_t total_byte_count = 0;
> > +
> > +  if (has_phantom_prolog)
> > +    {
> > +      ++total_byte_count;
> > +      md_number_to_chars (frag_more (1), AARCH64_UNOP_ENDC, 1);
> > +    }
> > 
> > +  uint32_t unwind_bytes_offset = 0;
> > +  for (int i = 0; i < (int)seh_ctx->aarch64_ctx.unwind_codes_count; ++i)
> 
> Why the cast? Why isn't i of an appropriate unsigned type?

Ok. 

> > +    {
> > +      const seh_aarch64_unwind_code *code = seh_ctx->aarch64_ctx.unwind_codes
> > +					    + i;
> > +      const int byte_count = aarch64_unwind_code_pack_data[code->type].size;
> > +      unwind_bytes_offset += byte_count;
> > +
> > +      if (unwind_bytes_offset > last_epilog_index)
> > +	break;
> > +
> > +      if (unwind_bytes_offset > prolog_size
> > +	  && unwind_bytes_offset <= first_epilog_index)
> > +	continue;
> > +
> > +      /*  emit unwind code bytes in big endian.  */
> > +      number_to_chars_bigendian (frag_more (byte_count), code->value,
> > +				 byte_count);
> > +      total_byte_count += byte_count;
> > +    }
> > +
> > +    /* handle word alignment.  */
> > +    int required_padding = (4 - total_byte_count % 4) % 4;
> 
> And again - why plain int? And what are the literal 4-s expressing?

Yes, it makes sense to change.
 
> > +    if (required_padding)
> > +      {
> > +	const uint32_t nop_chain = 0xe3e3e3e3;
> 
> Please at least have a comment here, such that grep-ing for AARCH64_UNOP_NOP
> will hit this line.

A comment will be added.
 
> > @@ -1352,6 +1431,213 @@ seh_x64_write_function_xdata (seh_context *c)
> >  }
> >  #endif
> > 
> > +#if defined (COFFAARCH64)
> > +/* Write out the xdata information for one function (aarch64).  */
> > +static void
> > +seh_aarch64_write_function_xdata (seh_context *seh_ctx)
> 
> For the function parameter and elsewhere - can you please see about applying
> const to pointed-to types wherever possible?

It was following the same semantics as x64, however it will be changed during
the refactoring.
 
> > +{
> > +  if (!seh_ctx->aarch64_ctx.unwind_codes_byte_count)
> > +    return;
> > +
> > +  /* Set 4-byte alignment.  */
> > +  frag_align (2, 0, 0);
> 
> This may be related to the 4-s above - can they be tied together by using
> some #define-s?

It will be refactored.

> > +  fragS *start_frag, *end_frag;
> > +  addressT start_value, end_value;
> > +  start_frag = symbol_get_frag_and_value (seh_ctx->start_addr, &start_value);
> > +  end_frag = symbol_get_frag_and_value (seh_ctx->end_addr, &end_value);
> > +  offsetT offset;
> > +  frag_offset_ignore_align_p (end_frag, start_frag, &offset);
> > +  start_value += offset / OCTETS_PER_BYTE;
> > +  offset = end_value - start_value;
> > +
> > +  if (offset < 0)
> > +    {
> > +      as_bad (_("the function size expression for %s "
> > +	      "does not evaluate to a non-negative value"),
> > +	      S_GET_NAME (seh_ctx->start_addr));
> > +      return;
> > +    }
> > +
> > +  uintptr_t func_size = offset;
> > +
> > +  const uint32_t max_frag_size = ((1 << 18) - 1) << 2;
> 
> Where does this boundary come from?

https://learn.microsoft.com/en-us/cpp/build/arm64-exception-handling?view=msvc-170#large-functions
The large functions should be split into fragments smaller than 1MB.

> > +  uintptr_t fragment_offset = 0;
> > +  bool is_fragmented_function = func_size > max_frag_size;
> > +
> > +  /* [first_fragment_scope, last_fragment_scope).  */
> > +  unsigned int first_fragment_scope = 0;
> > +  unsigned int last_fragment_scope = 0;
> > +  uint32_t prolog_size = 0;
> 
> The initializer is pointless here. Also you want to be consistent with
> variable definitions: Either at the beginning of a scope, or really
> immediately ahead of when they're first used.

It will be moved closer to the place of use.

> > +  uint32_t prolog_insruction_count = 0;
> 
> Nit: typo (missing 't' as it looks). But maybe prolog_insn_count would
> be slightly better anyway?
> 
> For both of them (and many others elsewhere) also again: Why fixed-
> width?

Yes, just unsigned works here.
 
> > +  for (unsigned int i = 0; i < seh_ctx->aarch64_ctx.unwind_codes_count; ++i)
> > +    {
> > +      if (seh_ctx->aarch64_ctx.unwind_codes[i].type == end)
> 
> This use of "end" makes pretty clear that those enumerators lack some
> disambiguating prefix.
 
A prefix will be added.

> > +	{
> > +	  prolog_insruction_count = i + 1;
> > +	  break;
> > +	}
> > +    }
> > +
> > +  if (seh_ctx->aarch64_ctx.epilogue_scopes_count)
> > +    prolog_size = seh_ctx->aarch64_ctx.epilogue_scopes[0].epilogue_start_index;
> > +  else
> > +    prolog_size = seh_ctx->aarch64_ctx.unwind_codes_byte_count;
> > +
> > +  seh_aarch64_func_fragment *fragment;
> > +  fragment = &seh_ctx->aarch64_ctx.func_fragment;
> 
> Why not right as initializer of the variable? (Apparently I overlooked
> similar anomalies already further up.)

The reason is the 80 characters limit per line. Any other preferable style?
 
> > +  while (true)
> > +    {
> > +      fragment->xdata_addr = symbol_temp_new_now ();
> > +      fragment->offset = fragment_offset;
> > +      fragment->next = NULL;
> > +
> > +      uintptr_t frag_size = func_size - fragment_offset;
> > +      if (frag_size > max_frag_size)
> > +	frag_size = max_frag_size;
> > +
> > +      bool is_first_frag = fragment_offset == 0;
> > +      bool is_last_frag = (fragment_offset + frag_size) == func_size;
> > +
> > +      if (!is_fragmented_function)
> > +	last_fragment_scope = seh_ctx->aarch64_ctx.epilogue_scopes_count;
> > +      else
> > +	{
> > +	  first_fragment_scope = last_fragment_scope;
> > +	  for (unsigned int i = first_fragment_scope;
> > +	       i < seh_ctx->aarch64_ctx.epilogue_scopes_count; ++i)
> > +	    {
> > +	      const seh_aarch64_epilogue_scope *scope;
> > +	      scope = seh_ctx->aarch64_ctx.epilogue_scopes;
> > +	      scope += i;
> > +	      if (scope->epilogue_start_offset >= (fragment_offset + frag_size))
> > +		break;
> > +
> > +	      if (scope->epilogue_end_offset >= (fragment_offset + frag_size))
> > +		{
> > +		  frag_size = scope->epilogue_start_offset - fragment_offset;
> > +		  break;
> > +		}
> > +
> > +	      if (scope->epilogue_start_offset >= fragment_offset)
> > +		last_fragment_scope = i + 1;
> > +	    }
> > +	}
> > +
> > +      seh_aarch64_xdata_header *header = &seh_ctx->aarch64_ctx.xdata_header;
> > +      const
> > +      seh_aarch64_epilogue_scope *scopes =
> > seh_ctx->aarch64_ctx.epilogue_scopes;
> > +
> > +      header->func_length = frag_size >> 2;
> 
> Another such magic number.

This is how the function length is encoded.
https://learn.microsoft.com/en-us/cpp/build/arm64-exception-handling?view=msvc-170#xdata-records

> > +      header->vers = 0;
> > +      header->e = 0;
> > +      header->code_words = 0;
> > +      header->epilogue_count = 0;
> > +
> > +      header->ext_code_words = 0;
> > +      header->ext_epilogue_count = last_fragment_scope
> > +				   - first_fragment_scope;
> > +      header->reserved = 0;
> > +
> > +      uint32_t first_epilog_index = 0;
> > +      uint32_t last_epilog_index = 0;
> > +      if (!header->ext_epilogue_count)
> > +	{
> > +	  first_epilog_index = prolog_size;
> > +	  last_epilog_index = prolog_size;
> > +	}
> > +      else
> > +	{
> > +	  const seh_aarch64_epilogue_scope *scope;
> > +	  scope = scopes + first_fragment_scope;
> > +	  first_epilog_index = scope->epilogue_start_index;
> > +	  if (last_fragment_scope == seh_ctx->aarch64_ctx.epilogue_scopes_count)
> > +	    last_epilog_index = seh_ctx->aarch64_ctx.unwind_codes_byte_count;
> > +	  else
> > +	    {
> > +	      scope = scopes + last_fragment_scope;
> > +	      last_epilog_index = scope->epilogue_start_index;
> > +	    }
> > +	}
> > +
> > +      uint32_t unwind_bytes = 0;
> > +      if (is_first_frag || is_last_frag)
> > +	unwind_bytes += prolog_size;
> 
> Why += when = would do?

The code is easily extendable with '+=' if needed.

> > +      if (header->ext_epilogue_count)
> > +	unwind_bytes += last_epilog_index - first_epilog_index;
> > +
> > +      if (is_fragmented_function && is_last_frag && unwind_bytes)
> > +	{
> > +	  unwind_bytes += 1;
> > +	  ++header->ext_epilogue_count;
> > +	}
> 
> Something like this (and perhaps quite a few other places) could really
> do with comments. Otherwise how is someone else supposed to be knowing
> why things are the way they are?

Extra comments will be added.
 
> > +      header->ext_code_words = (unwind_bytes  + 3) / 4;
> 
> Nit: Stray blank (and again magic numbers).

Another encoding in xdata. A comment will help. 
 
> > +      if ((header->ext_code_words == 0 && header->ext_epilogue_count == 0)
> > +	  || header->ext_code_words > 31
> > +	  || header->ext_epilogue_count > 31)
> > +	md_number_to_chars (frag_more (8),
> > +			   seh_ctx->aarch64_ctx.xdata_header_value, 8);
> > +      else
> > +	{
> > +	  header->code_words = header->ext_code_words;
> > +	  header->epilogue_count = header->ext_epilogue_count;
> > +	  if (header->epilogue_count == 1)
> > +	    {
> > +	      header->e = 1;
> > +	      if (is_fragmented_function && is_last_frag)
> > +		header->ext_epilogue_count = 0;
> > +	      else
> > +		{
> > +		  const seh_aarch64_epilogue_scope *scope;
> > +		  scope = scopes + first_fragment_scope;
> > +		  header->ext_epilogue_count = scope->epilogue_start_index;
> > +		}
> > +	    }
> > +	  out_four (seh_ctx->aarch64_ctx.xdata_header_value);
> > +	}
> > +
> > +      bool has_phantom_prolog = is_fragmented_function && is_last_frag;
> > +      if (header->ext_epilogue_count && !header->e)
> > +	{
> > +	  seh_aarch64_emit_epilog_scopes (seh_ctx,
> > +					 fragment_offset, prolog_size,
> > +					 first_fragment_scope,
> > +					 last_fragment_scope,
> > +					 has_phantom_prolog);
> > +	  if (is_fragmented_function && is_last_frag)
> > +	    {
> > +	      uint32_t epilog_start_offset;
> > +	      epilog_start_offset = frag_size - prolog_insruction_count * 4;
> > +	      md_number_to_chars (frag_more (4),
> > +				  (1 << 22) | (epilog_start_offset >> 2), 4);
> 
> And yet several more magic numbers.

Ok, it will be explained in comments.
 
> > @@ -1441,6 +1728,24 @@ write_function_pdata (seh_context *c)
> >    switch_pdata (c->code_seg);
> > 
> >  #if defined (COFFAARCH64)
> > +  if (c->aarch64_ctx.unwind_codes_byte_count)
> > +    {
> > +      seh_aarch64_func_fragment *fragment = &c->aarch64_ctx.func_fragment;
> > +      while (fragment)
> > +	{
> > +	  exp.X_op = O_symbol_rva;
> > +	  exp.X_add_number = fragment->offset;
> > +	  exp.X_add_symbol = c->start_addr;
> > +	  emit_expr (&exp, 4);
> > +
> > +	  exp.X_op = O_symbol_rva;
> > +	  /* TODO: Implementing packed unwind data.  */
> > +	  exp.X_add_number = 0;
> > +	  exp.X_add_symbol = fragment->xdata_addr;
> > +	  emit_expr (&exp, 4);
> > +	  fragment = fragment->next;
> > +	}
> > +    }
> 
> Wouldn't this better live ...
> 
> >  #else
> >    switch (seh_get_target_kind ())
> >      {
> 
> ... in the body of this switch()? Later on more #ifdef-ary could (imo should)
> then be added, to make sure we don't build dead code.

It will disappear after refactoring.

Regards,
Evgeny