[PATCH v7 2/3] Extract SEH shared helpers into separate file.

[Saurabh Jha]

Appreciate that you copy-pasted existing code. But I have some code 
comments that you might consider. They're no big deals and are not blockers.

On 3/19/2026 11:16 AM, Evgeny Karpov wrote:
> The patch moves SEH helpers to a separate shared file,
> which will be reused by the SEH implementation on AArch64.
>
> Signed-off-by: Evgeny Karpov <evgeny@kmaps.co>
>
> ChangeLog:
>
> 	* gas/config/obj-coff-seh.c (struct seh_seg_list): Move into
> 	  obj-coff-seh-shared.c.
> 	(get_pxdata_name): Likewise.
> 	(alloc_pxdata_item): Likewise.
> 	(make_pxdata_seg): Likewise.
> 	(seh_hash_insert): Likewise.
> 	(seh_hash_find): Likewise.
> 	(seh_hash_find_or_make): Likewise.
> 	(seh_validate_seg): Likewise.
> 	(switch_xdata): Likewise.
> 	(switch_pdata): Likewise.
> 	(verify_context): Likewise.
> 	(skip_whitespace_and_comma): Likewise.
> 	* gas/config/obj-coff-seh.h (OBJ_COFF_SEH_H): Add guard.
> 	(obj_coff_seh_code): Likewise.
> 	* gas/config/obj-coff.c: Update.
> 	* gas/config/obj-coff-seh-shared.c: New file.
> ---
>   gas/config/obj-coff-seh-shared.c | 214 +++++++++++++++++++++++++++++++
>   gas/config/obj-coff-seh.c        | 193 ----------------------------
>   gas/config/obj-coff-seh.h        |   5 +
>   gas/config/obj-coff.c            |   1 +
>   4 files changed, 220 insertions(+), 193 deletions(-)
>   create mode 100644 gas/config/obj-coff-seh-shared.c
>
> diff --git a/gas/config/obj-coff-seh-shared.c b/gas/config/obj-coff-seh-shared.c
> new file mode 100644
> index 00000000000..f3afd981aca
> --- /dev/null
> +++ b/gas/config/obj-coff-seh-shared.c
> @@ -0,0 +1,214 @@
> +/* Shared helpers for SEH .pdata/.xdata COFF object file format for
> +   multiple architectures.
> +   Copyright (C) 2026 Free Software Foundation, Inc.
> +
> +   This file is part of GAS.
> +
> +   GAS is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3, or (at your option)
> +   any later version.
> +
> +   GAS is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with GAS; see the file COPYING.  If not, write to the Free
> +   Software Foundation, 51 Franklin Street - Fifth Floor, Boston, MA
> +   02110-1301, USA.  */
> +
> +#include "obj-coff-seh.h"
> +typedef struct seh_context seh_context_t;
> +
> +/* Private segment collection list.  */
> +struct seh_seg_list {
> +  segT seg;
> +  subsegT subseg;
> +  char *seg_name;
> +};
> +
> +static seh_context_t *seh_ctx_cur = NULL;
> +
> +static htab_t seh_hash;
> +
> +static struct seh_seg_list *x_segcur = NULL;
> +static struct seh_seg_list *p_segcur = NULL;
> +
> +/* Build based on segment the derived .pdata/.xdata
> +   segment name containing origin segment's postfix name part.  */
> +static char *
> +get_pxdata_name (segT seg, const char *base_name)
> +{
> +  const char *name,*dollar, *dot;
> +  char *sname;
> +
> +  name = bfd_section_name (seg);
> +
> +  dollar = strchr (name, '$');
> +  dot = strchr (name + 1, '.');
> +
> +  if (!dollar && !dot)
> +    name = "";
> +  else if (!dollar)
> +    name = dot;
> +  else if (!dot)
> +    name = dollar;
> +  else if (dot < dollar)
> +    name = dot;
> +  else
> +    name = dollar;
> +
> +  sname = notes_concat (base_name, name, (const char *) NULL);
> +
> +  return sname;
> +}
> +
> +/* Allocate a seh_seg_list structure.  */
> +static struct seh_seg_list *
> +alloc_pxdata_item (segT seg, subsegT subseg, char *name)
> +{
> +  struct seh_seg_list *r;
> +
> +  r = notes_alloc (sizeof (struct seh_seg_list) + strlen (name));
> +  r->seg = seg;
> +  r->subseg = subseg;
> +  r->seg_name = name;
> +  return r;
> +}
> +
> +/* Generate pdata/xdata segment with same linkonce properties
> +   of based segment.  */
> +static segT
> +make_pxdata_seg (segT cseg, char *name)
> +{
> +  segT save_seg = now_seg;
> +  subsegT save_subseg = now_subseg;
> +  segT r;
> +  flagword flags;
> +
> +  r = subseg_new (name, 0);
> +  /* Check if code segment is marked as linked once.  */
> +  flags = (bfd_section_flags (cseg)
> +	   & (SEC_LINK_ONCE | SEC_LINK_DUPLICATES_DISCARD
> +	      | SEC_LINK_DUPLICATES_ONE_ONLY | SEC_LINK_DUPLICATES_SAME_SIZE
> +	      | SEC_LINK_DUPLICATES_SAME_CONTENTS));
> +
> +  /* Add standard section flags.  */
> +  flags |= SEC_ALLOC | SEC_LOAD | SEC_READONLY | SEC_DATA;
> +
> +  /* Apply possibly linked once flags to new generated segment, too.  */
> +  if (!bfd_set_section_flags (r, flags))
> +    as_bad (_("bfd_set_section_flags: %s"),
> +	    bfd_errmsg (bfd_get_error ()));
> +
> +  /* Restore to previous segment.  */
> +  subseg_set (save_seg, save_subseg);
> +  return r;
> +}
> +
> +static void
> +seh_hash_insert (const char *name, struct seh_seg_list *item)
> +{
> +  str_hash_insert (seh_hash, name, item, 1);
> +}
> +
> +static struct seh_seg_list *
> +seh_hash_find (char *name)
> +{
> +  return str_hash_find (seh_hash, name);
> +}
Why do we need these wrapper functions? seh_hash_insert and 
seh_hash_find. Is it a pattern to do it this way in binutils?
> +
> +static struct seh_seg_list *
> +seh_hash_find_or_make (segT cseg, const char *base_name)
> +{
> +  struct seh_seg_list *item;
> +  char *name;
> +
> +  /* Initialize seh_hash once.  */
> +  if (!seh_hash)
> +    seh_hash = str_htab_create ();

Why don't you move this "if" to the top and then declare and initialise 
item and name directly after that?

> +
> +  name = get_pxdata_name (cseg, base_name);
> +
> +  item = seh_hash_find (name);
Maybe add a comment saying this uses "seh_hash"? But then, why wouldn't 
you have it as parameter and get rid of the wrapper "seh_hash_find" 
altogether.
> +  if (!item)
> +    {
> +      item = alloc_pxdata_item (make_pxdata_seg (cseg, name), 0, name);
> +
> +      seh_hash_insert (item->seg_name, item);
> +    }
> +  else
> +    notes_free (name);
> +
> +  return item;
> +}
> +
> +/* Check if current segment has same name.  */
> +static int
> +seh_validate_seg (const char *directive)
> +{
> +  const char *cseg_name, *nseg_name;
> +  if (seh_ctx_cur->code_seg == now_seg)
> +    return 1;
Move this "if" to the top and initialize and declare variables cseg_name 
and nseg_name after that?
> +  cseg_name = bfd_section_name (seh_ctx_cur->code_seg);
> +  nseg_name = bfd_section_name (now_seg);
> +  as_bad (_("%s used in segment '%s' instead of expected '%s'"),
> +	  directive, nseg_name, cseg_name);
> +  ignore_rest_of_line ();
> +  return 0;
> +}
> +
> +/* Switch back to the code section, whatever that may be.  */
> +static void
> +switch_xdata (subsegT subseg, segT code_seg)
> +{
> +  x_segcur = seh_hash_find_or_make (code_seg, ".xdata");
> +
> +  subseg_set (x_segcur->seg, subseg);
> +}
> +
> +static void
> +switch_pdata (segT code_seg)
> +{
> +  p_segcur = seh_hash_find_or_make (code_seg, ".pdata");
> +
> +  subseg_set (p_segcur->seg, p_segcur->subseg);
> +}
> +
> +/* Verify that we're in the context of a seh_proc.  */
> +
> +static int
> +verify_context (const char *directive)
> +{
> +  if (seh_ctx_cur == NULL)
> +    {
> +      as_bad (_("%s used outside of .seh_proc block"), directive);
> +      ignore_rest_of_line ();
> +      return 0;
> +    }
> +  return 1;
> +}
> +
> +/* Skip whitespace and a comma.  Error if the comma is not seen.  */

Is that an extra space before "Error"?

> +
> +static int
> +skip_whitespace_and_comma (int required)
> +{
> +  SKIP_WHITESPACE ();
> +  if (*input_line_pointer == ',')
> +    {
> +      input_line_pointer++;
> +      SKIP_WHITESPACE ();
> +      return 1;
> +    }
> +  else if (required)
Since the above "if" returns. Can this be converted to another `if` as 
part of another if-else block?
> +    {
> +      as_bad (_("missing separator"));
> +      ignore_rest_of_line ();
> +    }
> +  else
> +    demand_empty_rest_of_line ();
> +  return 0;
> +}
> diff --git a/gas/config/obj-coff-seh.c b/gas/config/obj-coff-seh.c
> index 7479d3ee6b5..1ac64f8f9ba 100644
> --- a/gas/config/obj-coff-seh.c
> +++ b/gas/config/obj-coff-seh.c
> @@ -20,150 +20,9 @@
>   
>   #include "obj-coff-seh.h"
>   
> -
> -/* Private segment collection list.  */
> -struct seh_seg_list {
> -  segT seg;
> -  int subseg;
> -  char *seg_name;
> -};
> -
> -/* Local data.  */
> -static seh_context *seh_ctx_cur = NULL;
> -
> -static htab_t seh_hash;
> -
> -static struct seh_seg_list *x_segcur = NULL;
> -static struct seh_seg_list *p_segcur = NULL;
> -
>   static void write_function_xdata (seh_context *);
>   static void write_function_pdata (seh_context *);
>   
> -

> -/* Build based on segment the derived .pdata/.xdata
> -   segment name containing origin segment's postfix name part.  */
> -static char *
> -get_pxdata_name (segT seg, const char *base_name)
> -{
> -  const char *name,*dollar, *dot;
> -  char *sname;
> -
> -  name = bfd_section_name (seg);
> -
> -  dollar = strchr (name, '$');
> -  dot = strchr (name + 1, '.');
> -
> -  if (!dollar && !dot)
> -    name = "";
> -  else if (!dollar)
> -    name = dot;
> -  else if (!dot)
> -    name = dollar;
> -  else if (dot < dollar)
> -    name = dot;
> -  else
> -    name = dollar;
> -
> -  sname = notes_concat (base_name, name, (const char *) NULL);
> -
> -  return sname;
> -}
> -
> -/* Allocate a seh_seg_list structure.  */
> -static struct seh_seg_list *
> -alloc_pxdata_item (segT seg, int subseg, char *name)
> -{
> -  struct seh_seg_list *r;
> -
> -  r = notes_alloc (sizeof (struct seh_seg_list) + strlen (name));
> -  r->seg = seg;
> -  r->subseg = subseg;
> -  r->seg_name = name;
> -  return r;
> -}
> -
> -/* Generate pdata/xdata segment with same linkonce properties
> -   of based segment.  */
> -static segT
> -make_pxdata_seg (segT cseg, char *name)
> -{
> -  segT save_seg = now_seg;
> -  int save_subseg = now_subseg;
> -  segT r;
> -  flagword flags;
> -
> -  r = subseg_new (name, 0);
> -  /* Check if code segment is marked as linked once.  */
> -  flags = (bfd_section_flags (cseg)
> -	   & (SEC_LINK_ONCE | SEC_LINK_DUPLICATES_DISCARD
> -	      | SEC_LINK_DUPLICATES_ONE_ONLY | SEC_LINK_DUPLICATES_SAME_SIZE
> -	      | SEC_LINK_DUPLICATES_SAME_CONTENTS));
> -
> -  /* Add standard section flags.  */
> -  flags |= SEC_ALLOC | SEC_LOAD | SEC_READONLY | SEC_DATA;
> -
> -  /* Apply possibly linked once flags to new generated segment, too.  */
> -  if (!bfd_set_section_flags (r, flags))
> -    as_bad (_("bfd_set_section_flags: %s"),
> -	    bfd_errmsg (bfd_get_error ()));
> -
> -  /* Restore to previous segment.  */
> -  subseg_set (save_seg, save_subseg);
> -  return r;
> -}
> -
> -static void
> -seh_hash_insert (const char *name, struct seh_seg_list *item)
> -{
> -  str_hash_insert (seh_hash, name, item, 1);
> -}
> -
> -static struct seh_seg_list *
> -seh_hash_find (char *name)
> -{
> -  return str_hash_find (seh_hash, name);
> -}
> -
> -static struct seh_seg_list *
> -seh_hash_find_or_make (segT cseg, const char *base_name)
> -{
> -  struct seh_seg_list *item;
> -  char *name;
> -
> -  /* Initialize seh_hash once.  */
> -  if (!seh_hash)
> -    seh_hash = str_htab_create ();
> -
> -  name = get_pxdata_name (cseg, base_name);
> -
> -  item = seh_hash_find (name);
> -  if (!item)
> -    {
> -      item = alloc_pxdata_item (make_pxdata_seg (cseg, name), 0, name);
> -
> -      seh_hash_insert (item->seg_name, item);
> -    }
> -  else
> -    notes_free (name);
> -
> -  return item;
> -}
> -
> -/* Check if current segment has same name.  */
> -static int
> -seh_validate_seg (const char *directive)
> -{
> -  const char *cseg_name, *nseg_name;
> -  if (seh_ctx_cur->code_seg == now_seg)
> -    return 1;
> -  cseg_name = bfd_section_name (seh_ctx_cur->code_seg);
> -  nseg_name = bfd_section_name (now_seg);
> -  as_bad (_("%s used in segment '%s' instead of expected '%s'"),
> -  	  directive, nseg_name, cseg_name);
> -  ignore_rest_of_line ();
> -  return 0;
> -}
> -
>   /* Switch back to the code section, whatever that may be.  */
>   static void
>   obj_coff_seh_code (int ignored ATTRIBUTE_UNUSED)
> @@ -171,22 +30,6 @@ obj_coff_seh_code (int ignored ATTRIBUTE_UNUSED)
>     subseg_set (seh_ctx_cur->code_seg, 0);
>   }
>   
> -static void
> -switch_xdata (int subseg, segT code_seg)
> -{
> -  x_segcur = seh_hash_find_or_make (code_seg, ".xdata");
> -
> -  subseg_set (x_segcur->seg, subseg);
> -}
> -
> -static void
> -switch_pdata (segT code_seg)
> -{
> -  p_segcur = seh_hash_find_or_make (code_seg, ".pdata");
> -
> -  subseg_set (p_segcur->seg, p_segcur->subseg);
> -}
> -

>   /* Parsing routines.  */
>   
>   /* Return the style of SEH unwind info to generate.  */
> @@ -242,20 +85,6 @@ verify_target (const char *directive)
>     return true;
>   }
>   
> -/* Verify that we're in the context of a seh_proc.  */
> -
> -static int
> -verify_context (const char *directive)
> -{
> -  if (seh_ctx_cur == NULL)
> -    {
> -      as_bad (_("%s used outside of .seh_proc block"), directive);
> -      ignore_rest_of_line ();
> -      return 0;
> -    }
> -  return 1;
> -}
> -
>   /* Similar, except we also verify the appropriate target.  */
>   
>   static int
> @@ -270,28 +99,6 @@ verify_context_and_target (const char *directive, seh_kind target)
>     return verify_context (directive);
>   }
>   
> -/* Skip whitespace and a comma.  Error if the comma is not seen.  */
> -
> -static int
> -skip_whitespace_and_comma (int required)
> -{
> -  SKIP_WHITESPACE ();
> -  if (*input_line_pointer == ',')
> -    {
> -      input_line_pointer++;
> -      SKIP_WHITESPACE ();
> -      return 1;
> -    }
> -  else if (required)
> -    {
> -      as_bad (_("missing separator"));
> -      ignore_rest_of_line ();
> -    }
> -  else
> -    demand_empty_rest_of_line ();
> -  return 0;
> -}
> -
>   /* Mark current context to use 32-bit instruction (arm).  */
>   
>   static void
> diff --git a/gas/config/obj-coff-seh.h b/gas/config/obj-coff-seh.h
> index d926f314ec5..dd75909dec3 100644
> --- a/gas/config/obj-coff-seh.h
> +++ b/gas/config/obj-coff-seh.h
> @@ -59,6 +59,9 @@
>     .seh_code
>   */
>   
> +#ifndef OBJ_COFF_SEH_H
> +#define OBJ_COFF_SEH_H
> +
>   /* architecture specific pdata/xdata handling.  */
>   #define SEH_CMDS \
>           {"seh_proc", obj_coff_seh_proc, 0}, \
> @@ -202,3 +205,5 @@ static void obj_coff_seh_code (int);
>      PEX64_SCOPE_ENTRY_SIZE * (IDX))
>   
>   #endif
> +
> +#endif /* OBJ_COFF_SEH_H.  */
> diff --git a/gas/config/obj-coff.c b/gas/config/obj-coff.c
> index 30e3ca2e2c8..7732c0af911 100644
> --- a/gas/config/obj-coff.c
> +++ b/gas/config/obj-coff.c
> @@ -54,6 +54,7 @@ static symbolS *def_symbol_in_progress;
>   static const char weak_altprefix[] = ".weak.";
>   #endif /* TE_PE */
>   
> +#include "obj-coff-seh-shared.c"
>   #include "obj-coff-seh.c"
>   
>   typedef struct