RFC/RFA: MN10300: Fix handling of protected functions in shared libraries.

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

RFC/RFA: MN10300: Fix handling of protected functions in shared libraries.

Nick Clifton
Hi Alex, Hi Jeff, Hi Richard,

  Consider the following small test case:

    % cat test1.c
    extern int g (void) __attribute__ ((visibility("protected")));
    int f (void) { return g (); }

    % cat test2.c
    extern int g(void) __attribute__ ((visibility("protected")));
    int i;
    int g (void) { return i; }

    % gcc -fPIC -c test1.c test2.c
    % gcc -shared -o libtest_protected.so test1.o test2.o

  When compiled with a MN10300 toolchain based on the current FSF GCC
  and binutils sources the final link will fail with:
   
    test1.o: In function `f':
    test1.c:(.text+0x7): warning: error: inappropriate relocation type for shared library (did you forget -fpic?)

  The problem here is that GCC has decided that since "g" is protected
  it does not need a PLT entry.  But the linker has decided that since
  "g" is a function it does need a PLT entry (even though it is
  protected) so that function pointer comparison will work.  (See the
  definition of SYMBOL_REFERENCES_LOCAL in bfd/elf-bfd.h for more
  information on this).

  I have a small patch that fixes this problem (see below), but I am not
  sure if this is the correct solution.  As far as I can see, this is
  not an MN10300 specific problem however, so surely other targets ought
  to have similar problems ?  (But other targets do not seem to use
  targetm.binds_local_p to decide if a symbol is global or local.  cf/
  mn10300_encode_section_info).

  Anyway any advice or comments on the situation would be appreciated.

Cheers
  Nick

Index: gcc/config/mn10300/mn10300.c
===================================================================
--- gcc/config/mn10300/mn10300.c (revision 174069)
+++ gcc/config/mn10300/mn10300.c (working copy)
@@ -3315,8 +3315,30 @@
     }
 }
 
+static bool
+mn10300_binds_local (const_tree exp)
+{
+  bool local = default_binds_local_p (exp);
+
+  /* The default binds_local function will decide that protected functions
+     bind locally.  Whilst technically this is true, in practice in PIC mode
+     protected functions still need a PLT entry so that function pointer
+     comparison will work.  */
+  if (local
+      && flag_pic
+      && DECL_P (exp)
+      && TREE_CODE (exp) == FUNCTION_DECL
+      && DECL_VISIBILITY (exp) == VISIBILITY_PROTECTED)
+    return false;
+
+  return local;
+}
+
 /* Initialize the GCC target structure.  */
 
+#undef  TARGET_BINDS_LOCAL_P
+#define TARGET_BINDS_LOCAL_P mn10300_binds_local
+
 #undef  TARGET_MACHINE_DEPENDENT_REORG
 #define TARGET_MACHINE_DEPENDENT_REORG mn10300_reorg
 
 
Reply | Threaded
Open this post in threaded view
|

Re: RFC/RFA: MN10300: Fix handling of protected functions in shared libraries.

Jeff Law
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/23/11 09:21, Nick Clifton wrote:

> Hi Alex, Hi Jeff, Hi Richard,
>
>   Consider the following small test case:
>
>     % cat test1.c
>     extern int g (void) __attribute__ ((visibility("protected")));
>     int f (void) { return g (); }
>
>     % cat test2.c
>     extern int g(void) __attribute__ ((visibility("protected")));
>     int i;
>     int g (void) { return i; }
>
>     % gcc -fPIC -c test1.c test2.c
>     % gcc -shared -o libtest_protected.so test1.o test2.o
>
>   When compiled with a MN10300 toolchain based on the current FSF GCC
>   and binutils sources the final link will fail with:
>    
>     test1.o: In function `f':
>     test1.c:(.text+0x7): warning: error: inappropriate relocation type for shared library (did you forget -fpic?)
>
>   The problem here is that GCC has decided that since "g" is protected
>   it does not need a PLT entry.  But the linker has decided that since
>   "g" is a function it does need a PLT entry (even though it is
>   protected) so that function pointer comparison will work.  (See the
>   definition of SYMBOL_REFERENCES_LOCAL in bfd/elf-bfd.h for more
>   information on this).
>
>   I have a small patch that fixes this problem (see below), but I am not
>   sure if this is the correct solution.  As far as I can see, this is
>   not an MN10300 specific problem however, so surely other targets ought
>   to have similar problems ?  (But other targets do not seem to use
>   targetm.binds_local_p to decide if a symbol is global or local.  cf/
>   mn10300_encode_section_info).
>
>   Anyway any advice or comments on the situation would be appreciated.
This isn't something I have much experience with; presumably we must
have the PLT entry even though "g" is considered protected?  If so, then
ISTM GCC will need to cope/adapt and your approach seems as good as any
other to me -- unless we wanted to move the code into a more generic
location.

jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJN2ohHAAoJEBRtltQi2kC7lpcH/ihLGKBhkB7PG2ZjkXZJBH2l
WMuG7lVDSKiveqLTCI5o4bXRue2Xt+cusYGn/+nUGv5oMfPZCb5qmypT58e0IvVM
w1oFks9X+0DyhuIQ0gD4BEbS+R36KkkMlnI4zGqpUq4J+i6XJFIrurw4DghJ22Pe
tU7P7C7+uUeAH5f/Be3QIx1MNl0BnruyzPRSrXzGnrg5DP6yyXdwE8SmAhqEuUvJ
UkTqT1HWDVgS1eN+lh8tHUUu2ygqoTELpVOwVkvTXP1qrJKmwgbV+3Tvu8jMfvYJ
olm2HWUINPX2zvlaqfYUNB+5gFifkqVg7JXOF+NUasaUZwPIiReGaxbsRSFt7Ug=
=zrdM
-----END PGP SIGNATURE-----
Reply | Threaded
Open this post in threaded view
|

Re: RFC/RFA: MN10300: Fix handling of protected functions in shared libraries.

Alan Modra-3
In reply to this post by Nick Clifton
On Mon, May 23, 2011 at 04:21:38PM +0100, Nick Clifton wrote:
>   The problem here is that GCC has decided that since "g" is protected
>   it does not need a PLT entry.

Right.

>  But the linker has decided that since
>   "g" is a function it does need a PLT entry (even though it is
>   protected) so that function pointer comparison will work.

Nope.  This sounds like a linker bug.  You don't actually need a plt
entry in the shared lib.  What you do need to do is be careful about
taking the address of "g".  The address should *not* resolve locally,
but rather to the same address used by the executable, ie. the plt
entry in the executable.  That just means that "g" must be dynamic.

>  (See the
>   definition of SYMBOL_REFERENCES_LOCAL in bfd/elf-bfd.h for more
>   information on this).

Ick, I see that comment is mine.  It needs clarifying and should have
been moved when _bfd_elf_symbol_refs_local_p was written.  "reference
the .plt" should have been "reference the .plt entry in the
executable".

        * elf-bfd.h (SYMBOL_REFERENCES_LOCAL): Remove most of comment.
        * elflink.c (_bfd_elf_symbol_refs_local_p): Expand
        local_protected comment.

Index: bfd/elf-bfd.h
===================================================================
RCS file: /cvs/src/src/bfd/elf-bfd.h,v
retrieving revision 1.319
diff -u -p -r1.319 elf-bfd.h
--- bfd/elf-bfd.h 23 May 2011 06:22:50 -0000 1.319
+++ bfd/elf-bfd.h 26 May 2011 04:13:18 -0000
@@ -232,11 +232,7 @@ struct elf_link_hash_entry
 };
 
 /* Will references to this symbol always reference the symbol
-   in this object?  STV_PROTECTED is excluded from the visibility test
-   here so that function pointer comparisons work properly.  Since
-   function symbols not defined in an app are set to their .plt entry,
-   it's necessary for shared libs to also reference the .plt even
-   though the symbol is really local to the shared lib.  */
+   in this object?  */
 #define SYMBOL_REFERENCES_LOCAL(INFO, H) \
   _bfd_elf_symbol_refs_local_p (H, INFO, 0)
 
Index: bfd/elflink.c
===================================================================
RCS file: /cvs/src/src/bfd/elflink.c,v
retrieving revision 1.405
diff -u -p -r1.405 elflink.c
--- bfd/elflink.c 18 May 2011 14:04:31 -0000 1.405
+++ bfd/elflink.c 26 May 2011 04:13:23 -0000
@@ -2881,8 +2881,10 @@ _bfd_elf_symbol_refs_local_p (struct elf
     return TRUE;
 
   /* Function pointer equality tests may require that STV_PROTECTED
-     symbols be treated as dynamic symbols, even when we know that the
-     dynamic linker will resolve them locally.  */
+     symbols be treated as dynamic symbols.  If the address of a
+     function not defined in an executable is set to that function's
+     plt entry in the executable, then the address of the function in
+     a shared library must also be the plt entry in the executable.  */
   return local_protected;
 }
 

--
Alan Modra
Australia Development Lab, IBM