Re: [PATCH v9 1/1] aarch64: Implement Structured Exception Handling (SEH) on AArch64

On Mon, 22 Jun 2026, Alice Carlotti wrote:
> On Wed, Jun 17, 2026 at 10:39:47AM +0200, Evgeny Karpov wrote:
> ...
> > +/* Write out the xdata information for one function.  */
> > +static void
> > +seh_aarch64_write_function_xdata (struct seh_aarch64_context *seh_ctx)
> 
> Oops, I forgot to write my comments on this functions.
> 
> In general, I think this function ought handle function fragmention, but the
> encoding details (including choosing between compact or extended header fields)
> should be handled in seh_aarch64_emit_xdata_record.  This separation of
> concerns should help with readability.
> 
> I think there are also several corner cases that are mishandled in this code,
> but I've been struggling to work out what's going on well enough to say that
> particular pieces are right or wrong.  I'll try to point any specific issues I
> see, but I'd appreciate if you could also see whether you can do anything more
> to improve the structure of this code.

Some checks will be moved to seh_aarch64_emit_xdata_record when it makes sense.
 
> > +  uintptr_t fragment_offset = 0;
> > +  unsigned first_fragment_scope = 0;
> > +  unsigned last_fragment_scope = 0;
> > +
> > +  /* Large functions (>= 1MB) will be split into multiple fragments.
> > +     However, it is expected the most of the functions will have only one
> > +     fragment. This loop iterates fragments and emit them.  */
> 
> See previous review comments about how this implementation doesn't currently
> support (or need to handle) fragmenting due to too many unwind codes or too
> many epilogue scopes.

The description with limitations will be extended mentioning this.

> > +      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;
> 
> It would be clearer to use separate variables here, and only combine them into
> a single header value within seh_aarch64_emit_xdata_record.   We can't do the
> type-punning anyway, so there's no benefit to putting the values into a struct.
> This also avoids any ambiguity about whether the short or extended fields
> should be used for computations in this function.

This can be done after validating big endian host if needed.
 
> > +
> > +      /* Calculate how many unwind bytes will be emitted in .xdata record.  */
> > +      unsigned unwind_bytes = prologue_size;
> > +
> > +      /* Check if current fragment has a phantom prologue. If yes, then
> > +	 the unwinding size should be adjusted.  */
> > +      const bool has_phantom_prologue = is_fragmented_function && is_last_frag;
> The is_last_frag part looks wrong - for a typical function layout, I'd expect
> only the first fragment to have a real prologue, and the rest to have a phantom
> prologue.

Ok, it will be validated and addressed if needed in v10.
 
> > +		{
> > +		  const seh_aarch64_epilogue_scope *scope;
> > +		  scope = scopes + first_fragment_scope;
> > +		  header->ext_epilogue_count = scope->epilogue_start_index;
> > +		}
> > +	    }
> > +	}
> This code is mostly selecting between different header formats, so I think it
> should be part of seh_aarch64_emit_xdata_record.

It will be moved to seh_aarch64_emit_xdata_record.

Regards,
Evgeny