[PATCH] Fix msp430 section name scribbling and tests

[PATCH] Fix msp430 section name scribbling and tests

[Michael Matz]

msp430 upper/lower section selector simply overwrites section names
without going through bfd_rename_section.  This doesn't really work:
as soon as the generic routine to map to output section names is used
(e.g. by having a complicated selector, or simply by adding an early-out
into analyze_walk_wild_section_handler for always use the generic
matcher) it iterates over the new names and so sees e.g. '.either..data'
and makes that orphan (because of no output statement matching that),
and then errors out because there may be no output statements for
.upper.data or .lower.data.  In the testcases that works because that
.data is actually empty, and when using the non-generic routines for
section matching it can use the per-bfd section-name-hash.  Because that
one still is unchanged it will find the section formerly named .data for
which an output statement exists, put it into that section, and makes it
not ortphan.  That works because the actual section is empty and hence
this placement is harmless.

But it's really not how this all is expected to work, one can't just
change section->name alone.  So to make it reliably work we either
would have to force users to write at least .lower.XYZ statements for
the three usual sections (.text, .data, .bss), even if the input is empty,
or to check if renaming to .either.XYZ is going to do any good.

Here we do the latter, we check before renaming and then actually do
rename properly.

FWIW, to see the testsuite breaking without this patch one could apply
this patch that forces the generic matching routine to always be used:

--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -839,6 +839,7 @@ analyze_walk_wild_section_handler (lang_wild_statement_type *ptr)
   ptr->handler_data[2] = NULL;
   ptr->handler_data[3] = NULL;
   ptr->tree = NULL;
+  return;

   /* Count how many wildcard_specs there are, and how many of those
      actually use wildcards in the name.  Also, bail out if any of the
---
Regtested for msp430-elf.  This problem prevented some other work in 
changing how sections are selected, so I want to get rid of this first.

Okay for master?

 ld/emultempl/msp430.em | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/ld/emultempl/msp430.em b/ld/emultempl/msp430.em
index f188b46b4aa..c8a39f265f2 100644
--- a/ld/emultempl/msp430.em
+++ b/ld/emultempl/msp430.em
@@ -388,6 +388,38 @@ change_output_section (lang_statement_union_type **head,
   return false;
 }
 
+static bool
+has_upper_or_lower (const char *secname)
+{
+  char * lower_name;
+  char * upper_name;
+  char * name;
+  bool ret = false;
+  /* Compute the names of the corresponding upper and lower
+     sections.  If the input section name contains another period,
+     only use the part of the name before the second dot.  */
+  if (strchr (secname + 1, '.') != NULL)
+    {
+      name = xstrdup (secname);
+
+      * strchr (name + 1, '.') = 0;
+    }
+  else
+    name = (char *) secname;
+
+  lower_name = concat (".lower", name, NULL);
+  upper_name = concat (".upper", name, NULL);
+  if (lang_output_section_find (lower_name)
+      || lang_output_section_find (upper_name))
+    ret = true;
+
+  free (upper_name);
+  free (lower_name);
+  if (name != (char*) secname)
+    free (name);
+  return ret;
+}
+
 static void
 add_region_prefix (bfd *abfd ATTRIBUTE_UNUSED, asection *s,
 		   void *unused ATTRIBUTE_UNUSED)
@@ -417,7 +449,8 @@ add_region_prefix (bfd *abfd ATTRIBUTE_UNUSED, asection *s,
       bfd_rename_section (s, concat (".lower", curr_name, NULL));
       break;
     case REGION_EITHER:
-      s->name = concat (".either", curr_name, NULL);
+      if (has_upper_or_lower (curr_name))
+	bfd_rename_section (s, concat (".either", curr_name, NULL));
       break;
     default:
       /* Unreachable.  */
-- 
2.36.1

Re: [PATCH] Fix msp430 section name scribbling and tests

[Alan Modra]

On Fri, Nov 25, 2022 at 04:00:05PM +0000, Michael Matz via Binutils wrote:
> msp430 upper/lower section selector simply overwrites section names
> without going through bfd_rename_section.  This doesn't really work:

Right.

> as soon as the generic routine to map to output section names is used
> (e.g. by having a complicated selector, or simply by adding an early-out
> into analyze_walk_wild_section_handler for always use the generic
> matcher) it iterates over the new names and so sees e.g. '.either..data'
> and makes that orphan (because of no output statement matching that),
> and then errors out because there may be no output statements for
> .upper.data or .lower.data.  In the testcases that works because that
> .data is actually empty, and when using the non-generic routines for
> section matching it can use the per-bfd section-name-hash.  Because that
> one still is unchanged it will find the section formerly named .data for
> which an output statement exists, put it into that section, and makes it
> not ortphan.  That works because the actual section is empty and hence
> this placement is harmless.
> 
> But it's really not how this all is expected to work, one can't just
> change section->name alone.  So to make it reliably work we either
> would have to force users to write at least .lower.XYZ statements for
> the three usual sections (.text, .data, .bss), even if the input is empty,
> or to check if renaming to .either.XYZ is going to do any good.
> 
> Here we do the latter, we check before renaming and then actually do
> rename properly.

I'm inclined to think that you should just apply the single line
bfd_rename_section change, and adjust the testsuite script file.
The standard scripts do contain the lower/upper/either variants for
.text, .data and .bss.  

Something like this.

	* emultempl/msp430.em (add_region_prefix <REGION_EITHER>): Use
	bfd_rename_section.
	* testsuite/ld-msp430-elf/msp430-tiny-rom.ld: Handle varian data
	and bss input sections.

diff --git a/ld/emultempl/msp430.em b/ld/emultempl/msp430.em
index f188b46b4aa..048e3ebfe5a 100644
--- a/ld/emultempl/msp430.em
+++ b/ld/emultempl/msp430.em
@@ -417,7 +417,7 @@ add_region_prefix (bfd *abfd ATTRIBUTE_UNUSED, asection *s,
       bfd_rename_section (s, concat (".lower", curr_name, NULL));
       break;
     case REGION_EITHER:
-      s->name = concat (".either", curr_name, NULL);
+      bfd_rename_section (s, concat (".either", curr_name, NULL));
       break;
     default:
       /* Unreachable.  */
diff --git a/ld/testsuite/ld-msp430-elf/msp430-tiny-rom.ld b/ld/testsuite/ld-msp430-elf/msp430-tiny-rom.ld
index 3e263796948..c88442377d1 100644
--- a/ld/testsuite/ld-msp430-elf/msp430-tiny-rom.ld
+++ b/ld/testsuite/ld-msp430-elf/msp430-tiny-rom.ld
@@ -26,13 +26,19 @@ SECTIONS
   .data :
   {
     . = ALIGN(2);
+    *(.lower.data.* .lower.data)
     *(.data.* .data)
+    *(.either.data.* .either.data)
+    *(.upper.data.* .upper.data)
   } > RAM AT> ROM
 
   .bss :
   {
     . = ALIGN(2);
+    *(.lower.bss.* .lower.bss)
     *(.bss.* .bss)
+    *(.either.bss.* .either.bss)
+    *(.upper.bss.* .upper.bss)
   } > RAM
 
   .upper.text :


-- 
Alan Modra
Australia Development Lab, IBM