On Fri, Nov 5, 2021 at 3:27 PM Martin Liška <mli...@suse.cz> wrote: > > On 10/26/21 09:45, Richard Biener wrote: > > On Mon, Oct 25, 2021 at 6:32 PM Segher Boessenkool > > <seg...@kernel.crashing.org> wrote: > >> > >> Hi! > >> > >> On Mon, Oct 25, 2021 at 03:36:25PM +0200, Martin Liška wrote: > >>> --- a/gcc/config/rs6000/rs6000-internal.h > >>> +++ b/gcc/config/rs6000/rs6000-internal.h > >>> @@ -189,4 +189,13 @@ extern bool rs6000_passes_vector; > >>> extern bool rs6000_returns_struct; > >>> extern bool cpu_builtin_p; > >>> > >>> +struct rs6000_asm_out_state : public asm_out_state > >>> +{ > >>> + /* Initialize ELF sections. */ > >>> + void init_elf_sections (); > >>> + > >>> + /* Initialize XCOFF sections. */ > >>> + void init_xcoff_sections (); > >>> +}; > >> > >> Our coding convention says to use "class", not "struct" (since this > >> isn't valid C code at all). > >> > >>> - sdata2_section > >>> + sec.sdata2 > >>> = get_unnamed_section (SECTION_WRITE, output_section_asm_op, > >>> SDATA2_SECTION_ASM_OP); > >> > >> (broken indentation) > >> > >>> +/* Implement TARGET_ASM_INIT_SECTIONS. */ > >> > >> That comment is out-of-date. > >> > >>> +static asm_out_state * > >>> +rs6000_elf_asm_init_sections (void) > >>> +{ > >>> + rs6000_asm_out_state *target_state > >>> + = new (ggc_alloc<rs6000_asm_out_state> ()) rs6000_asm_out_state (); > >> > >> Hrm, maybe we can have a macro or function that does this, ggc_new or > >> something? > >> > >>> +/* Implement TARGET_ASM_INIT_SECTIONS. */ > >>> + > >>> +static asm_out_state * > >>> +rs6000_xcoff_asm_init_sections (void) > >> > >> Here, too. Both implementations are each one of several functions that > >> together implement the target macro. > >> > >>> + /* The section that holds the DWARF2 frame unwind information, when > >>> known. > >>> + The section is set either by the target's init_sections hook or > >>> by the > >>> + first call to switch_to_eh_frame_section. */ > >>> + section *eh_frame; > >>> + > >>> + /* RS6000 sections. */ > >> > >> Nothing here? Just remove the comment header? > >> > >> The idea looks fine to me. > > > > Yeah, of course then the target hook does not need to do the allocation > > and we could simply keep the current init_sections hook but change it > > to take the asm_out_state to initialize as argument. > > Makes sense. > > > > > Note that I'd put > > > > + /* RS6000 sections. */ > > + > > + /* ELF sections. */ > > + section *toc; > > + section *sdata2; > > + > > + /* XCOFF sections. */ > > + section *read_only_data; > > + section *private_data; > > + section *tls_data; > > + section *tls_private_data; > > + section *read_only_private_data; > > > > into a union, thus > > > > union { > > struct /* RS6000 sections */ { > > /* ELF sections. */ > > section *toc; > > ... > > } rs6000; > > struct /* darwin sections */ { > > ... > > }; > > Union is a bit tricky for GGC marking script, but we can manage that. > > > > > not sure whether we need some magic GTY marking here to make > > it pick up the 'correct' set. Another alternative would be > > > > section *target[MAX_TARGET_SECTIONS]; > > > > and #defines in the targets mapping the former global variables to > > indices in that array. > > > > All of this isn't "nice C++" of course, but well ... I'm not the one > > to insist ;) > > Anyway, I took a look at targets that do call the init_sections hook and I > noticed > Darwin uses pretty many sections and comes up with an array that is defined > here: > ./gcc/config/darwin-sections.def > > I tend to creating all sections in asm_out_state with DEF_SECTION, where the > list > will be extensible with a target-specific definition list. > > What do you think Richi?
That sounds good to me as well. Richard. > > Thanks, > Martin > > > > > Richard. > > > >> > >> Segher >