Re: PING: PATCH: PR target/46770: Use .init_array/.fini_array sections

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Aug 19, 2011 at 1:17 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Sorry for the delay.
>
>> --- a/gcc/config.gcc
>> +++ b/gcc/config.gcc
>> @@ -186,6 +186,9 @@
>> ?# ?configure_default_options
>> ?# ? ? ? ? ? ? ? ? ? ?Set to an initializer for configure_default_options
>> ?# ? ? ? ? ? ? ? ? ? ?in configargs.h, based on --with-cpu et cetera.
>> +#
>> +# ?use_initfini_array ? ? ? ?If set to yes, .init_array/.fini_array sections
>> +# ? ? ? ? ? ? ? ? ? ?will be used if they work.
>>
>> ?# The following variables are used in each case-construct to build up the
>> ?# outgoing variables:
>> @@ -238,6 +241,7 @@ default_gnu_indirect_function=no
>> ?target_gtfiles=
>> ?need_64bit_hwint=
>> ?need_64bit_isa=
>> +use_initfini_array=yes
>
> What is this for, when nothing ever sets it to anything but yes?
> If the $enable_initfini_array = yes test works, then there shouldn't be
> any need to override it on a per-target basis...

Done.

>> --- /dev/null
>> +++ b/gcc/config/initfini-array.h
>> @@ -0,0 +1,44 @@
>> +/* Definitions for ELF systems with .init_array/.fini_array section
>> + ? support.
>> + ? Copyright (C) 2011
>> + ? Free Software Foundation, Inc.
>> +
>> + ? This file is part of GCC.
>> +
>> + ? GCC is free software; you can redistribute it and/or modify it
>> + ? under the terms of the GNU General Public License as published
>> + ? by the Free Software Foundation; either version 3, or (at your
>> + ? option) any later version.
>> +
>> + ? GCC is distributed in the hope that it will be useful, but WITHOUT
>> + ? ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>> + ? or FITNESS FOR A PARTICULAR PURPOSE. ?See the GNU General Public
>> + ? License for more details.
>> +
>> + ? You should have received a copy of the GNU General Public License
>> + ? along with GCC; see the file COPYING3. ?If not see
>> + ? <http://www.gnu.org/licenses/>. ?*/
>> +
>> +#define USE_INITFINI_ARRAY
>> +
>> +#undef INIT_SECTION_ASM_OP
>> +#undef FINI_SECTION_ASM_OP
>> +
>> +/* FIXME: INIT_ARRAY_SECTION_ASM_OP and FINI_ARRAY_SECTION_ASM_OP
>> + ? ? ? aren't used in any assembly codes. ?But we have to define
>> + ? ? ? them to something. ?*/
>> +#define INIT_ARRAY_SECTION_ASM_OP Something
>> +#define FINI_ARRAY_SECTION_ASM_OP Something
>
> Can't you just define it to an empty string? ?And, a couple of targets
> define INIT_ARRAY_SECTION_ASM_OP/FINI_ARRAY_SECTION_ASM_OP, you either need
> to undef it first, or define only if it wasn't defined.

Done.

>> +
>> +#ifndef TARGET_ASM_INIT_SECTIONS
>> +#define TARGET_ASM_INIT_SECTIONS default_elf_initfini_array_init_sections
>> +#endif
>> +extern void default_elf_initfini_array_init_sections (void);
>
> Why do you need this (and the default_initfini_array_init_sections () call
> in all the backends)? ?Isn't it easier to just initialize the two global
> vars only when you are actually going to use them (if they are still NULL)?

Done.

>> --- a/gcc/varasm.c
>> +++ b/gcc/varasm.c
>> @@ -7350,4 +7350,62 @@ make_debug_expr_from_rtl (const_rtx exp)
>> ? ?return dval;
>> ?}
>>
>> +static GTY(()) section *elf_init_array_section;
>> +static GTY(()) section *elf_fini_array_section;
>> +
>> +void
>> +default_elf_initfini_array_init_sections (void)
>> +{
>> + ?elf_init_array_section = get_unnamed_section (0, output_section_asm_op,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "\t.section\t.init_array");
>> + ?elf_fini_array_section = get_unnamed_section (0, output_section_asm_op,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "\t.section\t.fini_array");
>> +}
>
> Remove above function.

Done.

>> +
>> +static section *
>> +get_elf_initfini_array_priority_section (int priority,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?bool constructor_p)
>> +{
>> + ?section *sec;
>> + ?if (priority != DEFAULT_INIT_PRIORITY)
>> + ? ?{
>> + ? ? ?char buf[18];
>> + ? ? ?sprintf (buf, "%s.%.5u",
>> + ? ? ? ? ? ?constructor_p ? ".init_array" : ".fini_array",
>> + ? ? ? ? ? ?priority);
>> + ? ? ?sec = get_section (buf, SECTION_WRITE, NULL_TREE);
>> + ? ?}
>
> I'd just put here
> ? else
> ? ? {
> ? ? ? if (elf_init_array_section == NULL)
> ? ? ? ? elf_init_array_section = get_unnamed_section...
> ? ? ? if (elf_fini_array_section == NULL)
> ? ? ? ? elf_fini_array_section = get_unnamed_section...
>> + ? ?sec = constructor_p ? elf_init_array_section : elf_fini_array_section;
> ? ? }

Done.

>> +void
>> +default_initfini_array_init_sections (void)
>> +{
>> +#ifdef USE_INITFINI_ARRAY
>> + ?default_elf_initfini_array_init_sections ();
>> +#endif
>> +}
>
> And remove this (and all callers etc.).

Done.

> On which targets has it been tested? ?Would be nice to test it at least on
> targets that define their own INIT_ARRAY_SECTION_ASM_OP (pa64-hpux, arm,
> m32c, rx) and on {i?86,x86_64,ia64}-linux and some solaris target.
>

Here is the updated patch. I tested it on Linux/ia32 and Linux/x86-64.
OK for trunk?

Bootstrap on Linux/ia64 has failed for a while and I don't have other
platforms. If it can't be easily tested on other affected platforms, can
we just enable it for Linux/ia32 and Linux/x86-64 first?

Thanks.

-- 
H.J.
----
2011-08-19  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/46770
	* config.gcc (tm_file): Add initfini-array.h if
	.init_arary/.fini_array supported.

	* crtstuff.c: Don't generate .ctors nor .dtors sections if
	USE_INITFINI_ARRAY is defined.

	* varasm.c (elf_init_array_section): New.
	(elf_fini_array_section): Likewise.
	(get_elf_initfini_array_priority_section): Likewise.
	(default_elf_init_array_asm_out_constructor): Likewise.
	(default_elf_fini_array_asm_out_destructor): Likewise.

	* config/initfini-array.h: New.

Attachment: gcc-pr46770-9.patch
Description: Text document