Re: eu-strip --reloc-debug-sections

[Mark Wielaard <mjw at redhat dot com>]

On Tue, 2011-05-24 at 12:31 +0200, Mark Wielaard wrote:
> On Mon, 2011-05-23 at 13:43 -0700, Roland McGrath wrote:
> > You don't want to constrain it to STT_SECTION symbols.  Any symbol whose
> > shndx is in a non-allocated section ought to be fine.  Whether you see only
> > section-relative references or see symbols too depends on the tools and
> > machine (IIRC s390 gets a lot of symbol-relative relocations).
> 
> Too bad, it kept the code simple since I could just ignore the actual
> symbol value was always zero, which made the ET_REL case almost
> completely trivial. And in all the cases I saw the cross-debug-section
> relocations use the STT_SECTION symbol of another debug section plus an
> addend (either explicit in the RELA case, or already at the offset
> location in the REL case).

I added the s390 testcase and the following patch to the
mjw/reloc-debug-sections branch:

2011-05-23  Mark Wielaard  <mjw@redhat.com>
 
    * strip.c (relocate): Take new arguments is_rela to indicate
    whether the relocation is from a SHT_REL or SHT_RELA section. 
    Relocate against any debug section symbol, not just STT_SECTION
    symbols. For SHT_REL relocations, fetch addend from offset and
    add it to symbol value if not zero.

With this the s390 testcase sees a reduction in size of 30%. One other
testcase is also slightly smaller.

Thanks,

Mark

commit 63868c2afb1123bf8ac2f99048e6f3f70dcf4c0e
Author: Mark Wielaard <mjw@redhat.com>
Date:   Tue May 24 16:09:31 2011 +0200

    strip: --reloc-debug-sections, relocate against any debug section symbol.

diff --git a/src/strip.c b/src/strip.c
index f680061..cf6bd97 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -1682,7 +1682,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 	      /* Apply one relocation.  Returns true when trivial
 		 relocation actually done.  */
 	      bool relocate (GElf_Addr offset, const GElf_Sxword addend,
-			     int rtype, int symndx)
+			     bool is_rela, int rtype, int symndx)
 	      {
 		/* R_*_NONE relocs can always just be removed.  */
 		if (rtype == 0)
@@ -1704,80 +1704,98 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 		GElf_Sym *sym = gelf_getsymshndx (symdata, xndxdata,
 						  symndx, &sym_mem,
 						  &xndx);
-		if (GELF_ST_TYPE (sym->st_info) == STT_SECTION)
+		Elf32_Word sec = (sym->st_shndx == SHN_XINDEX
+				  ? xndx : sym->st_shndx);
+		if (ebl_debugscn_p (ebl, shdr_info[sec].name))
 		  {
-		    Elf32_Word sec = (sym->st_shndx == SHN_XINDEX
-				      ? xndx : sym->st_shndx);
-		    if (ebl_debugscn_p (ebl, shdr_info[sec].name))
-		      {
-			size_t size;
-			switch (type)
-			  {
-#define DO_TYPE(NAME, Name)					\
-			    case ELF_T_##NAME:			\
-			      size = sizeof (GElf_##Name);	\
-			      break;
-			    TYPES;
-#undef DO_TYPE
-			  default:
-			    return false;
-			  }
-
-			if (offset + size > tdata->d_size)
-			  error (0, 0, gettext ("bad relocation"));
-
-			/* For SHT_REL sections this is all that needs
-			   to be checked.  The addend is contained in
-			   the original data at the offset already.
-			   And the (section) symbol address is zero.
-			   So just remove the relocation, it isn't
-			   needed anymore.  */
-			if (addend == 0)
-			  return true;
+		    size_t size;
 
 #define DO_TYPE(NAME, Name) GElf_##Name Name;
-			union { TYPES; } tmpbuf;
+		    union { TYPES; } tmpbuf;
 #undef DO_TYPE
-			Elf_Data tmpdata =
-			  {
-			    .d_type = type,
-			    .d_buf = &tmpbuf,
-			    .d_size = size,
-			    .d_version = EV_CURRENT,
-			  };
-			Elf_Data rdata =
-			  {
-			    .d_type = type,
-			    .d_buf = tdata->d_buf + offset,
-			    .d_size = size,
-			    .d_version = EV_CURRENT,
-			  };
 
-			/* For SHT_RELA sections we just take the
-			   addend and put it into the relocation slot.
-			   The (section) symbol address can be
-			   ignored, since it is zero.  */
-			switch (type)
-			  {
+		    switch (type)
+		      {
 #define DO_TYPE(NAME, Name)				\
-			    case ELF_T_##NAME:		\
-			      tmpbuf.Name = addend;	\
-			      break;
-			    TYPES;
+			case ELF_T_##NAME:		\
+			  size = sizeof (GElf_##Name);	\
+			  tmpbuf.Name = 0;		\
+			  break;
+			TYPES;
 #undef DO_TYPE
-			  default:
-			    abort ();
-			  }
+		      default:
+			return false;
+		      }
+
+		    if (offset + size > tdata->d_size)
+		      error (0, 0, gettext ("bad relocation"));
 
-			Elf_Data *s = gelf_xlatetof (debugelf, &rdata,
-						     &tmpdata,
+		    /* When the symbol value is zero then for SHT_REL
+		       sections this is all that needs to be checked.
+		       The addend is contained in the original data at
+		       the offset already.  So if the (section) symbol
+		       address is zero and the given addend is zero
+		       just remove the relocation, it isn't needed
+		       anymore.  */
+		    if (addend == 0 && sym->st_value == 0)
+		      return true;
+
+		    Elf_Data tmpdata =
+		      {
+			.d_type = type,
+			.d_buf = &tmpbuf,
+			.d_size = size,
+			.d_version = EV_CURRENT,
+		      };
+		    Elf_Data rdata =
+		      {
+			.d_type = type,
+			.d_buf = tdata->d_buf + offset,
+			.d_size = size,
+			.d_version = EV_CURRENT,
+		      };
+
+		    GElf_Addr value = sym->st_value;
+		    if (is_rela)
+		      {
+			/* For SHT_RELA sections we just take the
+			   given addend and add it to the value.  */
+			value += addend;
+		      }
+		    else
+		      {
+			/* For SHT_REL sections we have to peek at
+			   what is already in the section at the given
+			   offset to get the addend.  */
+			Elf_Data *d = gelf_xlatetom (debugelf, &tmpdata,
+						     &rdata,
 						     ehdr->e_ident[EI_DATA]);
-			if (s == NULL)
+			if (d == NULL)
 			  INTERNAL_ERROR (fname);
-			assert (s == &rdata);
+			assert (d == &tmpdata);
+		      }
 
-			return true;
+		    switch (type)
+		      {
+#define DO_TYPE(NAME, Name)					\
+			case ELF_T_##NAME:			\
+			  tmpbuf.Name += (GElf_##Name) value;	\
+			  break;
+			TYPES;
+#undef DO_TYPE
+		      default:
+			abort ();
 		      }
+
+		    /* Now finally put in the new value.  */
+		    Elf_Data *s = gelf_xlatetof (debugelf, &rdata,
+						 &tmpdata,
+						 ehdr->e_ident[EI_DATA]);
+		    if (s == NULL)
+		      INTERNAL_ERROR (fname);
+		    assert (s == &rdata);
+
+		    return true;
 		  }
 		return false;
 	      }
@@ -1789,7 +1807,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 		  {
 		    GElf_Rel rel_mem;
 		    GElf_Rel *r = gelf_getrel (reldata, relidx, &rel_mem);
-		    if (! relocate (r->r_offset, 0,
+		    if (! relocate (r->r_offset, 0, false,
 				    GELF_R_TYPE (r->r_info),
 				    GELF_R_SYM (r->r_info)))
 		      {
@@ -1803,7 +1821,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 		  {
 		    GElf_Rela rela_mem;
 		    GElf_Rela *r = gelf_getrela (reldata, relidx, &rela_mem);
-		    if (! relocate (r->r_offset, r->r_addend,
+		    if (! relocate (r->r_offset, r->r_addend, true,
 				    GELF_R_TYPE (r->r_info),
 				    GELF_R_SYM (r->r_info)))
 		      {