Re: MIPS and -msym32

Re: MIPS and -msym32

[Crestez Dan Leonard]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

On 08/01/2014 01:59 PM, Mark Wielaard wrote:
> Hi Leonard,
> 
> On Tue, Jul 22, 2014 at 05:44:48PM +0300, Crestez Dan Leonard wrote:
>> I used the elfutils mips patch from debian:
>> 	http://sources.debian.net/src/elfutils/0.159-4/debian/patches/mips_backend.diff
> 
> It would be convenient if the MIPS port was integrated upstream.
> Do you happen to know whether the porters might want to contribute it?
> The contribution policy for elfutils is documented at:
> https://git.fedorahosted.org/cgit/elfutils.git/tree/CONTRIBUTING

The patch I found does not list any author. I just "found it". Maybe you should email
the debian maintainer to ask where it's from?

Regards,
Leonard

Re: MIPS and -msym32

[Kurt Roeckx]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

On Fri, Aug 15, 2014 at 05:24:56PM +0200, Kurt Roeckx wrote:
> 
> There is an other mips related patch in Debian.  I have code for
> that, it was mailed to this list, I needed to make some changes,
> I think I've done that but I didn't really finish them so it was
> never send.  I can send what I currently have if that's useful.

So the patch currently in Debian is:
Index: elfutils-0.153/src/readelf.c
===================================================================
--- elfutils-0.153.orig/src/readelf.c   2012-08-10 22:01:55.000000000 +0200
+++ elfutils-0.153/src/readelf.c        2012-09-18 21:46:27.000000000 +0200
@@ -7364,7 +7364,8 @@
       GElf_Shdr shdr_mem;
       GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);

-      if (shdr != NULL && shdr->sh_type == SHT_PROGBITS)
+      if (shdr != NULL && (
+        (shdr->sh_type == SHT_PROGBITS) || (shdr->sh_type == SHT_MIPS_DWARF)))
        {
          static const struct
          {

I think the patch in attachment is supposed to replace that, together with
the proper implementation of the hook in the mips backend.


Kurt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: check_debug_section.patch --]
[-- Type: text/x-diff, Size: 8512 bytes --]

diff --git a/backends/i386_symbol.c b/backends/i386_symbol.c
index 7dbf899..c09119d 100644
--- a/backends/i386_symbol.c
+++ b/backends/i386_symbol.c
@@ -65,11 +65,16 @@ i386_reloc_simple_type (Ebl *ebl __attribute__ ((unused)), int type)
 }
 
 /* Check section name for being that of a debug information section.  */
-bool (*generic_debugscn_p) (const char *);
+bool (*generic_debugscn_p) (const GElf_Shdr *shdr, const char *);
 bool
-i386_debugscn_p (const char *name)
+i386_debugscn_p (const GElf_Shdr *shdr, const char *name)
 {
-  return (generic_debugscn_p (name)
+  if (shdr && shdr->sh_type != SHT_PROGBITS)
+    return false;
+  if (!name)
+    return false;
+
+  return (generic_debugscn_p (shdr, name)
 	  || strcmp (name, ".stab") == 0
 	  || strcmp (name, ".stabstr") == 0);
 }
diff --git a/backends/libebl_CPU.h b/backends/libebl_CPU.h
index 36b3a4a..bdb4af1 100644
--- a/backends/libebl_CPU.h
+++ b/backends/libebl_CPU.h
@@ -43,7 +43,7 @@ extern const char *EBLHOOK(init) (Elf *elf, GElf_Half machine,
 
 #define HOOK(eh, name)	eh->name = EBLHOOK(name)
 
-extern bool (*generic_debugscn_p) (const char *) attribute_hidden;
+extern bool (*generic_debugscn_p) (const GElf_Shdr *shdr, const char *) attribute_hidden;
 
 
 #endif	/* libebl_CPU.h */
diff --git a/libdwfl/relocate.c b/libdwfl/relocate.c
index 2c24bd5..a4b4594 100644
--- a/libdwfl/relocate.c
+++ b/libdwfl/relocate.c
@@ -288,7 +288,7 @@ relocate_section (Dwfl_Module *mod, Elf *relocated, const GElf_Ehdr *ehdr,
     /* No contents to relocate.  */
     return DWFL_E_NOERROR;
 
-  if (debugscn && ! ebl_debugscn_p (mod->ebl, tname))
+  if (debugscn && ! ebl_debugscn_p (mod->ebl, tshdr, tname))
     /* This relocation section is not for a debugging section.
        Nothing to do here.  */
     return DWFL_E_NOERROR;
diff --git a/libebl/ebl-hooks.h b/libebl/ebl-hooks.h
index d3cf3e6..bb7fb22 100644
--- a/libebl/ebl-hooks.h
+++ b/libebl/ebl-hooks.h
@@ -108,8 +108,8 @@ bool EBLHOOK(check_object_attribute) (Ebl *, const char *, int, uint64_t,
 /* Describe auxv element type.  */
 int EBLHOOK(auxv_info) (GElf_Xword, const char **, const char **);
 
-/* Check section name for being that of a debug informatino section.  */
-bool EBLHOOK(debugscn_p) (const char *);
+/* Check section for being that of a debug information section.  */
+bool EBLHOOK(debugscn_p) (const GElf_Shdr *, const char *);
 
 /* Check whether given relocation is a copy relocation.  */
 bool EBLHOOK(copy_reloc_p) (int);
diff --git a/libebl/ebldebugscnp.c b/libebl/ebldebugscnp.c
index f2351e2..6817f26 100644
--- a/libebl/ebldebugscnp.c
+++ b/libebl/ebldebugscnp.c
@@ -1,4 +1,4 @@
-/* Check section name for being that of a debug informatino section.
+/* Check section for being that of a debug information section.
    Copyright (C) 2002 Red Hat, Inc.
    This file is part of elfutils.
    Written by Ulrich Drepper <drepper@redhat.com>, 2002.
@@ -36,9 +36,7 @@
 
 
 bool
-ebl_debugscn_p (ebl, name)
-     Ebl *ebl;
-     const char *name;
+ebl_debugscn_p (Ebl *ebl, const GElf_Shdr *shdr, const char *name)
 {
-  return ebl->debugscn_p (name);
+  return ebl->debugscn_p (shdr, name);
 }
diff --git a/libebl/eblopenbackend.c b/libebl/eblopenbackend.c
index ed0c0ff..b48ab0c 100644
--- a/libebl/eblopenbackend.c
+++ b/libebl/eblopenbackend.c
@@ -174,7 +174,7 @@ static int default_auxv_info (GElf_Xword a_type,
 			      const char **name, const char **format);
 static bool default_object_note (const char *name, uint32_t type,
 				 uint32_t descsz, const char *desc);
-static bool default_debugscn_p (const char *name);
+static bool default_debugscn_p (const GElf_Shdr *shdr, const char *name);
 static bool default_copy_reloc_p (int reloc);
 static bool default_none_reloc_p (int reloc);
 static bool default_relative_reloc_p (int reloc);
@@ -617,7 +617,7 @@ default_object_note (const char *name __attribute__ ((unused)),
 }
 
 static bool
-default_debugscn_p (const char *name)
+default_debugscn_p (const GElf_Shdr *shdr, const char *name)
 {
   /* We know by default only about the DWARF debug sections which have
      fixed names.  */
@@ -657,6 +657,12 @@ default_debugscn_p (const char *name)
     };
   const size_t ndwarf_scn_names = (sizeof (dwarf_scn_names)
 				   / sizeof (dwarf_scn_names[0]));
+
+  if (shdr && shdr->sh_type != SHT_PROGBITS)
+    return false;
+  if (!name)
+    return false;
+
   for (size_t cnt = 0; cnt < ndwarf_scn_names; ++cnt)
     if (strcmp (name, dwarf_scn_names[cnt]) == 0)
       return true;
diff --git a/libebl/eblsectionstripp.c b/libebl/eblsectionstripp.c
index 9497068..ece43dc 100644
--- a/libebl/eblsectionstripp.c
+++ b/libebl/eblsectionstripp.c
@@ -43,7 +43,7 @@ ebl_section_strip_p (Ebl *ebl, const GElf_Ehdr *ehdr, const GElf_Shdr *shdr,
      is unfortunately no other way.  */
   if (unlikely (only_remove_debug))
     {
-      if (ebl_debugscn_p (ebl, name))
+      if (ebl_debugscn_p (ebl, shdr, name))
 	return true;
 
       if (shdr->sh_type == SHT_RELA || shdr->sh_type == SHT_REL)
@@ -55,7 +55,7 @@ ebl_section_strip_p (Ebl *ebl, const GElf_Ehdr *ehdr, const GElf_Shdr *shdr,
 	    {
 	      const char *s_l = elf_strptr (ebl->elf, ehdr->e_shstrndx,
 					    shdr_l->sh_name);
-	      if (s_l != NULL && ebl_debugscn_p (ebl, s_l))
+	      if (s_l != NULL && ebl_debugscn_p (ebl, shdr_l, s_l))
 		return true;
 	    }
 	}
diff --git a/libebl/libebl.h b/libebl/libebl.h
index cae31c9..d81b21e 100644
--- a/libebl/libebl.h
+++ b/libebl/libebl.h
@@ -180,8 +180,8 @@ extern bool ebl_check_object_attribute (Ebl *ebl, const char *vendor,
 					const char **value_name);
 
 
-/* Check section name for being that of a debug informatino section.  */
-extern bool ebl_debugscn_p (Ebl *ebl, const char *name);
+/* Check section for being that of a debug information section.  */
+extern bool ebl_debugscn_p (Ebl *ebl, const GElf_Shdr *shdr, const char *name);
 
 /* Check whether given relocation is a copy relocation.  */
 extern bool ebl_copy_reloc_p (Ebl *ebl, int reloc);
diff --git a/src/ldgeneric.c b/src/ldgeneric.c
index 1b5d0f9..b209622 100644
--- a/src/ldgeneric.c
+++ b/src/ldgeneric.c
@@ -879,12 +879,13 @@ mark_section_group (struct usedfiles *fileinfo, Elf32_Word shndx,
 
       if (ld_state.strip == strip_none
 	  /* If we are stripping, remove debug sections.  */
-	  || (!ebl_debugscn_p (ld_state.ebl,
+	  || (!ebl_debugscn_p (ld_state.ebl, shdr,
 			       elf_strptr (fileinfo->elf, fileinfo->shstrndx,
 					   shdr->sh_name))
 	      /* And the relocation sections for the debug sections.  */
 	      && ((shdr->sh_type != SHT_RELA && shdr->sh_type != SHT_REL)
 		  || !ebl_debugscn_p (ld_state.ebl,
+				      &SCNINFO_SHDR (fileinfo->scninfo[shdr->sh_info].shdr),
 				      elf_strptr (fileinfo->elf,
 						  fileinfo->shstrndx,
 						  SCNINFO_SHDR (fileinfo->scninfo[shdr->sh_info].shdr).sh_name)))))
diff --git a/src/ldscript.y b/src/ldscript.y
index ec58e21..6acbe53 100644
--- a/src/ldscript.y
+++ b/src/ldscript.y
@@ -212,7 +212,7 @@ outputsection:	  assignment ';'
 		      $$->val.section.name = $1;
 		      $$->val.section.input = $3->next;
 		      if (ld_state.strip == strip_debug
-			  && ebl_debugscn_p (ld_state.ebl, $1))
+			  && ebl_debugscn_p (ld_state.ebl, NULL, $1))
 			$$->val.section.ignored = true;
 		      else
 			$$->val.section.ignored = false;
@@ -235,7 +235,7 @@ outputsection:	  assignment ';'
 			new_input_section_name ($1, false);
 		      $$->val.section.input->val.section->keep_flag = false;
 		      if (ld_state.strip == strip_debug
-			  && ebl_debugscn_p (ld_state.ebl, $1))
+			  && ebl_debugscn_p (ld_state.ebl, NULL, $1))
 			$$->val.section.ignored = true;
 		      else
 			$$->val.section.ignored = false;
diff --git a/src/strip.c b/src/strip.c
index 5e9c883..925e505 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -1648,7 +1648,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 
 	      const char *tname =  elf_strptr (debugelf, shstrndx,
 					       tshdr->sh_name);
-	      if (! tname || ! ebl_debugscn_p (ebl, tname))
+	      if (! ebl_debugscn_p (ebl, tshdr, tname))
 		continue;
 
 	      /* OK, lets relocate all trivial cross debug section
@@ -1698,7 +1698,8 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
 						  &xndx);
 		Elf32_Word sec = (sym->st_shndx == SHN_XINDEX
 				  ? xndx : sym->st_shndx);
-		if (ebl_debugscn_p (ebl, shdr_info[sec].name))
+		if (ebl_debugscn_p (ebl, &shdr_info[sec].shdr,
+				    shdr_info[sec].name))
 		  {
 		    size_t size;
 

Re: MIPS and -msym32

[Kurt Roeckx]

[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]

On Fri, Aug 15, 2014 at 04:38:22PM +0200, Mark Wielaard wrote:
> Hi Kurt,
> 
> On Fri, 2014-08-01 at 15:21 +0300, Crestez Dan Leonard wrote:
> > On 08/01/2014 01:59 PM, Mark Wielaard wrote:
> > > On Tue, Jul 22, 2014 at 05:44:48PM +0300, Crestez Dan Leonard wrote:
> > >> I used the elfutils mips patch from debian:
> > >> 	http://sources.debian.net/src/elfutils/0.159-4/debian/patches/mips_backend.diff
> > > 
> > > It would be convenient if the MIPS port was integrated upstream.
> > > Do you happen to know whether the porters might want to contribute it?
> > > The contribution policy for elfutils is documented at:
> > > https://git.fedorahosted.org/cgit/elfutils.git/tree/CONTRIBUTING
> > 
> > The patch I found does not list any author. I just "found it". Maybe you should email
> > the debian maintainer to ask where it's from?
> 
> Could you help us out with this? Do you know who the authors are of the
> elfutils mips patch in debian and whether they would be willing to
> contribute it upstream as described in the CONTRIBUTING document?

It's been on my TODO list for a very long time to properly
document the author and get things merged.

The changelog indicates this was written by Christian Aichinger
<Greek0@gmx.net>

There is an other mips related patch in Debian.  I have code for
that, it was mailed to this list, I needed to make some changes,
I think I've done that but I didn't really finish them so it was
never send.  I can send what I currently have if that's useful.


Kurt

Re: MIPS and -msym32

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 987 bytes --]

Hi Kurt,

On Fri, 2014-08-01 at 15:21 +0300, Crestez Dan Leonard wrote:
> On 08/01/2014 01:59 PM, Mark Wielaard wrote:
> > On Tue, Jul 22, 2014 at 05:44:48PM +0300, Crestez Dan Leonard wrote:
> >> I used the elfutils mips patch from debian:
> >> 	http://sources.debian.net/src/elfutils/0.159-4/debian/patches/mips_backend.diff
> > 
> > It would be convenient if the MIPS port was integrated upstream.
> > Do you happen to know whether the porters might want to contribute it?
> > The contribution policy for elfutils is documented at:
> > https://git.fedorahosted.org/cgit/elfutils.git/tree/CONTRIBUTING
> 
> The patch I found does not list any author. I just "found it". Maybe you should email
> the debian maintainer to ask where it's from?

Could you help us out with this? Do you know who the authors are of the
elfutils mips patch in debian and whether they would be willing to
contribute it upstream as described in the CONTRIBUTING document?

Thanks,

Mark

Re: MIPS and -msym32

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1904 bytes --]

Hi Leonard,

On Tue, Jul 22, 2014 at 05:44:48PM +0300, Crestez Dan Leonard wrote:
> I used the elfutils mips patch from debian:
> 	http://sources.debian.net/src/elfutils/0.159-4/debian/patches/mips_backend.diff

It would be convenient if the MIPS port was integrated upstream.
Do you happen to know whether the porters might want to contribute it?
The contribution policy for elfutils is documented at:
https://git.fedorahosted.org/cgit/elfutils.git/tree/CONTRIBUTING

> The generated dwarf files confuse systemtap is multiple ways. 

Do you happen to have one such DWARF file around? I like to better
understand which address size is set where.

> Here is a hack I used to get around this:
> --- a/libdw/dwarf_diecu.c
> +++ b/libdw/dwarf_diecu.c
> @@ -47,7 +47,22 @@ dwarf_diecu (die, result, address_sizep, offset_sizep)
>    *result = CUDIE (die->cu);
>  
>    if (address_sizep != NULL)
> +  {
>      *address_sizep = die->cu->address_size;
> +    /* Hack: */
> +    if (1)
> +    {
> +      struct Elf *elf = die->cu->dbg->elf;
> +      GElf_Ehdr ehdr_mem;
> +      GElf_Ehdr* ehdr = gelf_getehdr (elf, &ehdr_mem);
> +      if (ehdr &&
> +              ehdr->e_machine == EM_MIPS &&
> +              ehdr->e_ident[EI_CLASS] == ELFCLASS64)
> +      {
> +        *address_sizep = 8;
> +      }
> +    }
> +  }
>    if (offset_sizep != NULL)
>      *offset_sizep = die->cu->offset_size;
> 
> This is obviously evil.

Yes, it is :)
Assuming that address size used in the CU is correct, it seems the
above check should be in systemtap instead.

> Apparently the gcc folks decided that this -msym32 behavior was too
> confusing and changed it to generate dwarf with a pointer size of 8:
> https://gcc.gnu.org/ml/gcc/2009-01/msg00611.html

Hmmm. That helps, but is technically wrong IMHO.
It would be better to fix binutils instead.

Thanks,

Mark

MIPS and -msym32

[Crestez Dan Leonard]

[-- Attachment #1: Type: text/plain, Size: 4199 bytes --]

Hello,

I've been playing around with systemtap on mips.

I used the elfutils mips patch from debian:
	http://sources.debian.net/src/elfutils/0.159-4/debian/patches/mips_backend.diff

For systemtap I ported some old patches from cisco on top of release 2.5. Here is the link to the patches on top of 1.6:
	https://sourceware.org/git/gitweb.cgi?p=systemtap.git;a=log;h=refs/heads/systemtap-1.6-cisco-patches

One of the issues I encountered is that by default 64-bit mips kernels are compiled with -msym32. This causes gcc to emit dwarf information with a pointer size of 4, even though the file is 64bit. In theory this can be disabled by passing KBUILD_SYM32=no to the kernel make commandline. This is a bad idea. It would disable some optimizations which have been enabled on mips for many years. I actually tried to do it locally but it broke module loading and I was unable to boot.

The generated dwarf files confuse systemtap is multiple ways. 

When fetching some parameters systemtap relies on address_size from dwarf_diecu to determine max_fetch_size. This caused an error like "semantic error: single register too big for fetch/store ???: identifier '$data' at ..." for an unsigned long variable.

Here is a hack I used to get around this:
--- a/libdw/dwarf_diecu.c
+++ b/libdw/dwarf_diecu.c
@@ -47,7 +47,22 @@ dwarf_diecu (die, result, address_sizep, offset_sizep)
   *result = CUDIE (die->cu);
 
   if (address_sizep != NULL)
+  {
     *address_sizep = die->cu->address_size;
+    /* Hack: */
+    if (1)
+    {
+      struct Elf *elf = die->cu->dbg->elf;
+      GElf_Ehdr ehdr_mem;
+      GElf_Ehdr* ehdr = gelf_getehdr (elf, &ehdr_mem);
+      if (ehdr &&
+              ehdr->e_machine == EM_MIPS &&
+              ehdr->e_ident[EI_CLASS] == ELFCLASS64)
+      {
+        *address_sizep = 8;
+      }
+    }
+  }
   if (offset_sizep != NULL)
     *offset_sizep = die->cu->offset_size;

This is obviously evil. What is worse is that -msym32 seems to break systemtap address mapping. Earlier I had to do the following hack inside systemtap.

diff --git a/tapsets.cxx b/tapsets.cxx
index 4e42403..94ee230 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -1265,6 +1265,22 @@ dwarf_query::add_probe_point(const string& dw_funcname,
       else
         {
           assert (has_kernel || has_module);
+          if (1)
+            {
+              Dwarf_Addr bias;
+              Elf* elf = (dwarf_getelf (dwfl_module_getdwarf (dw.module, &bias))
+                 ?: dwfl_module_getelf (dw.module, &bias));
+              GElf_Ehdr ehdr_mem;
+              GElf_Ehdr* em = gelf_getehdr (elf, &ehdr_mem);
+              int elf_machine = em->e_machine;
+              if (elf_machine == EM_MIPS)
+                {
+                  if (sess.verbose > 1)
+                    clog << "Hack reloc=" << hex << reloc_addr
+                      << " from addr=" << hex << addr;
+                  reloc_addr = addr - 0xc0000400;
+                }
+            }
           results.push_back (new dwarf_derived_probe(funcname, filename, line,
                                                      module, reloc_section, addr, reloc_addr,
                                                      *this, scope_die));

At the time I did the hack I was unaware of -msym32, I just wanted something to work. If I compile with KBUILD_SYM32=no it seems that the above hack might no longer be necessary (the addresses "look" right). I can't actually test the generated probes because my system won't boot with KBUILD_SYM32=no.

Apparently the gcc folks decided that this -msym32 behavior was too confusing and changed it to generate dwarf with a pointer size of 8:
https://gcc.gnu.org/ml/gcc/2009-01/msg00611.html

In the future this might not matter, but my cross-compiler is based on gcc-4.3.3. I'm posting this here because maybe somebody has a better idea about how to deal with this strange flavor of dwarf.

If this could be detected and handled inside systemtap fully then maybe you could apply the patches. Elfutils seems mostly blameless to me, it's just reading the information that GCC wrote.

Regards,
Leonard