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

On Fri, 26 Jun 2026, Alice Carlotti wrote:
> > Changes in v10:
> > - Rename offset_shift/reg_shift to offset_multiplier/reg_multiplier.
> I think you misinterpreted my review - I wanted you to rename the local
> variables in seh_aarch64_add_unwind_element:
> divided_by -> offset_multipler
> regn_divided_by -> reg_multiplier
> 
> The offset_shift and reg_shift field names were fine, and the new names are
> wrong (because the values are shift amounts, not multipliers).  Please change
> the field names back to offset_shift and reg_shift.

Ok, it was not clear in the previous comment. 
 
> > Yes, something should be wrong if func_size < 0, no need to check if func_size >= 0.
> 
> Can you explain why there's no need to check when func_size >= 0?  It looks to
> me like we could end up in that case without the function size being fully
> evaluated, but I don't really understand what this is checking.
 
func_size had mostly been calculated correctly in the past,
except in some cases when the alignment was not handled correctly.
In that case, func_size was calculated as a negative value.
The issue with the code alignment has been fixed, however 
that condition still checks that case.

> > The description will be extended with text that big endian host has not been validated.
> > It will be validated, and a separate patch will be submitted if needed. 
> 
> I think deliberately committing code that is broken for a big endian host, and
> then testing and fixing it later, is the wrong approach when doing it correctly
> is no more complex (and, in my opinion, might be a bit clearer).

It does not look like a common case for aarch64-w64-mingw32 target,
however it will be validated, and required changes will be done if needed.
 
> > +      reg = atoi (symbol_name + 1);
> > +      (void) restore_line_pointer (name_end);
> > +
> > +      if (!skip_whitespace_and_comma (1))
> > +	return;
> > +
> > +      /* Check that reference registers are not higher than x30.  */
> 
> Nit: s/reference/referenced/

Ok.

> > +
> > +  /* 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;
> This condition is still wrong.
  
Thanks for noticing this. There was an issue with phantom encoding. 

> > +      header->code_words = header->ext_code_words;
> > +      header->epilogue_count = header->ext_epilogue_count;
> > +      if (header->epilogue_count == 1)
> > +	{
> > +	  header->e = 1;
> > +	  if (has_phantom_prologue)
> > +	    header->ext_epilogue_count = 0;
> This looks wrong - surely the start index is 1 in this case (with unwind code
> index 0 being end_c, to indicate the phantom prologue).

That was fixed for the phantom prolog. The start index was correct, however 
it should be encoded in epilogue_count.
header->epilogue_count = 0;
 
> > +	      header->ext_epilogue_count = scope->epilogue_start_index;
> > +	    }
> > +	}
> > +    }
> > +
> > +  md_number_to_chars (frag_more (header_size), seh_ctx->xdata_header_value,
> > +		      header_size);
> 
> I think we should just build up the headers manually - this avoids endianess or
> other platform-specific issues, and simplifies the code a bit.  So something
> like:
> 
> +  unsigned code_words = ((unwind_bytes + 3) >> 2);
> +
> +  /* Compute and emit the xdata record header.  */
> +  uint32_t header = frag_size >> 2;
> +  if (seh_ctx->has_exception_data)
> +    header |= 1 << 20;
> +  if (epilogue_count <= 31 && code_words <= 31)
> +    {
> +      header |= epilogue_count << 22;
> +      header |= code_words << 27;
> +      md_number_to_chars (frag_more (4), header, 4));
> +    }
> +  else
> +    {
> +      md_number_to_chars (frag_more (4), header, 4));
> +      uint32_t header2 = epilogue_count | (code_words << 16)
> +      md_number_to_chars (frag_more (4), header2, 4));
> +    }
> 
> This doesn't use the E=1 format (which only saves 4 bytes) - if we wanted to
> include that then we'd need to add an initial check for:
> +  if (epilogue_count == 1 && code_words <= 31
> +      && /* Check epilogue start index <= 31.  */
> +      && /* Check epilogue is at the end of the function - this doesn't seem
> to be documented, but it's the only sensible way I can see to specify the
> offset in this case.  */)
> 
> However, I can't see clear documentation for how this works, so it might be
> better to ignore the E=1 format for now, and seek clarification before
> implementing it.

epilogue_count <= 31 && code_words <=31
This condition does not cover a case when epilogue_count == 0 && code_words == 0.
It should use the extended header in that case.
It looks like if E=1 and both code words and epilogue count are 0, the extended 
header should be used anyway.
 
> > +
> > +      /* Calculate current fragment size.  */
> > +      uintptr_t frag_size = func_size - fragment_offset;
> > +      if (frag_size > max_frag_size)
> > +	frag_size = max_frag_size;
> > +
> > +      const bool is_last_frag = (fragment_offset + frag_size) == func_size;
> This value shouldn't be needed in the function call, so we can just inline this
> condition into the loop termination check.

The condition will be inlined.

> > +
> > +  subsegT subsection;
> > +
> > +  union {
> > +    seh_aarch64_xdata_header xdata_header;
> > +    valueT xdata_header_value;
> > +  };
> The only header field that is now used outside of seh_aarch64_emit_xdata_record
> is xdata_header->x.  Please replace this union with a single boolean value
> seh_ctx_cur->has_exception_data (or some similar name).  We can keep details
> of the header layout contained to the single function that writes those other
> fields.
 
Ok, it will be changed.

Regards,
Evgeny