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.
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 */ {
...
};
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 ;)
Richard.
>
> Segher
