[PATCH] middle-end: Call get_constant_section with DECL not EXP.

[Richard Biener]

On Fri, Jul 10, 2020 at 4:54 PM David Edelsohn <dje.gcc@gmail.com> wrote:
>
> On Fri, Jul 10, 2020 at 2:55 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Thu, Jul 9, 2020 at 8:29 PM David Edelsohn <dje.gcc@gmail.com> wrote:
> > >
> > > output_constant_def_contents() can call get_constant_section() with an
> > > EXP that is a CONSTRUCTOR, which is not a declaration.  This can hit
> > > asserts in GCC machinery to choose a named section for the initialization
> > > data that expects the parameters to be DECLs.
> > >
> > > get_constant_section() is a wrapper around TARGET_ASM_SELECT_SECTION,
> > > which is documented as:
> > >
> > > Return the section into which @var{exp} should be placed.  You can
> > > assume that @var{exp} is either a @code{VAR_DECL} node or a constant of
> > > some sort.
> > >
> > > A CONSTRUCTOR initializer is a "constant of some sort", but isn't
> > > consistent with DECL_SECTION_NAME, etc.
> > >
> > > This patch changes get_constant_section() and its callers to pass the
> > > DECL within the EXP instead of the EXP, and to explicitly pass the reloc
> > > information that must be determined from the EXP.
> > >
> > > Another option is to remove get_constant_section() completely, replace
> > > it with direct calls to targetm.asm_out.select_section() (for which it
> > > now is a complete pass-through), and update the Target Hook
> > > documentation.
> > >
> > > What's the preferred path forward?
> >
> > Given we can always pass a decl - in the cases you change the former
> > parameter is simply DECL_INITIAL of the now passed decl - this is
> > a welcome cleanup of the TARGET_ASM_SELECT_SECTION
> > interface and it should be amended with adjusting its documentation.
> >
> > That in turn might be an opportunity to cleanup target code.
> >
> > I think we cannot rule out that targets special-case say STRING_CST
> > but not DECL_INITIAL (decl) == STRING_CST so some kind of
> > auditing is probably required.
>
> A quick check shows that target implementations believe that they
> should receive a DECL, but some are prepared for non-DECLs.
>
> rl78 explicitly handles CONSTRUCTOR.
>
> c6x explicitly handles the STRING_CST.
>
> msp430 explicitly handles both CONSTRUCTOR and TREE_CONSTANT cases.
>
> v850 explicitly handles TREE_CONSTANT.
>
> i386 explicitly handles SECCAT_RODATA_MERGE_STR.
>
> Also, categorize_decl_for_section() is used by
> default_elf_select_section() and explicitly tests
>
>   else if (TREE_CODE (decl) == STRING_CST)
>
> and
>
>   else if (TREE_CODE (decl) == CONSTRUCTOR)

Looks like nobody else is interested ... the above doesn't exactly say
whether targets checking for, say, STRING_CST also look at DECL_INITIAL
to check for a STRING_CST there so I take it as a comment indicating
that the patch may change behavior which of course wouldn't be trivially OK.

> >
> > David - I know you need this for AIX - I think the target hook implementation
> > is supposed to assume that if it doesn't get passed a decl then it's
> > a "random constant" without any special attributes attached.  What's
> > your requirement that can possibly only be fulfilled with seeing a decl?
>
> BIGGEST_ALIGNMENT on AIX is defined as 128.  The vector in the
> testcase requests an alignment of 256.  GCC generates a pseudo-op in
> the assembly for an alignment of 256, but the initializer is placed in
> a section with weaker alignment.
>
> For AIX XCOFF object files, MAXOFILE_ALIGNMENT is 32768.  I was
> attempting to have GCC generate a special, named section that
> specifies the stricter, requested alignment into which the
> appropriately aligned initizer would be placed.
>
> The named section machinery of GCC uses DECL_SECTION_NAME, which is
> unhappy when handed an EXP.
>
> I was attempting to implement something more correct for GCC
> implicitly specified or user-specified stricter alignment, but Power
> hardware doesn't require stricter alignment, so maybe it's better to
> just punt for CONSTRUCTOR in the AIX implementation.

So if you get things working without this patch (which I still like ...) then
it's going to be easier for you.  Otherwise we'd have to adjust targets
to handle the new way the same as the old (which, if there was
conflicting behavior for decl with DECL_INITIAL STRING_CST vs.
STRING_CST itself will be impossible...)

Richard.

> Thanks, David