Home Original page

[PATCH v4 3/7] Adjust x64 SEH implementation for AArch64

Jan Beulich jbeulich@suse.com
Fri Oct 10 13:55:39 GMT 2025 
On 15.08.2025 00:47, Evgeny Karpov wrote:
> This patch defines the required unwind codes for AArch64 and updates handlers
> for SEH commands that are used by both x64 and AArch64.
> It implements encoding for unwind elements that will be emitted
> to pdata/xdata sections.
> The x64 SEH implementation emits .pdata/.xdata records at every seh_endproc.
> This has been adjusted to keep the x64 SEH implementation unchanged and to
> allow the aarch64 SEH to emit .pdata/.xdata records at the end of the assembly.

Which needs to be this way because of ...?

> gas/ChangeLog:
> 
> 	* config/obj-coff-seh.c (struct aarch64_unwind_code_pack_info): New.
> 	(defined): Add aarch64_unwind_code_pack_data.
> 	(obj_coff_seh_code): Add COFFAARCH64 guard.
> 	(seh_get_target_kind): Update.
> 	(verify_context): Update.
> 	(verify_context_and_target):  Update.
> 	(obj_coff_seh_eh): Add COFFAARCH64 guard.
> 	(seh_aarch64_add_unwind_element): New.
> 	(do_seh_endproc): Remove.

Resulting in more open-coding / duplicate code at the two prior call sites.
Tolerable, but not very nice.

> --- a/gas/config/obj-coff-seh.c
> +++ b/gas/config/obj-coff-seh.c
> @@ -28,14 +28,86 @@ struct seh_seg_list {
>    char *seg_name;
>  };
> 
> +struct aarch64_unwind_code_pack_info {
> +  const char *directive;
> +  unsigned offset_bits;
> +  unsigned reg_bits;
> +  unsigned code_bits;
> +  unsigned code;
> +  unsigned offset_right_shift;
> +  unsigned offset;
> +  unsigned reg_right_shift;
> +  unsigned reg_offset;
> +  unsigned type;
> +  unsigned size;
> +};
> +
>  /* Local data.  */
> +#if defined (COFFAARCH64)
> +static seh_context *seh_ctx_root = NULL;
> +#endif
>  static seh_context *seh_ctx_cur = NULL;
> +static bool in_seh_proc = false;
> 
>  static htab_t seh_hash;
> 
>  static struct seh_seg_list *x_segcur = NULL;
>  static struct seh_seg_list *p_segcur = NULL;
> 
> +#if defined (COFFAARCH64)
> +static const struct aarch64_unwind_code_pack_info
> +aarch64_unwind_code_pack_data[] = {
> +/* Unwind codes packing for AArch64 is described at
> +   https://learn.microsoft.com/en-us/cpp/build/arm64-exception-handling?view=msvc-170#unwind-codes
> +   and calculated in seh_aarch64_add_unwind_element function.
> +
> +   directive, offset_bits, reg_bits, code_bits, code, offset_right_shift,
> +   offset, reg_right_shift, reg_offset, type, size.  */
> +  {NULL,		  5, 0, 3, AARCH64_UNOP_ALLOCS,      4, 0, 0,
> +   0,  alloc_s, 1},

Now that we've been using C99 for a while, can't such initializers become
dedicated ones? That'll be bigger code size, yes, but far easier to follow
(and, if need be, edit).

> @@ -227,18 +301,24 @@ seh_get_target_kind (void)
>      }
>    return seh_kind_unknown;
>  }
> +#endif
> 
>  /* Verify that seh directives are supported.  */
> 
>  static bool
>  verify_target (const char *directive)
>  {
> +#if defined (COFFAARCH64)
> +  (void) directive; /* Unused.  */

Please use ATTRIBUTE_UNUSED instead.

> @@ -247,7 +327,7 @@ verify_target (const char *directive)
>  static int
>  verify_context (const char *directive)
>  {
> -  if (seh_ctx_cur == NULL)
> +  if (!in_seh_proc)
>      {
>        as_bad (_("%s used outside of .seh_proc block"), directive);
>        ignore_rest_of_line ();

I fear I don't understand why this change would be needed. (Can it
be broken out as a separate change, with a proper description?)

> @@ -366,10 +450,21 @@ obj_coff_seh_handler (int what ATTRIBUTE_UNUSED)
>    seh_ctx_cur->handler_data.X_add_number = 0;
>    seh_ctx_cur->handler_flags = 0;
> 
> +#if defined (COFFAARCH64)
> +  seh_ctx_cur->aarch64_ctx.xdata_header.x = 1;

Why unconditionally 1?

> +#endif
> +
>    if (!skip_whitespace_and_comma (0))
>      return;
> 
> -  if (seh_get_target_kind () == seh_kind_x64)
> +  bool seh_handler_supported;
> +#if defined (COFFAARCH64)
> +  seh_handler_supported = true;
> +#else
> +  seh_handler_supported = seh_get_target_kind () == seh_kind_x64;
> +#endif
> +
> +  if (seh_handler_supported)
>      {

Easier (less churn and less code volume) as

#if !defined (COFFAARCH64)
  if (seh_handler_supported)
#endif
    {

(Such an #if may want to have a brief comment attached, though.)

> @@ -401,26 +496,97 @@ obj_coff_seh_handler (int what ATTRIBUTE_UNUSED)
>  static void
>  obj_coff_seh_handlerdata (int what ATTRIBUTE_UNUSED)
>  {
> +#if !defined (COFFAARCH64)
>    if (!verify_context_and_target (".seh_handlerdata", seh_kind_x64))
>      return;
> +#endif
>    demand_empty_rest_of_line ();
> 
>    switch_xdata (seh_ctx_cur->subsection + 1, seh_ctx_cur->code_seg);
>  }
> 
> +#if defined (COFFAARCH64)
> +/* Obtain available unwind element.  */
> +
> +static void
> +seh_aarch64_add_unwind_element (seh_aarch64_unwind_types unwind_type,
> +				int offset, int reg)
> +{
> +  gas_assert (in_seh_proc);
> +
> +  seh_aarch64_context* aarch64_ctx = &seh_ctx_cur->aarch64_ctx;

Nit: Misplaced *.

> +  seh_aarch64_unwind_code *aarch64_element;
> +  aarch64_element = aarch64_ctx->unwind_codes
> +		    + aarch64_ctx->unwind_codes_count++;

Is it wise to update the count before any error checking?

> +  const struct aarch64_unwind_code_pack_info *unwind_code_pack_info;
> +  unwind_code_pack_info = aarch64_unwind_code_pack_data + unwind_type;
> +  aarch64_element->value = 0;
> +  int value_offset_bits = 0;
> +
> +  const unsigned max_unwind_size = AARCH64_MAX_UNWIND_CODES_SIZE;
> +  if ((aarch64_ctx->unwind_codes_byte_count
> +      + unwind_code_pack_info->size) > max_unwind_size)
> +    {
> +      as_bad (_ ("no unwind element available."));

Nit: No full stop on diagnostics please.

> +      return;
> +    }
> +
> +  if (unwind_code_pack_info->offset_bits)
> +    {
> +      offset = (offset >> unwind_code_pack_info->offset_right_shift)
> +	       - unwind_code_pack_info->offset;
> +      offset &= (1 << unwind_code_pack_info->offset_bits) - 1;

Stray / odd "<<" - a problem with your email UI?

> +      aarch64_element->value |= offset << value_offset_bits;
> +      value_offset_bits += unwind_code_pack_info->offset_bits;
> +    }
> +
> +  if (unwind_code_pack_info->reg_bits)
> +    {
> +      reg = (reg >> unwind_code_pack_info->reg_right_shift)
> +	    - unwind_code_pack_info->reg_offset;
> +      reg &= (1 << unwind_code_pack_info->reg_bits) - 1;
> +      aarch64_element->value |= reg << value_offset_bits;
> +      value_offset_bits += unwind_code_pack_info->reg_bits;
> +    }
> +
> +  if (unwind_code_pack_info->code_bits)
> +    {
> +      int code = unwind_code_pack_info->code;

Likely unsigned int, seeing ...

> +      code &= (1 << unwind_code_pack_info->code_bits) - 1;
> +      aarch64_element->value |= code << value_offset_bits;

... it's used?

> @@ -429,13 +595,21 @@ obj_coff_seh_endproc (int what ATTRIBUTE_UNUSED)
>    if (!verify_target (".seh_endproc"))
>      return;
>    demand_empty_rest_of_line ();
> -  if (seh_ctx_cur == NULL)
> +  if (!in_seh_proc)
>      {
>        as_bad (_(".seh_endproc used without .seh_proc"));
>        return;
>      }
>    seh_validate_seg (".seh_endproc");
> -  do_seh_endproc ();
> +
> +  seh_ctx_cur->end_addr = symbol_temp_new_now ();
> +#if !defined (COFFAARCH64)
> +  emit_pdata_xdata_records (seh_ctx_cur);
> +  free_seh_ctx (seh_ctx_cur);
> +  seh_ctx_cur = NULL;

Related to an earlier remark: If this was moved ...

> +#endif

.. below here, wouldn't ...

> +
> +  in_seh_proc = false;

... this new variable become unnecessary?

> @@ -461,15 +640,45 @@ obj_coff_seh_proc (int what ATTRIBUTE_UNUSED)
>        return;
>      }
> 
> +

Nit: Please don't add double blank lines.

> +#if defined (COFFAARCH64)
> +  if (!seh_ctx_root)
> +  {
> +    seh_ctx_root = XCNEW (seh_context);
> +    seh_ctx_cur = seh_ctx_root;
> +  }
> +  else
> +  {
> +    seh_ctx_cur->next = XCNEW (seh_context);
> +    seh_ctx_cur = seh_ctx_cur->next;
> +  }
> +
> +  seh_ctx_cur->next = NULL;
> +#else
>    seh_ctx_cur = XCNEW (seh_context);
> +#endif
> 
>    seh_ctx_cur->code_seg = now_seg;
> 
> -  if (seh_get_target_kind () == seh_kind_x64)
> +  bool use_xdata;
> +#if defined (COFFAARCH64)
> +  use_xdata = true;
> +#else
> +  use_xdata = seh_get_target_kind () == seh_kind_x64;
> +#endif
> +
> +  if (use_xdata)

See an earlier remark on how to simplify this.

>      {
>        x_segcur = seh_hash_find_or_make (seh_ctx_cur->code_seg, ".xdata");
>        seh_ctx_cur->subsection = x_segcur->subseg;
>        x_segcur->subseg += 2;
> +
> +#if defined (COFFAARCH64)
> +      seh_ctx_cur->aarch64_ctx.unwind_codes_count = 0;
> +      seh_ctx_cur->aarch64_ctx.epilogue_scopes_count = 0;
> +      seh_ctx_cur->aarch64_ctx.epilogue_scopes_capacity = 0;
> +      seh_ctx_cur->aarch64_ctx.epilogue_scopes = NULL;
> +#endif

This also could do with a brief comment.

> @@ -481,6 +690,7 @@ obj_coff_seh_proc (int what ATTRIBUTE_UNUSED)
>    demand_empty_rest_of_line ();
> 
>    seh_ctx_cur->start_addr = symbol_temp_new_now ();
> +  in_seh_proc = true;
>  }
> 
>  /* Mark end of prologue for current context.  */
> @@ -498,6 +708,22 @@ obj_coff_seh_endprologue (int what ATTRIBUTE_UNUSED)
>      as_warn (_("duplicate .seh_endprologue in .seh_proc block"));
>    else
>      seh_ctx_cur->endprologue_addr = symbol_temp_new_now ();
> +
> +#if defined (COFFAARCH64)
> +  const int n = seh_ctx_cur->aarch64_ctx.unwind_codes_count;

I question the use of plain int in cases like this and ...

> +  /* Unwind codes need to be reversed.  */
> +  for (int i = 0; i < n / 2; ++i)

... this one. Apart from this - n isn't ...

> +    {
> +      seh_aarch64_unwind_code *unwind_codes;
> +      unwind_codes = seh_ctx_cur->aarch64_ctx.unwind_codes;
> +      seh_aarch64_unwind_code temp = unwind_codes[i];
> +      unwind_codes[i] = unwind_codes[n-i-1];
> +      unwind_codes[n-i-1] = temp;
> +    }
> +
> +   seh_aarch64_add_unwind_element (end, 0, 0);
> +#endif
>  }

... needed outside the loop either, so why is it not restricted to the
loop just like i is?

> @@ -505,10 +731,27 @@ obj_coff_seh_endprologue (int what ATTRIBUTE_UNUSED)
>  void
>  obj_coff_seh_do_final (void)
>  {
> -  if (seh_ctx_cur != NULL)
> +  if (in_seh_proc)
>      as_bad (_("open SEH entry at end of file (missing .seh_endproc)"));
> +
> +#if defined (COFFAARCH64)
> +
> +  if (!seh_ctx_root)
> +    return;
> +
> +  struct seh_context *seh_ctx = seh_ctx_root;
> +  while (seh_ctx)
> +  {
> +    emit_pdata_xdata_records (seh_ctx);
> +    struct seh_context *next = seh_ctx->next;
> +    free_seh_ctx (seh_ctx);
> +    seh_ctx = next;
> +  }
> +  seh_ctx_root = NULL;

Move this up to prevent the value becoming a dangling pointer during the 1st
loop iteration?

> @@ -938,17 +1209,23 @@ write_function_xdata (seh_context *c)
>    segT save_seg = now_seg;
>    int save_subseg = now_subseg;
> 
> +#if !defined (COFFAARCH64)

You use this here and elsewhere; why ...

>    /* MIPS, SH, ARM don't have xdata.  */
>    if (seh_get_target_kind () != seh_kind_x64)
>      return;
> +#endif
> 
>    switch_xdata (c->subsection, c->code_seg);
> 
> +#if defined (COFFAARCH64)
> +#else

... not also here?

> @@ -1011,6 +1289,8 @@ write_function_pdata (seh_context *c)
>    memset (&exp, 0, sizeof (expressionS));
>    switch_pdata (c->code_seg);
> 
> +#if defined (COFFAARCH64)
> +#else

Same question here.

> --- a/gas/config/obj-coff-seh.h
> +++ b/gas/config/obj-coff-seh.h
> @@ -112,6 +112,17 @@ typedef enum seh_aarch64_unwind_types
>  	{"seh_code", obj_coff_seh_code, 0}, \
>  	{"seh_handlerdata", obj_coff_seh_handlerdata, 0},
> 
> +#if defined (COFFAARCH64)
> +#undef SEH_CMDS

Better avoid the need to use #undef by using #if / #else?

> +#define SEH_CMDS \
> +	{"seh_proc", obj_coff_seh_proc, 0}, \
> +	{"seh_endproc", obj_coff_seh_endproc, 0}, \
> +	{"seh_endprologue", obj_coff_seh_endprologue, 0}, \
> +	{"seh_stackalloc", obj_coff_seh_stackalloc, 0}, \
> +	{"seh_handler", obj_coff_seh_handler, 0}, \
> +	{"seh_handlerdata", obj_coff_seh_handlerdata, 0},
> +#endif

While I realize pre-existing .seh_* directives lack documentation, I
think we better wouldn't continue this with this new addition. (This isn't
a request to document what was there before, just one to document what is
being added.)

> @@ -271,18 +282,20 @@ typedef enum seh_kind {
> 
>  /* Forward declarations.  */
>  static void obj_coff_seh_stackalloc (int);
> -static void obj_coff_seh_setframe (int);
>  static void obj_coff_seh_endprologue (int);
> +static void obj_coff_seh_endproc (int);
> +static void obj_coff_seh_proc (int);
> +static void obj_coff_seh_handler (int);
> +static void obj_coff_seh_handlerdata (int);
> +#if !defined (COFFAARCH64)
> +static void obj_coff_seh_setframe (int);
>  static void obj_coff_seh_save (int);
>  static void obj_coff_seh_pushreg (int);
>  static void obj_coff_seh_pushframe (int);
> -static void obj_coff_seh_endproc  (int);
>  static void obj_coff_seh_eh (int);
>  static void obj_coff_seh_32 (int);
> -static void obj_coff_seh_proc  (int);
> -static void obj_coff_seh_handler (int);
> -static void obj_coff_seh_handlerdata (int);
>  static void obj_coff_seh_code (int);
> +#endif

Can't all of these be deleted (in a separate, pre-req patch)? obj-coff.c
includes obj-coff-seh.c well ahead of expanding SEH_CMDS.

Jan
More information about the Binutils mailing list