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

[Richard Biener]

On Tue, Jul 21, 2020 at 3:17 PM David Edelsohn <dje.gcc@gmail.com> wrote:
>
> On Tue, Jul 21, 2020 at 4:04 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > 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...)
>
> I already fixed the issue in the rs6000 backend. I explicitly exempt
> CONSTRUCTOR, as other backends have special cases for non-DECLs.

OK.

> Let me know how you would like to proceed with this patch.

Well, either dump it (too bad) or make sure targets behave the same
before/after or reasonable in case there is an existing conflict between
the case when it gets passed a decl vs its DECL_INITIAL.

Richard.

> Thanks, David