[PATCH 3/4] ELF: SHF_STRINGS isn't really tied to SHF_MERGE

[Richard Sandiford]

Jan Beulich <jbeulich@suse.com> writes:
> It's not overly useful without it, but the spec doesn't name any
> dependency between the two. People may want to use it for purely
> informational purposes, for example. Adjust, in particular, entity size
> processing to be engaged if either flag is set, as mandated by the spec.
>
> --- a/bfd/elf.c
> +++ b/bfd/elf.c
> @@ -846,7 +846,10 @@ _bfd_elf_make_section_from_shdr (bfd *ab
>        newsect->entsize = hdr->sh_entsize;
>      }
>    if ((hdr->sh_flags & SHF_STRINGS) != 0)
> -    flags |= SEC_STRINGS;
> +    {
> +      flags |= SEC_STRINGS;
> +      newsect->entsize = hdr->sh_entsize;
> +    }
>    if ((hdr->sh_flags & SHF_TLS) != 0)
>      flags |= SEC_THREAD_LOCAL;
>    if ((hdr->sh_flags & SHF_EXCLUDE) != 0)
> @@ -3709,7 +3712,10 @@ elf_fake_sections (bfd *abfd, asection *
>        this_hdr->sh_entsize = asect->entsize;
>      }
>    if ((asect->flags & SEC_STRINGS) != 0)
> -    this_hdr->sh_flags |= SHF_STRINGS;
> +    {
> +      this_hdr->sh_flags |= SHF_STRINGS;
> +      this_hdr->sh_entsize = asect->entsize;
> +    }
>    if ((asect->flags & SEC_GROUP) == 0 && elf_group_name (asect) != NULL)
>      this_hdr->sh_flags |= SHF_GROUP;
>    if ((asect->flags & SEC_THREAD_LOCAL) != 0)
> --- a/gas/doc/as.texi
> +++ b/gas/doc/as.texi
> @@ -7001,17 +7001,18 @@ Note - some sections, eg @code{.text} an
>  special and have fixed types.  Any attempt to declare them with a different
>  type will generate an error from the assembler.
>  
> -If @var{flags} contains the @code{M} symbol then the @var{type} argument must
> -be specified as well as an extra argument---@var{entsize}---like this:
> +If @var{flags} contains the @code{M} and/or @code{S} symbol then the @var{type}
> +argument must be specified as well as an extra argument---@var{entsize}---like
> +this:
>  
>  @smallexample
>  .section @var{name} , "@var{flags}"M, @@@var{type}, @var{entsize}
>  @end smallexample
>  
>  Sections with the @code{M} flag but not @code{S} flag must contain fixed size
> -constants, each @var{entsize} octets long. Sections with both @code{M} and
> -@code{S} must contain zero terminated strings where each character is
> -@var{entsize} bytes long. The linker may remove duplicates within sections with
> +constants, each @var{entsize} octets long. Sections with @code{S} must contain
> +zero terminated strings where each character is @var{entsize} bytes long. For
> +@code{M} sections the linker may remove duplicates within sections with
>  the same name, same entity size and same flags.  @var{entsize} must be an
>  absolute expression.  For sections with both @code{M} and @code{S}, a string
>  which is a suffix of a larger string is considered a duplicate.  Thus

We have a codebase that made a lot of use of constructs like:

	.section .rodata.foo, "aS"
foo:
        .asciz "..."

which after this patch need to become:

	.section .rodata.foo, "aS", @progbits, 1
foo:
        .asciz "..."

The codebase is being changed to use the longer form.  But it kind-of
feels a bit redundant.  How about assuming that the entry size for
a string is 1 (i.e. that the string is has single-byte characters)
if no size is explicitly given, just like we assume @progbits for most
sections when no type is given?

If instead we require users to update their code, it would be worth
mentioning the change in gas/NEWS.

Thanks,
Richard


> @@ -7031,8 +7032,8 @@ is not generally a good idea as section
>  time, but the facility is provided for testing purposes.  An index of zero is
>  allowed.  It indicates that the linked-to section has already been discarded.
>  
> -Note: If both the @var{M} and @var{o} flags are present, then the fields
> -for the Merge flag should come first, like this:
> +Note: If both one of @var{M} or @var{S} and @var{o} flags are present, then the
> +fields for the Merge/String flag should come first, like this:
>  
>  @smallexample
>  .section @var{name},"@var{flags}"Mo,@@@var{type},@var{entsize},@var{SymbolName}
> @@ -7055,8 +7056,8 @@ indicates that only one copy of this sec
>  an alias for comdat
>  @end table
>  
> -Note: if both the @var{M} and @var{G} flags are present then the fields for
> -the Merge flag should come first, like this:
> +Note: Uf both one of @var{M} or @var{S} and @var{G} flags are present then the
> +fields for the Merge/String flag should come first, like this:
>  
>  @smallexample
>  .section @var{name} , "@var{flags}"MG, @@@var{type}, @var{entsize}, @var{GroupName}[, @var{linkage}]
> --- a/gas/config/obj-elf.c
> +++ b/gas/config/obj-elf.c
> @@ -809,7 +809,7 @@ change_section (const char *name,
>  	= match_p->linked_to_symbol_name;
>  
>        bfd_set_section_flags (sec, flags);
> -      if (flags & SEC_MERGE)
> +      if (flags & (SEC_MERGE | SEC_STRINGS))
>  	sec->entsize = entsize;
>        elf_group_name (sec) = match_p->group_name;
>  
> @@ -864,7 +864,8 @@ change_section (const char *name,
>  	       processor or application specific attribute as suspicious?  */
>  	    elf_section_flags (sec) = attr;
>  
> -	  if ((flags & SEC_MERGE) && old_sec->entsize != (unsigned) entsize)
> +	  if ((flags & (SEC_MERGE | SEC_STRINGS))
> +	      && old_sec->entsize != (unsigned) entsize)
>  	    as_bad (_("changed section entity size for %s"), name);
>  	}
>      }
> @@ -1318,36 +1319,39 @@ obj_elf_section (int push)
>  	    }
>  
>  	  SKIP_WHITESPACE ();
> -	  if ((attr & SHF_MERGE) != 0 && *input_line_pointer == ',')
> +	  if ((attr & (SHF_MERGE | SHF_STRINGS)) != 0
> +	      && *input_line_pointer == ',')
>  	    {
>  	      ++input_line_pointer;
>  	      SKIP_WHITESPACE ();
>  	      if (inherit && *input_line_pointer == ','
> -		  && (bfd_section_flags (now_seg) & SEC_MERGE) != 0)
> +		  && (bfd_section_flags (now_seg)
> +		      & (SEC_MERGE | SEC_STRINGS)) != 0)
>  		goto fetch_entsize;
>  	      entsize = get_absolute_expression ();
>  	      SKIP_WHITESPACE ();
>  	      if (entsize <= 0)
>  		{
> -		  as_warn (_("invalid merge entity size"));
> -		  attr &= ~SHF_MERGE;
> +		  as_warn (_("invalid merge / string entity size"));
> +		  attr &= ~(SHF_MERGE | SHF_STRINGS);
>  		  entsize = 0;
>  		}
>  	    }
> -	  else if ((attr & SHF_MERGE) != 0 && inherit
> -		    && (bfd_section_flags (now_seg) & SEC_MERGE) != 0)
> +	  else if ((attr & (SHF_MERGE | SHF_STRINGS)) != 0 && inherit
> +		    && (bfd_section_flags (now_seg)
> +			& (SEC_MERGE | SEC_STRINGS)) != 0)
>  	    {
>  	    fetch_entsize:
>  	      entsize = now_seg->entsize;
>  	    }
> -	  else if ((attr & SHF_MERGE) != 0)
> +	  else if ((attr & (SHF_MERGE | SHF_STRINGS)) != 0)
>  	    {
> -	      as_warn (_("entity size for SHF_MERGE not specified"));
> -	      attr &= ~SHF_MERGE;
> +	      as_warn (_("entity size for SHF_MERGE / SHF_STRINGS not specified"));
> +	      attr &= ~(SHF_MERGE | SHF_STRINGS);
>  	    }
>  
> -	  if ((attr & SHF_MERGE) != 0 && type == SHT_NOBITS)
> -	    as_warn (_("bogus SHF_MERGE for SHT_NOBITS section"));
> +	  if ((attr & (SHF_MERGE | SHF_STRINGS)) != 0 && type == SHT_NOBITS)
> +	    as_warn (_("bogus SHF_MERGE / SHF_STRINGS for SHT_NOBITS section"));
>  
>  	  if ((attr & SHF_LINK_ORDER) != 0 && *input_line_pointer == ',')
>  	    {
> --- a/gas/write.c
> +++ b/gas/write.c
> @@ -1864,7 +1864,7 @@ subsegs_finish_section (asection *s)
>  	do_not_pad_sections_to_alignment = 1;
>  
>        alignment = SUB_SEGMENT_ALIGN (now_seg, frchainP);
> -      if ((bfd_section_flags (now_seg) & SEC_MERGE)
> +      if ((bfd_section_flags (now_seg) & (SEC_MERGE | SEC_STRINGS))
>  	  && now_seg->entsize)
>  	{
>  	  unsigned int entsize = now_seg->entsize;