This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch ld]: Close BFDs before linker-plugin's atexit routine is called


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]