* [patch ld]: Close BFDs before linker-plugin's atexit routine is called @ 2011-02-09 18:52 Kai Tietz 2011-02-13 10:40 ` Kai Tietz 2011-02-13 23:25 ` Alan Modra 0 siblings, 2 replies; 7+ messages in thread From: Kai Tietz @ 2011-02-09 18:52 UTC (permalink / raw) To: Binutils; +Cc: Nick Clifton [-- Attachment #1: Type: text/plain, Size: 544 bytes --] Hello, This is patch addresses the unlink call when lto linker-plugin is used. As windows native doesn't support to unlink still opened files, it fails to do so as file-descriptors of bfds aren't closed before atexit routine of plugin gets called. 2011-02-09 Kai Tietz * ldmain.c (remove_output): Set output_bfd of link_info to nil and close all cached bfds. (main): Close output_bfd of link_info and set it to nil. Additionally close all cached bfds. Tested on x86_64-pc-linux-gnu and x86_64-w64-mingw32. Ok for apply? Regards, Kai [-- Attachment #2: ld_release_bfd.txt --] [-- Type: text/plain, Size: 1118 bytes --] Index: src/ld/ldmain.c =================================================================== --- src.orig/ld/ldmain.c 2011-01-18 19:49:25.000000000 +0100 +++ src/ld/ldmain.c 2011-02-09 18:49:38.148905600 +0100 @@ -180,6 +180,8 @@ remove_output (void) { if (link_info.output_bfd) bfd_cache_close (link_info.output_bfd); + link_info.output_bfd = NULL; + bfd_cache_close_all (); if (delete_output_file_on_failure) unlink_if_ordinary (output_filename); } @@ -489,6 +491,10 @@ main (int argc, char **argv) output_filename); /* The file will be removed by remove_output. */ + if (link_info.output_bfd) + bfd_cache_close (link_info.output_bfd); + link_info.output_bfd = NULL; + bfd_cache_close_all (); xexit (1); } else @@ -564,6 +570,10 @@ main (int argc, char **argv) /* Prevent remove_output from doing anything, after a successful link. */ output_filename = NULL; + if (link_info.output_bfd) + bfd_cache_close (link_info.output_bfd); + link_info.output_bfd = NULL; + bfd_cache_close_all (); xexit (0); return 0; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch ld]: Close BFDs before linker-plugin's atexit routine is called 2011-02-09 18:52 [patch ld]: Close BFDs before linker-plugin's atexit routine is called Kai Tietz @ 2011-02-13 10:40 ` Kai Tietz 2011-02-13 23:25 ` Alan Modra 1 sibling, 0 replies; 7+ messages in thread From: Kai Tietz @ 2011-02-13 10:40 UTC (permalink / raw) To: Binutils; +Cc: Nick Clifton PING. 2011/2/9 Kai Tietz <ktietz70@googlemail.com>: > Hello, > > This is patch addresses the unlink call when lto linker-plugin is > used. As windows > native doesn't support to unlink still opened files, it fails to do so > as file-descriptors > of bfds aren't closed before atexit routine of plugin gets called. > > 2011-02-09 Kai Tietz > > * ldmain.c (remove_output): Set output_bfd > of link_info to nil and close all cached bfds. > (main): Close output_bfd of link_info and set > it to nil. Additionally close all cached bfds. > > Tested on x86_64-pc-linux-gnu and x86_64-w64-mingw32. Ok for apply? > > Regards, > Kai > -- | (\_/) This is Bunny. Copy and paste | (='.'=) Bunny into your signature to help | (")_(") him gain world domination ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch ld]: Close BFDs before linker-plugin's atexit routine is called 2011-02-09 18:52 [patch ld]: Close BFDs before linker-plugin's atexit routine is called Kai Tietz 2011-02-13 10:40 ` Kai Tietz @ 2011-02-13 23:25 ` Alan Modra 2011-02-14 6:45 ` Kai Tietz 1 sibling, 1 reply; 7+ messages in thread From: Alan Modra @ 2011-02-13 23:25 UTC (permalink / raw) To: Kai Tietz; +Cc: Binutils, Nick Clifton On Wed, Feb 09, 2011 at 07:51:58PM +0100, Kai Tietz wrote: > Hello, > > This is patch addresses the unlink call when lto linker-plugin is > used. As windows > native doesn't support to unlink still opened files, it fails to do so > as file-descriptors > of bfds aren't closed before atexit routine of plugin gets called. > > 2011-02-09 Kai Tietz > > * ldmain.c (remove_output): Set output_bfd > of link_info to nil and close all cached bfds. > (main): Close output_bfd of link_info and set > it to nil. Additionally close all cached bfds. The fact that you need to patch three places to fix one problem says to me that this isn't the best fix.. Does a single bfd_cache_close_all at the start of plugin_call_cleanup fix your problem? -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch ld]: Close BFDs before linker-plugin's atexit routine is called 2011-02-13 23:25 ` Alan Modra @ 2011-02-14 6:45 ` Kai Tietz 2011-02-14 9:22 ` Kai Tietz 0 siblings, 1 reply; 7+ messages in thread From: Kai Tietz @ 2011-02-14 6:45 UTC (permalink / raw) To: Kai Tietz, Binutils, Nick Clifton 2011/2/14 Alan Modra <amodra@gmail.com>: > On Wed, Feb 09, 2011 at 07:51:58PM +0100, Kai Tietz wrote: >> Hello, >> >> This is patch addresses the unlink call when lto linker-plugin is >> used. As windows >> native doesn't support to unlink still opened files, it fails to do so >> as file-descriptors >> of bfds aren't closed before atexit routine of plugin gets called. >> >> 2011-02-09 Kai Tietz >> >> * ldmain.c (remove_output): Set output_bfd >> of link_info to nil and close all cached bfds. >> (main): Close output_bfd of link_info and set >> it to nil. Additionally close all cached bfds. Hi Alan, > The fact that you need to patch three places to fix one problem > says to me that this isn't the best fix.. Well, here I am not that sure about. I thought about fixing it just within the atexit-handler of plugin, but well, next one trying to opearate on files in a similar way, will have the same issues again. I think it is simply bad style to exit a program without even try to cleanup opened file descriptors and used memory. This could even help to use tools like valgrind on ld ... > Does a single > bfd_cache_close_all at the start of plugin_call_cleanup fix your > problem? Yes, it does. Regards, Kai ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch ld]: Close BFDs before linker-plugin's atexit routine is called 2011-02-14 6:45 ` Kai Tietz @ 2011-02-14 9:22 ` Kai Tietz 2011-02-14 9:51 ` Alan Modra 0 siblings, 1 reply; 7+ messages in thread From: Kai Tietz @ 2011-02-14 9:22 UTC (permalink / raw) To: Kai Tietz, Binutils, Nick Clifton, Alan Modra [-- Attachment #1: Type: text/plain, Size: 1861 bytes --] 2011/2/14 Kai Tietz <ktietz70@googlemail.com>: > 2011/2/14 Alan Modra <amodra@gmail.com>: >> On Wed, Feb 09, 2011 at 07:51:58PM +0100, Kai Tietz wrote: >>> Hello, >>> >>> This is patch addresses the unlink call when lto linker-plugin is >>> used. As windows >>> native doesn't support to unlink still opened files, it fails to do so >>> as file-descriptors >>> of bfds aren't closed before atexit routine of plugin gets called. >>> >>> 2011-02-09 Kai Tietz >>> >>> * ldmain.c (remove_output): Set output_bfd >>> of link_info to nil and close all cached bfds. >>> (main): Close output_bfd of link_info and set >>> it to nil. Additionally close all cached bfds. > > Hi Alan, > >> The fact that you need to patch three places to fix one problem >> says to me that this isn't the best fix.. > Well, here I am not that sure about. I thought about fixing it just > within the atexit-handler of plugin, but well, next one trying to > opearate on files in a similar way, will have the same issues again. I > think it is simply bad style to exit a program without even try to > cleanup opened file descriptors and used memory. This could even help > to use tools like valgrind on ld ... > >> Does a single >> bfd_cache_close_all at the start of plugin_call_cleanup fix your >> problem? > Yes, it does. I reworked patch a bit. It takes care that in ldmain's local atexit-handler BFDs are closed, and that for case of plugin the bfds getting closed before it tries to unlink files. Changelog 2011-02-14 Kai Tietz * ldmain.c (ld_close_bfds): New. (remove_output): Call ld_close_bfds. * plugin.c (plugin_call_cleanup): Likewise. * ldmain.h (ld_close_bfds): Add prototype. Tested for x86_64-pc-linux-gnu, and x86_64-w64-mingw32. Ok for apply? Regards, Kai [-- Attachment #2: ld_bfd_close.txt --] [-- Type: text/plain, Size: 1654 bytes --] Index: src/ld/ldmain.c =================================================================== --- src.orig/ld/ldmain.c 2011-02-10 09:47:59.000000000 +0100 +++ src/ld/ldmain.c 2011-02-14 10:13:39.624526400 +0100 @@ -173,13 +173,22 @@ static struct bfd_link_callbacks link_ca struct bfd_link_info link_info; \f +void +ld_close_bfds (void) +{ + if (link_info.output_bfd) + bfd_cache_close (link_info.output_bfd); + link_info.output_bfd = NULL; + bfd_cache_close_all (); +} + static void remove_output (void) { + ld_close_bfds (); + if (output_filename) { - if (link_info.output_bfd) - bfd_cache_close (link_info.output_bfd); if (delete_output_file_on_failure) unlink_if_ordinary (output_filename); } Index: src/ld/ldmain.h =================================================================== --- src.orig/ld/ldmain.h 2009-12-14 10:33:59.000000000 +0100 +++ src/ld/ldmain.h 2011-02-14 10:14:04.499526400 +0100 @@ -45,5 +45,6 @@ extern int overflow_cutoff_limit; extern void add_ysym (const char *); extern void add_wrap (const char *); extern void add_keepsyms_file (const char *); +extern void ld_close_bfds (void); #endif Index: src/ld/plugin.c =================================================================== --- src.orig/ld/plugin.c 2011-02-10 09:47:59.000000000 +0100 +++ src/ld/plugin.c 2011-02-14 10:15:38.608901400 +0100 @@ -824,6 +824,10 @@ static void plugin_call_cleanup (void) { plugin_t *curplug = plugins_list; + + /* Close all BFDs, so that unlink can operate on all targets. */ + ld_close_bfds (); + while (curplug) { if (curplug->cleanup_handler && !curplug->cleanup_done) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch ld]: Close BFDs before linker-plugin's atexit routine is called 2011-02-14 9:22 ` Kai Tietz @ 2011-02-14 9:51 ` Alan Modra 2011-02-14 10:01 ` Kai Tietz 0 siblings, 1 reply; 7+ messages in thread From: Alan Modra @ 2011-02-14 9:51 UTC (permalink / raw) To: Kai Tietz; +Cc: Binutils, Nick Clifton On Mon, Feb 14, 2011 at 10:21:58AM +0100, Kai Tietz wrote: > 2011/2/14 Kai Tietz <ktietz70@googlemail.com>: > > 2011/2/14 Alan Modra <amodra@gmail.com>: > >> On Wed, Feb 09, 2011 at 07:51:58PM +0100, Kai Tietz wrote: > >>> Hello, > >>> > >>> This is patch addresses the unlink call when lto linker-plugin is > >>> used.  As windows > >>> native doesn't support to unlink still opened files, it fails to do so > >>> as file-descriptors > >>> of bfds aren't closed before atexit routine of plugin gets called. > >>> > >>> 2011-02-09  Kai Tietz > >>> > >>>    * ldmain.c (remove_output): Set output_bfd > >>>    of link_info to nil and close all cached bfds. > >>>    (main): Close output_bfd of link_info and set > >>>    it to nil. Additionally close all cached bfds. > > > > Hi Alan, > > > >> The fact that you need to patch three places to fix one problem > >> says to me that this isn't the best fix.. > > Well, here I am not that sure about. I thought about fixing it just > > within the atexit-handler of plugin, but well, next one trying to > > opearate on files in a similar way, will have the same issues again. I > > think it is simply bad style to exit a program without even try to > > cleanup  opened file descriptors and used memory. This could even help > > to use tools like valgrind on ld ... Yes, it's true that closing the files really belongs in ld itself. > >>  Does a single > >> bfd_cache_close_all at the start of plugin_call_cleanup fix your > >> problem? > > Yes, it does. > > I reworked patch a bit. It takes care that in ldmain's local > atexit-handler BFDs are closed, and that for case of plugin the bfds > getting closed before it tries to unlink > files. Better, but this is what I'm about to commit. * ldmain.c (remove_output): Rename to.. (ld_cleanup): ..this. Call bfd_cache_close_all and plugin_call_cleanup. (main): Adjust. * plugin.c (plugin_call_cleanup): Make global. (plugin_load_plugins): Don't register plugin_call_cleanup with xatexit. * plugin.h (plugin_call_cleanup): Declare. Index: ld/ldmain.c =================================================================== RCS file: /cvs/src/src/ld/ldmain.c,v retrieving revision 1.148 diff -u -p -r1.148 ldmain.c --- ld/ldmain.c 14 Jan 2011 12:37:17 -0000 1.148 +++ ld/ldmain.c 14 Feb 2011 07:57:25 -0000 @@ -174,15 +174,14 @@ static struct bfd_link_callbacks link_ca struct bfd_link_info link_info; \f static void -remove_output (void) +ld_cleanup (void) { - if (output_filename) - { - if (link_info.output_bfd) - bfd_cache_close (link_info.output_bfd); - if (delete_output_file_on_failure) - unlink_if_ordinary (output_filename); - } + bfd_cache_close_all (); +#ifdef ENABLE_PLUGINS + plugin_call_cleanup (); +#endif + if (output_filename && delete_output_file_on_failure) + unlink_if_ordinary (output_filename); } int @@ -211,7 +210,7 @@ main (int argc, char **argv) bfd_set_error_program_name (program_name); - xatexit (remove_output); + xatexit (ld_cleanup); /* Set up the sysroot directory. */ ld_sysroot = get_sysroot (argc, argv); Index: ld/plugin.c =================================================================== RCS file: /cvs/src/src/ld/plugin.c,v retrieving revision 1.22 diff -u -p -r1.22 plugin.c --- ld/plugin.c 22 Jan 2011 10:16:29 -0000 1.22 +++ ld/plugin.c 14 Feb 2011 07:57:26 -0000 @@ -108,9 +108,6 @@ static bfd_boolean no_more_claiming = FA TRUE is returned from the hook. */ static bfd_boolean plugin_cached_allow_multiple_defs = FALSE; -/* Call 'cleanup' hook for all plugins at exit. */ -static void plugin_call_cleanup (void); - /* List of tags to set in the constant leading part of the tv array. */ static const enum ld_plugin_tag tv_header_tags[] = { @@ -721,8 +723,6 @@ plugin_load_plugins (void) if (!curplug) return 0; - xatexit (plugin_call_cleanup); - /* First pass over plugins to find max # args needed so that we can size and allocate the tv array. */ while (curplug) @@ -820,7 +820,7 @@ plugin_call_all_symbols_read (void) } /* Call 'cleanup' hook for all plugins at exit. */ -static void +void plugin_call_cleanup (void) { plugin_t *curplug = plugins_list; Index: ld/plugin.h =================================================================== RCS file: /cvs/src/src/ld/plugin.h,v retrieving revision 1.5 diff -u -p -r1.5 plugin.h --- ld/plugin.h 6 Dec 2010 12:44:51 -0000 1.5 +++ ld/plugin.h 14 Feb 2011 07:57:26 -0000 @@ -50,6 +50,9 @@ extern int plugin_call_claim_file (const /* Call 'all symbols read' hook for all plugins. */ extern int plugin_call_all_symbols_read (void); +/* Call 'cleanup' hook for all plugins at exit. */ +extern void plugin_call_cleanup (void); + /* Generate a dummy BFD to represent an IR file, for any callers of plugin_call_claim_file to use as the handle in the ld_plugin_input_file struct that they build to pass in. The BFD is initially writable, so -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch ld]: Close BFDs before linker-plugin's atexit routine is called 2011-02-14 9:51 ` Alan Modra @ 2011-02-14 10:01 ` Kai Tietz 0 siblings, 0 replies; 7+ messages in thread From: Kai Tietz @ 2011-02-14 10:01 UTC (permalink / raw) To: Kai Tietz, Binutils, Nick Clifton 2011/2/14 Alan Modra <amodra@gmail.com>: > On Mon, Feb 14, 2011 at 10:21:58AM +0100, Kai Tietz wrote: >> 2011/2/14 Kai Tietz <ktietz70@googlemail.com>: >> > 2011/2/14 Alan Modra <amodra@gmail.com>: >> >> On Wed, Feb 09, 2011 at 07:51:58PM +0100, Kai Tietz wrote: >> >>> Hello, >> >>> >> >>> This is patch addresses the unlink call when lto linker-plugin is >> >>> used. As windows >> >>> native doesn't support to unlink still opened files, it fails to do so >> >>> as file-descriptors >> >>> of bfds aren't closed before atexit routine of plugin gets called. >> >>> >> >>> 2011-02-09 Kai Tietz >> >>> >> >>> * ldmain.c (remove_output): Set output_bfd >> >>> of link_info to nil and close all cached bfds. >> >>> (main): Close output_bfd of link_info and set >> >>> it to nil. Additionally close all cached bfds. >> > >> > Hi Alan, >> > >> >> The fact that you need to patch three places to fix one problem >> >> says to me that this isn't the best fix.. >> > Well, here I am not that sure about. I thought about fixing it just >> > within the atexit-handler of plugin, but well, next one trying to >> > opearate on files in a similar way, will have the same issues again. I >> > think it is simply bad style to exit a program without even try to >> > cleanup opened file descriptors and used memory. This could even help >> > to use tools like valgrind on ld ... > > Yes, it's true that closing the files really belongs in ld itself. > >> >> Does a single >> >> bfd_cache_close_all at the start of plugin_call_cleanup fix your >> >> problem? >> > Yes, it does. >> >> I reworked patch a bit. It takes care that in ldmain's local >> atexit-handler BFDs are closed, and that for case of plugin the bfds >> getting closed before it tries to unlink >> files. > > Better, but this is what I'm about to commit. > > * ldmain.c (remove_output): Rename to.. > (ld_cleanup): ..this. Call bfd_cache_close_all and plugin_call_cleanup. > (main): Adjust. > * plugin.c (plugin_call_cleanup): Make global. > (plugin_load_plugins): Don't register plugin_call_cleanup with xatexit. > * plugin.h (plugin_call_cleanup): Declare. > > Index: ld/ldmain.c > =================================================================== > RCS file: /cvs/src/src/ld/ldmain.c,v > retrieving revision 1.148 > diff -u -p -r1.148 ldmain.c > --- ld/ldmain.c 14 Jan 2011 12:37:17 -0000 1.148 > +++ ld/ldmain.c 14 Feb 2011 07:57:25 -0000 > @@ -174,15 +174,14 @@ static struct bfd_link_callbacks link_ca > struct bfd_link_info link_info; > > static void > -remove_output (void) > +ld_cleanup (void) > { > - if (output_filename) > - { > - if (link_info.output_bfd) > - bfd_cache_close (link_info.output_bfd); > - if (delete_output_file_on_failure) > - unlink_if_ordinary (output_filename); > - } > + bfd_cache_close_all (); > +#ifdef ENABLE_PLUGINS > + plugin_call_cleanup (); > +#endif > + if (output_filename && delete_output_file_on_failure) > + unlink_if_ordinary (output_filename); > } > > int > @@ -211,7 +210,7 @@ main (int argc, char **argv) > > bfd_set_error_program_name (program_name); > > - xatexit (remove_output); > + xatexit (ld_cleanup); > > /* Set up the sysroot directory. */ > ld_sysroot = get_sysroot (argc, argv); > Index: ld/plugin.c > =================================================================== > RCS file: /cvs/src/src/ld/plugin.c,v > retrieving revision 1.22 > diff -u -p -r1.22 plugin.c > --- ld/plugin.c 22 Jan 2011 10:16:29 -0000 1.22 > +++ ld/plugin.c 14 Feb 2011 07:57:26 -0000 > @@ -108,9 +108,6 @@ static bfd_boolean no_more_claiming = FA > TRUE is returned from the hook. */ > static bfd_boolean plugin_cached_allow_multiple_defs = FALSE; > > -/* Call 'cleanup' hook for all plugins at exit. */ > -static void plugin_call_cleanup (void); > - > /* List of tags to set in the constant leading part of the tv array. */ > static const enum ld_plugin_tag tv_header_tags[] = > { > @@ -721,8 +723,6 @@ plugin_load_plugins (void) > if (!curplug) > return 0; > > - xatexit (plugin_call_cleanup); > - > /* First pass over plugins to find max # args needed so that we > can size and allocate the tv array. */ > while (curplug) > @@ -820,7 +820,7 @@ plugin_call_all_symbols_read (void) > } > > /* Call 'cleanup' hook for all plugins at exit. */ > -static void > +void > plugin_call_cleanup (void) > { > plugin_t *curplug = plugins_list; > Index: ld/plugin.h > =================================================================== > RCS file: /cvs/src/src/ld/plugin.h,v > retrieving revision 1.5 > diff -u -p -r1.5 plugin.h > --- ld/plugin.h 6 Dec 2010 12:44:51 -0000 1.5 > +++ ld/plugin.h 14 Feb 2011 07:57:26 -0000 > @@ -50,6 +50,9 @@ extern int plugin_call_claim_file (const > /* Call 'all symbols read' hook for all plugins. */ > extern int plugin_call_all_symbols_read (void); > > +/* Call 'cleanup' hook for all plugins at exit. */ > +extern void plugin_call_cleanup (void); > + > /* Generate a dummy BFD to represent an IR file, for any callers of > plugin_call_claim_file to use as the handle in the ld_plugin_input_file > struct that they build to pass in. The BFD is initially writable, so > > -- > Alan Modra > Australia Development Lab, IBM > Yes, looks good and passes my tests. Thanks, Kai ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-02-14 10:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-02-09 18:52 [patch ld]: Close BFDs before linker-plugin's atexit routine is called Kai Tietz 2011-02-13 10:40 ` Kai Tietz 2011-02-13 23:25 ` Alan Modra 2011-02-14 6:45 ` Kai Tietz 2011-02-14 9:22 ` Kai Tietz 2011-02-14 9:51 ` Alan Modra 2011-02-14 10:01 ` Kai Tietz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).