Re: [PATCH] String offset errors on evbarm 8.99 [PR 54159]

[tlaronde%polynum.com@localhost]

On Mon, Aug 26, 2019 at 02:12:44PM +0100, Robert Swindells wrote:
> 
> tlaronde%polynum.com@localhost wrote:
> >On Thu, Aug 15, 2019 at 10:45:10AM +0200, Martin Husemann wrote:
> >> On Thu, Aug 15, 2019 at 10:27:51AM +0200, tlaronde%polynum.com@localhost wrote:
> >> > Solution adopted:
> >> > 	The simplest: since the caller of swap_symbol_in() has the
> >> > 	information, I added a supplementary parameter to
> >> > 	swap_symbol_in() to pass the correct symtab_hdr pointer,
> >> > 	modified the calls and added ATTRIBUTE_UNUSED in the
> >> > 	implementations that don't use it.
> >> 
> >> Great job!
> >> 
> >> This should be discussed upstream on the binutils mailing list.
> >> There is nothing NetBSD specific in here, isn't it?
> >> 
> >
> >FWIW, I have submitted the patches, updated against binutils current
> >release, to upstream with the mention that this was done when tracking
> >a NetBSD PR.
> 
> I built a full distribution for earmv7hf with your patches and built a
> fair number of packages with it and everything seems fine.
> 
> Maybe we should just commit your patches to -current then pull them up
> to -9 after testing a bit more.
> 
> I'm guessing that we wouldn't pull up a new binutils release to -9.

I strongly advise not to _upgrade_ binutils because seeing patches made
and reverted a couple of weeks later, I think the new release will need
a whole cycle of debugging. But if the 2.32 branch can be updated from
upstream and if this branch has only bug fixes, it should be simpler to
update from source. (But they use Git and I don't and from a very
superficial use, git was saying that "master" was the same as the 2.32
branch for some commands, and saying they were different with other
commands. So I don't understand how they manage the branch and I'm not
sure the 2.32 has _only_ sure bug fixes.)

My patches are sure in the sense they correct the problem without
compromising everything else, but they were not (and I didn't judge them
to be) a long term solution. Patching specially will need a manual
handling from upstream due to conflicts.

FWIW, here are patches applied upstream to correct the bug at a better
level.
-- 
        Thierry Laronde <tlaronde +AT+ polynum +dot+ com>
                     http://www.kergis.com/
                       http://www.sbfa.fr/
Key fingerprint = 0FF7 E906 FBAF FE95 FD89  250D 52B1 AE95 6006 F40C

I'm going to apply the following.
----
ARM CMSE symbols

This patch removes use of st_target_internal to cache the result of
comparing symbol names against CMSE_PREFIX.  The problem with setting
a bit in st_target_internal in swap_symbol_in is that calling
bfd_elf_sym_name from swap_symbol_in requires symtab_hdr, and you
don't know for sure whether swap_symbol_in is operating on dynsyms
(and thus elf_tdata (abfd)->dynsymtab_hdr should be used) or on the
normal symtab (thus elf_tdata (abfd)->symtab_hdr).  You can make an
educated guess based on abfd->flags & DYNAMIC but that relies on
knowing a lot about calls to bfd_elf_get_elf_syms, and is fragile in
the face of possible future changes.

include/
	* elf/arm.h (ARM_GET_SYM_CMSE_SPCL, ARM_SET_SYM_CMSE_SPCL): Delete.
bfd/
	* elf32-arm.c (cmse_scan): Don't use ARM_GET_SYM_CMSE_SPCL,
	instead recognize CMSE_PREFIX in symbol name.
	(elf32_arm_gc_mark_extra_sections): Likewise.
	(elf32_arm_filter_cmse_symbols): Don't test ARM_GET_SYM_CMSE_SPCL.
	(elf32_arm_swap_symbol_in): Don't invoke ARM_SET_SYM_CMSE_SPCL.

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 97d3726516..a725504c02 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,11 @@
+2019-08-22  Alan Modra  <amodra%gmail.com@localhost>
+
+	* elf32-arm.c (cmse_scan): Don't use ARM_GET_SYM_CMSE_SPCL,
+	instead recognize CMSE_PREFIX in symbol name.
+	(elf32_arm_gc_mark_extra_sections): Likewise.
+	(elf32_arm_filter_cmse_symbols): Don't test ARM_GET_SYM_CMSE_SPCL.
+	(elf32_arm_swap_symbol_in): Don't invoke ARM_SET_SYM_CMSE_SPCL.
+
 2019-08-20  Dennis Zhang  <dennis.zhang%arm.com@localhost>
 
 	* cpu-aarch64.c: New entries for Cortex-A34, Cortex-A65,
diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
index d1548d6db3..b675fc60c1 100644
--- a/bfd/elf32-arm.c
+++ b/bfd/elf32-arm.c
@@ -6002,12 +6002,12 @@ cmse_scan (bfd *input_bfd, struct elf32_arm_link_hash_table *htab,
       if (i < ext_start)
 	{
 	  cmse_sym = &local_syms[i];
-	  /* Not a special symbol.  */
-	  if (!ARM_GET_SYM_CMSE_SPCL (cmse_sym->st_target_internal))
-	    continue;
 	  sym_name = bfd_elf_string_from_elf_section (input_bfd,
 						      symtab_hdr->sh_link,
 						      cmse_sym->st_name);
+	  if (!sym_name || !CONST_STRNEQ (sym_name, CMSE_PREFIX))
+	    continue;
+
 	  /* Special symbol with local binding.  */
 	  cmse_invalid = TRUE;
 	}
@@ -6015,9 +6015,7 @@ cmse_scan (bfd *input_bfd, struct elf32_arm_link_hash_table *htab,
 	{
 	  cmse_hash = elf32_arm_hash_entry (sym_hashes[i - ext_start]);
 	  sym_name = (char *) cmse_hash->root.root.root.string;
-
-	  /* Not a special symbol.  */
-	  if (!ARM_GET_SYM_CMSE_SPCL (cmse_hash->root.target_internal))
+	  if (!CONST_STRNEQ (sym_name, CMSE_PREFIX))
 	    continue;
 
 	  /* Special symbol has incorrect binding or type.  */
@@ -15990,7 +15988,8 @@ elf32_arm_gc_mark_extra_sections (struct bfd_link_info *info,
 
 		  /* Assume it is a special symbol.  If not, cmse_scan will
 		     warn about it and user can do something about it.  */
-		  if (ARM_GET_SYM_CMSE_SPCL (cmse_hash->root.target_internal))
+		  if (CONST_STRNEQ (cmse_hash->root.root.root.string,
+				    CMSE_PREFIX))
 		    {
 		      cmse_sec = cmse_hash->root.root.u.def.section;
 		      if (!cmse_sec->gc_mark
@@ -18610,9 +18609,6 @@ elf32_arm_filter_cmse_symbols (bfd *abfd ATTRIBUTE_UNUSED,
 	  || cmse_hash->root.type != STT_FUNC)
 	continue;
 
-      if (!ARM_GET_SYM_CMSE_SPCL (cmse_hash->root.target_internal))
-	continue;
-
       syms[dst_count++] = sym;
     }
   free (cmse_name);
@@ -19935,9 +19931,6 @@ elf32_arm_swap_symbol_in (bfd * abfd,
 			  const void *pshn,
 			  Elf_Internal_Sym *dst)
 {
-  Elf_Internal_Shdr *symtab_hdr;
-  const char *name = NULL;
-
   if (!bfd_elf32_swap_symbol_in (abfd, psrc, pshn, dst))
     return FALSE;
   dst->st_target_internal = 0;
@@ -19966,13 +19959,6 @@ elf32_arm_swap_symbol_in (bfd * abfd,
   else
     ARM_SET_SYM_BRANCH_TYPE (dst->st_target_internal, ST_BRANCH_UNKNOWN);
 
-  /* Mark CMSE special symbols.  */
-  symtab_hdr = & elf_symtab_hdr (abfd);
-  if (symtab_hdr->sh_size)
-    name = bfd_elf_sym_name (abfd, symtab_hdr, dst, NULL);
-  if (name && CONST_STRNEQ (name, CMSE_PREFIX))
-    ARM_SET_SYM_CMSE_SPCL (dst->st_target_internal);
-
   return TRUE;
 }
 
diff --git a/include/ChangeLog b/include/ChangeLog
index 1813cb38d8..e779c17702 100644
--- a/include/ChangeLog
+++ b/include/ChangeLog
@@ -1,3 +1,7 @@
+2019-08-22  Alan Modra  <amodra%gmail.com@localhost>
+
+	* elf/arm.h (ARM_GET_SYM_CMSE_SPCL, ARM_SET_SYM_CMSE_SPCL): Delete.
+
 2019-08-09  Mihailo Stojanovic  <mihailo.stojanovic%rt-rk.com@localhost>
 
 	* elf/mips.h (SHT_GNU_XHASH): New define.
diff --git a/include/elf/arm.h b/include/elf/arm.h
index 5cb9970644..75fb5e26ca 100644
--- a/include/elf/arm.h
+++ b/include/elf/arm.h
@@ -399,11 +399,4 @@ enum arm_st_branch_type {
 	   | ((TYPE) & ENUM_ARM_ST_BRANCH_TYPE_BITMASK))
 #endif
 
-/* Get or set whether a symbol is a special symbol of an entry function of CMSE
-   secure code.  */
-#define ARM_GET_SYM_CMSE_SPCL(SYM_TARGET_INTERNAL) \
-  (((SYM_TARGET_INTERNAL) >> 2) & 1)
-#define ARM_SET_SYM_CMSE_SPCL(SYM_TARGET_INTERNAL) \
-  (SYM_TARGET_INTERNAL) |= 4
-
 #endif /* _ELF_ARM_H */

-- 
Alan Modra
Australia Development Lab, IBM