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

[Jan Beulich]

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?

> +      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?

> +    {
> +      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?

> +    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.

> @@ -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?

> +{
> +  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?

> +  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?

> +  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.

> +  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?

> +  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.

> +	{
> +	  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.)

> +  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.

> +      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?

> +      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?

> +      header->ext_code_words = (unwind_bytes  + 3) / 4;

Nit: Stray blank (and again magic numbers).

> +      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.

> @@ -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.

Jan