Patchwork [trans-mem,darwin] PR/52042 find tm_clone_table with PIE

login
register
mail settings
Submitter Patrick Marlier
Date Feb. 8, 2012, 3:36 a.m.
Message ID <4F31EDC9.2060600@gmail.com>
Download mbox | patch
Permalink /patch/140053/
State New
Headers show

Comments

Patrick Marlier - Feb. 8, 2012, 3:36 a.m.
Hi,

The problem in this PR is that with PIE, getsectdata does not return the 
position of tm_clone_table after the relocation.
While _dyld_get_image_vmaddr_slide(0) is enough for PIE, this is not 
enough for dylib.
I did not find an easy API function to get position of the 
tm_clone_table for a shared library (dylib). So the only way I found is 
to get the mach_header address of the current dylib (via 
_dyld_get_image_header_containing_address), iterate over loaded binaries 
to find the current shared library and use _dyld_get_image_vmaddr_slide 
to find the position.
Any other proposal (my knowledge of darwin is really limited)?

Can someone do a bootstrap and test libitm on darwin (I have a limited 
access to a darwin machine, at least libitm tests pass)? Thanks!

If tests passed, ok for 4.7?
--
Patrick Marlier.
libgcc:

	PR libitm/52042
         * config/darwin-crt-tm.c: Changes for PIE and shared library.
Jack Howarth - Feb. 8, 2012, 5:12 a.m.
On Tue, Feb 07, 2012 at 10:36:41PM -0500, Patrick Marlier wrote:
> Hi,
>
> The problem in this PR is that with PIE, getsectdata does not return the  
> position of tm_clone_table after the relocation.
> While _dyld_get_image_vmaddr_slide(0) is enough for PIE, this is not  
> enough for dylib.
> I did not find an easy API function to get position of the  
> tm_clone_table for a shared library (dylib). So the only way I found is  
> to get the mach_header address of the current dylib (via  
> _dyld_get_image_header_containing_address), iterate over loaded binaries  
> to find the current shared library and use _dyld_get_image_vmaddr_slide  
> to find the position.
> Any other proposal (my knowledge of darwin is really limited)?
>
> Can someone do a bootstrap and test libitm on darwin (I have a limited  
> access to a darwin machine, at least libitm tests pass)? Thanks!

Done.

Native configuration is x86_64-apple-darwin11.3.0

		=== libitm tests ===


Running target unix/-m32
FAIL: libitm.c++/eh-1.C execution test

		=== libitm Summary for unix/-m32 ===

# of expected passes		25
# of unexpected failures	1
# of expected failures		3
# of unsupported tests		1

Running target unix/-m64
FAIL: libitm.c++/eh-1.C execution test

		=== libitm Summary for unix/-m64 ===

# of expected passes		25
# of unexpected failures	1
# of expected failures		3
# of unsupported tests		1

		=== libitm Summary ===

# of expected passes		50
# of unexpected failures	2
# of expected failures		6
# of unsupported tests		2

Compiler version: gcc libitm 
Platform: x86_64-apple-darwin11.3.0
configure flags: --prefix=/sw --prefix=/sw/lib/gcc4.7 --mandir=/sw/share/man --infodir=/sw/lib/gcc4.7/info --with-build-config=bootstrap-lto --enable-stage1-languages=c,lto --enable-languages=c,c++,fortran,lto,objc,obj-c++,java --with-gmp=/sw --with-libiconv-prefix=/sw --with-ppl=/sw --with-cloog=/sw --with-mpc=/sw --with-system-zlib --x-includes=/usr/X11R6/include --x-libraries=/usr/X11R6/lib --program-suffix=-fsf-4.7 --enable-checking=release --enable-cloog-backend=isl

I believe the remaining libitm.c++/eh-1.C execution test failures are due to
the weakref linker bug currently in Xcode 4.x (radr://10466868) which I hae been
told will be fixed in the next Xcode release after Xcode 4.3.
                Jack

>
> If tests passed, ok for 4.7?
> --
> Patrick Marlier.
> libgcc:
>
> 	PR libitm/52042
>         * config/darwin-crt-tm.c: Changes for PIE and shared library.
>

> Index: config/darwin-crt-tm.c
> ===================================================================
> --- config/darwin-crt-tm.c	(revision 183968)
> +++ config/darwin-crt-tm.c	(working copy)
> @@ -26,8 +26,20 @@ see the files COPYING3 and COPYING.RUNTIME respect
>  #include <mach-o/dyld.h>
>  
>  /* not listed in mach-o/dyld.h for some reason.  */
> -extern char * getsectdata (const char*,const char*,unsigned long*); 
> +extern char *getsectdatafromheader (struct mach_header*, const char*,
> +				    const char*, unsigned long*);
> +extern char *getsectdatafromheader_64 (struct mach_header_64*, const char*,
> +				       const char*, unsigned long*);
>  
> +#ifdef __LP64__
> +#define GET_DATA_TMCT(mh,size) \
> +  getsectdatafromheader_64 ((struct mach_header_64*) mh, \
> +			    "__DATA", "__tm_clone_table", size)
> +#else
> +#define GET_DATA_TMCT(mh,size) \
> +  getsectdatafromheader (mh, "__DATA", "__tm_clone_table", size)
> +#endif
> +
>  #define WEAK __attribute__((weak))
>  
>  extern void _ITM_registerTMCloneTable (void *, size_t) WEAK;
> @@ -39,17 +51,27 @@ void __doTMRegistrations (void) __attribute__ ((co
>  
>  void __doTMRegistrations (void)
>  {
> -  char * tm_clone_table_sect_data;
> +  struct mach_header *mh;
> +  char *tmct_fixed, *tmct = NULL;
>    unsigned long tmct_siz;
> +  unsigned int i, img_count; 
>    
> -  tm_clone_table_sect_data = getsectdata ("__DATA",
> -					  "__tm_clone_table",
> -					  &tmct_siz);
> +  mh = _dyld_get_image_header_containing_address (
> +		(const void*)&__doTMRegistrations);
> +  tmct_fixed = GET_DATA_TMCT (mh, &tmct_siz);
>    tmct_siz /= (sizeof (size_t) * 2);
> +
> +  img_count = _dyld_image_count();
> +  for (i = 0; i < img_count && tmct == NULL; i++)
> +    {
> +      if (mh == _dyld_get_image_header(i))
> +	tmct = tmct_fixed + (unsigned long)_dyld_get_image_vmaddr_slide(i); 
> +    }
> +
>    if (_ITM_registerTMCloneTable != NULL
> -      && tm_clone_table_sect_data != NULL
> +      && tmct != NULL
>        && tmct_siz > 0)
> -    _ITM_registerTMCloneTable (tm_clone_table_sect_data, (size_t)tmct_siz);
> +    _ITM_registerTMCloneTable ((void *)tmct, (size_t)tmct_siz);
>  }
>  
>  #endif
> @@ -60,18 +82,27 @@ void __doTMdeRegistrations (void) __attribute__ ((
>  
>  void __doTMdeRegistrations (void)
>  {
> -  char * tm_clone_table_sect_data;
> +  struct mach_header *mh;
> +  char *tmct_fixed, *tmct = NULL;
>    unsigned long tmct_siz;
> +  unsigned int i, img_count; 
>    
> -  tm_clone_table_sect_data = getsectdata ("__DATA",
> -					  "__tm_clone_table",
> -					  &tmct_siz);
> -  
> +  mh = _dyld_get_image_header_containing_address (
> +		(const void *)&__doTMdeRegistrations);
> +  tmct_fixed = GET_DATA_TMCT (mh, &tmct_siz);
> +  tmct_siz /= (sizeof (size_t) * 2);
> +
> +  img_count = _dyld_image_count();
> +  for (i = 0; i < img_count && tmct == NULL; i++)
> +    {
> +      if (mh == _dyld_get_image_header(i))
> +	tmct = tmct_fixed + (unsigned long)_dyld_get_image_vmaddr_slide(i); 
> +    }
> +
>    if (_ITM_deregisterTMCloneTable != NULL
> -      && tm_clone_table_sect_data != NULL
> +      && tmct != NULL
>        && tmct_siz > 0)
> -    _ITM_deregisterTMCloneTable (tm_clone_table_sect_data);
> -
> +    _ITM_deregisterTMCloneTable ((void *)tmct);
>  }
>  
>  #endif
IainS - Feb. 8, 2012, 11:33 a.m.
Hi Patrick,

thanks for looking at this ..

On 8 Feb 2012, at 03:36, Patrick Marlier wrote:

> The problem in this PR is that with PIE, getsectdata does not return  
> the position of tm_clone_table after the relocation.
> While _dyld_get_image_vmaddr_slide(0) is enough for PIE, this is not  
> enough for dylib.
> I did not find an easy API function to get position of the  
> tm_clone_table for a shared library (dylib). So the only way I found  
> is to get the mach_header address of the current dylib (via  
> _dyld_get_image_header_containing_address), iterate over loaded  
> binaries to find the current shared library and use  
> _dyld_get_image_vmaddr_slide to find the position.
> Any other proposal (my knowledge of darwin is really limited)?
>
> Can someone do a bootstrap and test libitm on darwin (I have a  
> limited access to a darwin machine, at least libitm tests pass)?  
> Thanks!
>
> If tests passed, ok for 4.7?
> --
> Patrick Marlier.
> libgcc:
>
> 	PR libitm/52042
>        * config/darwin-crt-tm.c: Changes for PIE and shared library.
>
> <darwin-pie2.patch>

looks good to me .. and DTRT on Darwin 9 too .. (but I can't approve).

one nit (also mea culpa in the original...)

  /* not listed in mach-o/dyld.h for some reason.  */
-extern char * getsectdata (const char*,const char*,unsigned long*);
+extern char *getsectdatafromheader (struct mach_header*, const char*,
+				    const char*, unsigned long*);
+extern char *getsectdatafromheader_64 (struct mach_header_64*, const  
char*,
+				       const char*, unsigned long*);

these are in <mach-o/getsect.h>

(dyld documentation glitch ...)

===

Sometime: I wonder if we should move the definition of the section  
name to a common place so that we don't need to worry about keeping it  
in step.  Or even better have some compiler-wide mechanism for dealing  
with the fact that named sections might have different literals/ 
semantics per target.

Iain
Mike Stump - Feb. 8, 2012, 9:14 p.m.
On Feb 7, 2012, at 7:36 PM, Patrick Marlier wrote:
> The problem in this PR is that with PIE, getsectdata does not return the position of tm_clone_table after the relocation.


> If tests passed, ok for 4.7?

Ok.  Thanks for all your hard work.  If you want to change it to include the header and delete the declarations in the source, that is pre-approved as well.

> 	PR libitm/52042
>        * config/darwin-crt-tm.c: Changes for PIE and shared library.
Patrick Marlier - Feb. 9, 2012, 5:10 a.m.
On 02/08/2012 12:12 AM, Jack Howarth wrote:
> I believe the remaining libitm.c++/eh-1.C execution test failures are due to
> the weakref linker bug currently in Xcode 4.x (radr://10466868) which I hae been
> told will be fixed in the next Xcode release after Xcode 4.3.
>                  Jack

Humm... In fact, not completely. Of course, if the linker was working, 
the HAVE_ELF_STYLE_WEAKREF will be defined and no problem.

Otherwise in the eh-1.c, we can see that _ITM_cxa_allocation_exception 
is using the dummy function __cxa_allocation_exception defined into 
eh_cpp.cc but not the one from libstdc++. There is no dynamic symbol 
resolution since the function is defined in the file.

I do not really know why those functions are defined here, I think it 
may causes more problems than it solves for darwin.

When Rainer introduced this, he mentioned "on Tru64 UNIX. Of course it 
cannot work to provide only a single dummy function, but all weak 
definitions must be backed by dummy definitions on that platform."
http://patchwork.ozlabs.org/patch/126007/

Iain mentioned this when he introduced the dummy def for darwin:
*** FWIW, weakref actually work (at runtime) for earlier Darwin
- it's just that refs either need to be satisfied by dummies at link time -
- or the library namespace has to be flattened (which is generally 
undesirable).

I think refs (from libstdc++) should be satisfied at link time with 
-lstdc++ but probably I am missing something?

Note that removing definitions in eh_cpp.cc make the test passes.

Thanks guys!
--
Patrick.
IainS - Feb. 9, 2012, 8:36 a.m.
Hi Patrick,

On 9 Feb 2012, at 05:10, Patrick Marlier wrote:

> On 02/08/2012 12:12 AM, Jack Howarth wrote:
>> I believe the remaining libitm.c++/eh-1.C execution test failures  
>> are due to
>> the weakref linker bug currently in Xcode 4.x (radr://10466868)  
>> which I hae been
>> told will be fixed in the next Xcode release after Xcode 4.3.
>>                 Jack
>
> Humm... In fact, not completely. Of course, if the linker was  
> working, the HAVE_ELF_STYLE_WEAKREF will be defined and no problem.
>
> Otherwise in the eh-1.c, we can see that  
> _ITM_cxa_allocation_exception is using the dummy function  
> __cxa_allocation_exception defined into eh_cpp.cc but not the one  
> from libstdc++. There is no dynamic symbol resolution since the  
> function is defined in the file.
>
> I do not really know why those functions are defined here, I think  
> it may causes more problems than it solves for darwin.
>
> When Rainer introduced this, he mentioned "on Tru64 UNIX. Of course  
> it cannot work to provide only a single dummy function, but all weak  
> definitions must be backed by dummy definitions on that platform."
> http://patchwork.ozlabs.org/patch/126007/
>
> Iain mentioned this when he introduced the dummy def for darwin:
> *** FWIW, weakref actually work (at runtime) for earlier Darwin

Weak refs have been working a (long) time on Darwin @ runtime - don't  
remember what version they were introduced ... but maybe from almost/ 
actually the start.

> - it's just that refs either need to be satisfied by dummies at link  
> time -

We are dealing with tool bugs (or, at least, changes in behavior) -  
the Linker would not [intentionally, see below*] accept unsatisfied  
linkage for weak refs @ Darwin  <= 9 (XCode 3.1.4) ... and then for  
the XCode 3.2.x tool-chain on Darwin 10 it does accept it (i.e. ELF- 
like semantics work).  Then (an acknowledged bug) with the (current)  
XCode 4.x tool-chain on either D10 or D11 the linker is again not  
accepting.

* The (original, Darwin version, weak ref) idea, as documented, is  
that weak refs are for optional features - so the developer @ link- 
time supplies the library reference (place where the reference would  
be found, if present - this tells dyld what the proper two-level name  
of the lib is, when present).  Then, at runtime dyld should not barf  
if the reference is not present.  This is a different approach from  
the ELF weak-ref where the reference does not need to be supplied @  
link-time - although it seems that Darwin is shifting towards  
supporting both from D10 onwards.

> - or the library namespace has to be flattened (which is generally  
> undesirable).

this would be a (IMO very) Bad Idea because we'd be doing it for every  
exe that included libitm... bleah.  The two-level lib namespace is one  
of Darwin's nicer features ;)

> I think refs (from libstdc++) should be satisfied at link time with - 
> lstdc++ but probably I am missing something?

when you link c++ code, that is what is required .. but obv. we don't  
want lstdc++ on the link line for c (because the library *will* be  
found @ runtime - and thus the routines would be called).

I have an idea ... perhaps we supply (yet another) crt that is linked  
last on the line and provides the dummies.  That way the linkage will  
be satisfied from libstd++ for c++ and the dummies for c.

will try and see if I can fit a trial of that idea in on D9 & D10.

> Note that removing definitions in eh_cpp.cc make the test passes.

but it will make the c tests fail for D9 and any version of Darwin on  
which elf-style weak refs is not currently working ...

cheers
Iain

>
> Thanks guys!
> --
> Patrick.
Patrick Marlier - Feb. 28, 2012, 8:20 p.m.
Jack,

Can I ask you to close this PR?
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52042
Indeed, my bugzilla account is a simple one and I cannot to change the 
bug status...

--
Patrick


On 02/07/2012 10:36 PM, Patrick Marlier wrote:
> Hi,
>
> The problem in this PR is that with PIE, getsectdata does not return the
> position of tm_clone_table after the relocation.
> While _dyld_get_image_vmaddr_slide(0) is enough for PIE, this is not
> enough for dylib.
> I did not find an easy API function to get position of the
> tm_clone_table for a shared library (dylib). So the only way I found is
> to get the mach_header address of the current dylib (via
> _dyld_get_image_header_containing_address), iterate over loaded binaries
> to find the current shared library and use _dyld_get_image_vmaddr_slide
> to find the position.
> Any other proposal (my knowledge of darwin is really limited)?
>
> Can someone do a bootstrap and test libitm on darwin (I have a limited
> access to a darwin machine, at least libitm tests pass)? Thanks!
>
> If tests passed, ok for 4.7?
> --
> Patrick Marlier.
> libgcc:
>
> PR libitm/52042
> * config/darwin-crt-tm.c: Changes for PIE and shared library.
>
Mike Stump - Feb. 28, 2012, 8:29 p.m.
On Feb 28, 2012, at 12:20 PM, Patrick Marlier wrote:
> Can I ask you to close this PR?

Done.

Patch

Index: config/darwin-crt-tm.c
===================================================================
--- config/darwin-crt-tm.c	(revision 183968)
+++ config/darwin-crt-tm.c	(working copy)
@@ -26,8 +26,20 @@  see the files COPYING3 and COPYING.RUNTIME respect
 #include <mach-o/dyld.h>
 
 /* not listed in mach-o/dyld.h for some reason.  */
-extern char * getsectdata (const char*,const char*,unsigned long*); 
+extern char *getsectdatafromheader (struct mach_header*, const char*,
+				    const char*, unsigned long*);
+extern char *getsectdatafromheader_64 (struct mach_header_64*, const char*,
+				       const char*, unsigned long*);
 
+#ifdef __LP64__
+#define GET_DATA_TMCT(mh,size) \
+  getsectdatafromheader_64 ((struct mach_header_64*) mh, \
+			    "__DATA", "__tm_clone_table", size)
+#else
+#define GET_DATA_TMCT(mh,size) \
+  getsectdatafromheader (mh, "__DATA", "__tm_clone_table", size)
+#endif
+
 #define WEAK __attribute__((weak))
 
 extern void _ITM_registerTMCloneTable (void *, size_t) WEAK;
@@ -39,17 +51,27 @@  void __doTMRegistrations (void) __attribute__ ((co
 
 void __doTMRegistrations (void)
 {
-  char * tm_clone_table_sect_data;
+  struct mach_header *mh;
+  char *tmct_fixed, *tmct = NULL;
   unsigned long tmct_siz;
+  unsigned int i, img_count; 
   
-  tm_clone_table_sect_data = getsectdata ("__DATA",
-					  "__tm_clone_table",
-					  &tmct_siz);
+  mh = _dyld_get_image_header_containing_address (
+		(const void*)&__doTMRegistrations);
+  tmct_fixed = GET_DATA_TMCT (mh, &tmct_siz);
   tmct_siz /= (sizeof (size_t) * 2);
+
+  img_count = _dyld_image_count();
+  for (i = 0; i < img_count && tmct == NULL; i++)
+    {
+      if (mh == _dyld_get_image_header(i))
+	tmct = tmct_fixed + (unsigned long)_dyld_get_image_vmaddr_slide(i); 
+    }
+
   if (_ITM_registerTMCloneTable != NULL
-      && tm_clone_table_sect_data != NULL
+      && tmct != NULL
       && tmct_siz > 0)
-    _ITM_registerTMCloneTable (tm_clone_table_sect_data, (size_t)tmct_siz);
+    _ITM_registerTMCloneTable ((void *)tmct, (size_t)tmct_siz);
 }
 
 #endif
@@ -60,18 +82,27 @@  void __doTMdeRegistrations (void) __attribute__ ((
 
 void __doTMdeRegistrations (void)
 {
-  char * tm_clone_table_sect_data;
+  struct mach_header *mh;
+  char *tmct_fixed, *tmct = NULL;
   unsigned long tmct_siz;
+  unsigned int i, img_count; 
   
-  tm_clone_table_sect_data = getsectdata ("__DATA",
-					  "__tm_clone_table",
-					  &tmct_siz);
-  
+  mh = _dyld_get_image_header_containing_address (
+		(const void *)&__doTMdeRegistrations);
+  tmct_fixed = GET_DATA_TMCT (mh, &tmct_siz);
+  tmct_siz /= (sizeof (size_t) * 2);
+
+  img_count = _dyld_image_count();
+  for (i = 0; i < img_count && tmct == NULL; i++)
+    {
+      if (mh == _dyld_get_image_header(i))
+	tmct = tmct_fixed + (unsigned long)_dyld_get_image_vmaddr_slide(i); 
+    }
+
   if (_ITM_deregisterTMCloneTable != NULL
-      && tm_clone_table_sect_data != NULL
+      && tmct != NULL
       && tmct_siz > 0)
-    _ITM_deregisterTMCloneTable (tm_clone_table_sect_data);
-
+    _ITM_deregisterTMCloneTable ((void *)tmct);
 }
 
 #endif