On Tue, Jul 21, 2020 at 4:04 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Fri, Jul 10, 2020 at 4:54 PM David Edelsohn <dje....@gmail.com> wrote:
> >
> > On Fri, Jul 10, 2020 at 2:55 AM Richard Biener
> > <richard.guent...@gmail.com> wrote:
> > >
> > > On Thu, Jul 9, 2020 at 8:29 PM David Edelsohn <dje....@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.
Let me know how you would like to proceed with this patch.
Thanks, David
