[PATCH v4 3/7] Adjust x64 SEH implementation for AArch64
Evgeny Karpov
evgeny.karpov@arm.com
Fri Jan 30 17:46:51 GMT 2026
On Fri, Oct 10, 2025 Jan Beulich wrote:
> 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.
Splitting SEH implementations for x64 and aarch64 into two separate source files
should look better. Also it should prevent regressions in x64 implementation
because the x64 files will be only renamed.
Here is a proposal for the file names:
obj-coff-seh.h (common)
obj-coff-seh-i386.h / obj-coff-seh-i386.c
and obj-coff-seh-aarch64.h / obj-coff-seh-aarch64.c
>> --- 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).
The initializers will be changed.
>> @@ -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.
ATTRIBUTE_UNUSED will be used.
>> @@ -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?)
seh_ctx_cur is NULL in the aarch64 implementation, as data has postponed
emitting to xdata. in_seh_proc is a reliable way to validate the scope in both
implementations.
As mentioned above, this implementation will stay the same in x64, and it will
be different for aarch64 after separating architectures into different files.
>> @@ -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?
It indicates the presence of exception data, which is always emitted.
>> +#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.)
The implementation will be separated into different source files. It will
simplify readability.
>> @@ -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 *.
Ok.
>> + 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?
A good point. The count increment will be moved after the check.
>> + 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.
Ok.
>> + 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?
Yes, it should not be the case in the next submission.
>> + 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?
Yes, it is correct, and it will be changed.
>> @@ -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?
>
Refactoring might change the presence of in_seh_proc. For now, it looks like it
is needed.
>> @@ -461,15 +640,45 @@ obj_coff_seh_proc (int what ATTRIBUTE_UNUSED)
>> return;
>> }
>>
>> +
>
> Nit: Please don't add double blank lines.
Ok.
>> +#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.
A comment will be added.
>> @@ -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 ...
>
It will be changed to unsigned.
>> + {
>> + 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?
>
Ok, It will be moved inside.
>> @@ -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?
Yes, as an option, it can be moved. Anyway, multithreading is not supported
by this implementation.
>
>> @@ -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?
The first condition is needed only for x64, otherwise it should be modified.
However, the decision has been made to have a static implementation for aarch64.
The second place is needed for a call of two different architecture specific
implementations.
>> @@ -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.
The same here. The difference between dynamic implementation for x64
and static implementation for aarch64.
>> --- 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?
>
As an option, yes. It will be refactored and split.
>> +#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.)
Ok, new commands will be documented.
>> @@ -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.
A good point. It will stay the same for x64, however for aarch64 they will not
be declared if it is not necessary.
Regards,
Evgeny
More information about the Binutils
mailing list