Re: libebl: Add ebl_func_addr_mask plus ARM backend implementation. (Was: [PATCH] libebl: Add ebl_unwind_ret_mask.)

Re: libebl: Add ebl_func_addr_mask plus ARM backend implementation. (Was: [PATCH] libebl: Add ebl_unwind_ret_mask.)

[Mark Wielaard]

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

On Tue, 2014-06-17 at 22:09 +0200, Mark Wielaard wrote:
>     libebl: Add ebl_func_addr_mask plus ARM backend implementation.
>     
>     The ARM EABI says that the zero bit of function symbol st_value indicates
>     whether the symbol points to a THUMB or ARM function. Also the return
>     value address in an unwind will contain the same extra bit to indicate
>     whether to return to a regular ARM or THUMB function. Add a new ebl
>     function to mask off such bits and turn a function value into a function
>     address so that we get the actual value that a function symbol or return
>     address points to. It isn't easily possible to reuse the existing
>     ebl_resolve_sym_value for this purpose, so we end up with another hook
>     that can be used from dwfl_module_getsym, handle_cfi and elflint.
>     
>     Signed-off-by: Mark Wielaard <mjw@redhat.com>

Pushed after testing against Fedora armv7hl, which doesn't use THUMB by
default and Debian armhf which does use THUMB by default.

Cheers,

Mark

libebl: Add ebl_func_addr_mask plus ARM backend implementation. (Was: [PATCH] libebl: Add ebl_unwind_ret_mask.)

[Mark Wielaard]

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

On Sun, 2014-06-15 at 12:50 +0200, Mark Wielaard wrote:
> On Sun, 2014-06-15 at 12:39 +0200, Kurt Roeckx wrote:
> > On Sun, Jun 15, 2014 at 12:30:02PM +0200, Mark Wielaard wrote:
> > > Another ARM oddity. A return value address in an unwind will contain an
> > > extra bit to indicate whether to return to a regular ARM or THUMB function.
> > > Add a new ebl function to return a mask to use to get the actual return
> > > address during an unwind ebl_unwind_ret_mask.
> > 
> > Does this fix all the issues on armhf now, including the
> > backtrace?
> 
> Yes, you'll need this patch, the "libebl: Add sym_func_value hook" and
> the "tests: backtrace.c accept __libc_do_syscall as first frame symname"
> patches. With libc6-dbg installed all tests should PASS with 3 SKIPs,
> without libc6-dbg installed you should see 6 SKIPs (no FAILs).
> 
> The other recent patches should be optional, but help for arches which
> don't have a fully working unwind backend and make the tests a bit more
> robust and clear when something does go wrong.

I did push the test suite cleanup patches to master now and merged the
two separate ebl_unwind_ret_mask and sym_func_value hook patches into
one using just one new ebl function for both use cases as Roland
suggested. See attached (and on the mjw/pending branch).

Cheers,

Mark

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

commit 170fac58d8db9b623aec22e6c49d985811a4cf82
Author: Mark Wielaard <mjw@redhat.com>
Date:   Sat Jun 14 17:15:37 2014 +0200

    libebl: Add ebl_func_addr_mask plus ARM backend implementation.
    
    The ARM EABI says that the zero bit of function symbol st_value indicates
    whether the symbol points to a THUMB or ARM function. Also the return
    value address in an unwind will contain the same extra bit to indicate
    whether to return to a regular ARM or THUMB function. Add a new ebl
    function to mask off such bits and turn a function value into a function
    address so that we get the actual value that a function symbol or return
    address points to. It isn't easily possible to reuse the existing
    ebl_resolve_sym_value for this purpose, so we end up with another hook
    that can be used from dwfl_module_getsym, handle_cfi and elflint.
    
    Signed-off-by: Mark Wielaard <mjw@redhat.com>

diff --git a/backends/ChangeLog b/backends/ChangeLog
index bc5b843..aa1d717 100644
--- a/backends/ChangeLog
+++ b/backends/ChangeLog
@@ -1,3 +1,7 @@
+2014-06-17  Mark Wielaard  <mjw@redhat.com>
+
+	* arm_init.c (arm_init): Set func_addr_mask.
+
 2014-05-19  Mark Wielaard  <mjw@redhat.com>
 
 	* arm_init.c (arm_init): Hook check_reloc_target_type.
diff --git a/backends/arm_init.c b/backends/arm_init.c
index 92e6cd5..3283c97 100644
--- a/backends/arm_init.c
+++ b/backends/arm_init.c
@@ -69,5 +69,8 @@ arm_init (elf, machine, eh, ehlen)
   eh->frame_nregs = 16;
   HOOK (eh, set_initial_registers_tid);
 
+  /* Bit zero encodes whether an function address is THUMB or ARM. */
+  eh->func_addr_mask = ~(GElf_Addr)1;
+
   return MODVERSION;
 }
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index f1bc1a7..e11b7d1 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2014-06-17  Mark Wielaard  <mjw@redhat.com>
+
+	* frame_unwind.c (handle_cfi): Use ebl_func_addr_mask.
+	* dwfl_module_getsym.c (__libdwfl_getsym): Likewise.
+
 2014-06-15  Mark Wielaard  <mjw@redhat.com>
 
 	* linux-core-attach.c (core_memory_read): Use libdw/memory-access.h
diff --git a/libdwfl/dwfl_module_getsym.c b/libdwfl/dwfl_module_getsym.c
index 917d062..42d2b67 100644
--- a/libdwfl/dwfl_module_getsym.c
+++ b/libdwfl/dwfl_module_getsym.c
@@ -1,5 +1,5 @@
 /* Find debugging and symbol information for a module in libdwfl.
-   Copyright (C) 2006-2013 Red Hat, Inc.
+   Copyright (C) 2006-2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -119,7 +119,7 @@ __libdwfl_getsym (Dwfl_Module *mod, int ndx, GElf_Sym *sym, GElf_Addr *addr,
      descriptors).  */
 
   char *ident;
-  GElf_Addr st_value = sym->st_value;
+  GElf_Addr st_value = sym->st_value & ebl_func_addr_mask (mod->ebl);
   *resolved = false;
   if (! adjust_st_value && mod->e_type != ET_REL && alloc
       && (GELF_ST_TYPE (sym->st_info) == STT_FUNC
diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
index 18c808b..16cebd0 100644
--- a/libdwfl/frame_unwind.c
+++ b/libdwfl/frame_unwind.c
@@ -1,5 +1,5 @@
 /* Get previous frame state for an existing frame state.
-   Copyright (C) 2013 Red Hat, Inc.
+   Copyright (C) 2013, 2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -582,6 +582,10 @@ handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI *cfi, Dwarf_Addr bias)
 	  continue;
 	}
 
+      /* Some architectures encode some extra info in the return address.  */
+      if (regno == frame->fde->cie->return_address_register)
+	regval &= ebl_func_addr_mask (ebl);
+
       /* This is another strange PPC[64] case.  There are two
 	 registers numbers that can represent the same DWARF return
 	 register number.  We only want one to actually set the return
diff --git a/libebl/ChangeLog b/libebl/ChangeLog
index 7198d5e..5ec7101 100644
--- a/libebl/ChangeLog
+++ b/libebl/ChangeLog
@@ -1,3 +1,9 @@
+2014-06-17  Mark Wielaard  <mjw@redhat.com>
+
+	* eblinitreg.c (ebl_func_addr_mask): New function.
+	* libebl.h (ebl_func_addr_mask): Define.
+	* libeblP.h (struct ebl): Add func_addr_mask.
+
 2014-05-19  Mark Wielaard  <mjw@redhat.com>
 
 	* Makefile.am (gen_SOURCES): Add eblcheckreloctargettype.c.
diff --git a/libebl/ebl-hooks.h b/libebl/ebl-hooks.h
index 65c62ec..e1186f8 100644
--- a/libebl/ebl-hooks.h
+++ b/libebl/ebl-hooks.h
@@ -187,7 +187,7 @@ bool EBLHOOK(unwind) (Ebl *ebl, Dwarf_Addr pc, ebl_tid_registers_t *setfunc,
 		      bool *signal_framep);
 
 /* Returns true if the value can be resolved to an address in an
-   allocated section, which will be returned in *SHNDXP.
+   allocated section, which will be returned in *ADDR.
    (e.g. function descriptor resolving)  */
 bool EBLHOOK(resolve_sym_value) (Ebl *ebl, GElf_Addr *addr);
 
diff --git a/libebl/eblinitreg.c b/libebl/eblinitreg.c
index 8909c50..5729b3c 100644
--- a/libebl/eblinitreg.c
+++ b/libebl/eblinitreg.c
@@ -1,5 +1,5 @@
 /* Fetch live process Dwfl_Frame from PID.
-   Copyright (C) 2013 Red Hat, Inc.
+   Copyright (C) 2013, 2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -49,3 +49,10 @@ ebl_frame_nregs (Ebl *ebl)
 {
   return ebl == NULL ? 0 : ebl->frame_nregs;
 }
+
+GElf_Addr
+ebl_func_addr_mask (Ebl *ebl)
+{
+  return ((ebl == NULL || ebl->func_addr_mask == 0)
+	  ? ~(GElf_Addr)0 : ebl->func_addr_mask);
+}
diff --git a/libebl/libebl.h b/libebl/libebl.h
index d05751f..bb993bf 100644
--- a/libebl/libebl.h
+++ b/libebl/libebl.h
@@ -1,5 +1,5 @@
 /* Interface for libebl.
-   Copyright (C) 2000-2010, 2013 Red Hat, Inc.
+   Copyright (C) 2000-2010, 2013, 2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -409,6 +409,17 @@ extern bool ebl_set_initial_registers_tid (Ebl *ebl,
 extern size_t ebl_frame_nregs (Ebl *ebl)
   __nonnull_attribute__ (1);
 
+/* Mask to use for function symbol or unwind return addresses in case
+   the architecture adds some extra non-address bits to it.  This is
+   different from ebl_resolve_sym_value which only works for actual
+   symbol addresses (in non-ET_REL files) that might resolve to an
+   address in a different section.  ebl_func_addr_mask is called to
+   turn a given function value into the a real address or offset (the
+   original value might not be a real address).  This works for all
+   cases where an actual function address (or offset in ET_REL symbol
+   tables) is needed.  */
+extern GElf_Addr ebl_func_addr_mask (Ebl *ebl);
+
 /* Convert *REGNO as is in DWARF to a lower range suitable for
    Dwarf_Frame->REGS indexing.  */
 extern bool ebl_dwarf_to_regno (Ebl *ebl, unsigned *regno)
diff --git a/libebl/libeblP.h b/libebl/libeblP.h
index f91c2a0..dbd67f3 100644
--- a/libebl/libeblP.h
+++ b/libebl/libeblP.h
@@ -1,5 +1,5 @@
 /* Internal definitions for interface for libebl.
-   Copyright (C) 2000-2009, 2013 Red Hat, Inc.
+   Copyright (C) 2000-2009, 2013, 2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -64,6 +64,12 @@ struct ebl
      Ebl architecture can unwind iff FRAME_NREGS > 0.  */
   size_t frame_nregs;
 
+  /* Mask to use to turn a function value into a real function address
+     in case the architecture adds some extra non-address bits to it.
+     If not initialized (0) then ebl_func_addr_mask will return ~0,
+     otherwise it should be the actual mask to use.  */
+  GElf_Addr func_addr_mask;
+
   /* Function descriptor load address and table as used by
      ebl_resolve_sym_value if available for this arch.  */
   GElf_Addr fd_addr;
diff --git a/src/ChangeLog b/src/ChangeLog
index 7e68036..3023423 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,7 @@
+2014-06-14  Mark Wielaard  <mjw@redhat.com>
+
+	* elflint (check_symtab): Use ebl_func_addr_mask on st_value.
+
 2014-05-27  Mark Wielaard  <mjw@redhat.com>
 
 	* readelf.c (print_debug): Skip section if name is NULL.
diff --git a/src/elflint.c b/src/elflint.c
index bf6d044..5568c65 100644
--- a/src/elflint.c
+++ b/src/elflint.c
@@ -768,12 +768,18 @@ section [%2d] '%s': symbol %zu: function in COMMON section is nonsense\n"),
 	    {
 	      GElf_Addr sh_addr = (ehdr->e_type == ET_REL ? 0
 				   : destshdr->sh_addr);
+	      GElf_Addr st_value;
+	      if (GELF_ST_TYPE (sym->st_info) == STT_FUNC
+		  || (GELF_ST_TYPE (sym->st_info) == STT_GNU_IFUNC))
+		st_value = sym->st_value & ebl_func_addr_mask (ebl);
+	      else
+		st_value = sym->st_value;
 	      if (GELF_ST_TYPE (sym->st_info) != STT_TLS)
 		{
 		  if (! ebl_check_special_symbol (ebl, ehdr, sym, name,
 						  destshdr))
 		    {
-		      if (sym->st_value - sh_addr > destshdr->sh_size)
+		      if (st_value - sh_addr > destshdr->sh_size)
 			{
 			  /* GNU ld has severe bugs.  When it decides to remove
 			     empty sections it leaves symbols referencing them
@@ -798,7 +804,7 @@ section [%2d] '%s': symbol %zu: function in COMMON section is nonsense\n"),
 section [%2d] '%s': symbol %zu: st_value out of bounds\n"),
 				   idx, section_name (ebl, idx), cnt);
 			}
-		      else if ((sym->st_value - sh_addr
+		      else if ((st_value - sh_addr
 				+ sym->st_size) > destshdr->sh_size)
 			ERROR (gettext ("\
 section [%2d] '%s': symbol %zu does not fit completely in referenced section [%2d] '%s'\n"),
@@ -818,12 +824,12 @@ section [%2d] '%s': symbol %zu: referenced section [%2d] '%s' does not have SHF_
 		    {
 		      /* For object files the symbol value must fall
 			 into the section.  */
-		      if (sym->st_value > destshdr->sh_size)
+		      if (st_value > destshdr->sh_size)
 			ERROR (gettext ("\
 section [%2d] '%s': symbol %zu: st_value out of bounds of referenced section [%2d] '%s'\n"),
 			       idx, section_name (ebl, idx), cnt,
 			       (int) xndx, section_name (ebl, xndx));
-		      else if (sym->st_value + sym->st_size
+		      else if (st_value + sym->st_size
 			       > destshdr->sh_size)
 			ERROR (gettext ("\
 section [%2d] '%s': symbol %zu does not fit completely in referenced section [%2d] '%s'\n"),
@@ -852,20 +858,20 @@ section [%2d] '%s': symbol %zu: TLS symbol but no TLS program header entry\n"),
 			}
 		      else
 			{
-			  if (sym->st_value
+			  if (st_value
 			      < destshdr->sh_offset - phdr->p_offset)
 			    ERROR (gettext ("\
 section [%2d] '%s': symbol %zu: st_value short of referenced section [%2d] '%s'\n"),
 				   idx, section_name (ebl, idx), cnt,
 				   (int) xndx, section_name (ebl, xndx));
-			  else if (sym->st_value
+			  else if (st_value
 				   > (destshdr->sh_offset - phdr->p_offset
 				      + destshdr->sh_size))
 			    ERROR (gettext ("\
 section [%2d] '%s': symbol %zu: st_value out of bounds of referenced section [%2d] '%s'\n"),
 				   idx, section_name (ebl, idx), cnt,
 				   (int) xndx, section_name (ebl, xndx));
-			  else if (sym->st_value + sym->st_size
+			  else if (st_value + sym->st_size
 				   > (destshdr->sh_offset - phdr->p_offset
 				      + destshdr->sh_size))
 			    ERROR (gettext ("\