Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

["Uecker, Martin" <Martin.Uecker@med.uni-goettingen.de>]

Am Montag, den 17.12.2018, 10:31 -0700 schrieb Martin Sebor:
> On 12/16/18 6:45 AM, Uecker, Martin wrote:
> > Am Freitag, den 14.12.2018, 18:20 -0700 schrieb Martin Sebor:
> > > On 12/14/18 4:36 PM, Jeff Law wrote:
> > > > On 12/14/18 3:05 AM, Uecker, Martin wrote:
> > > > > 
> > > > > Am Donnerstag, den 13.12.2018, 16:35 -0700 schrieb Jeff Law:
> > > > > > On 12/12/18 11:12 AM, Uecker, Martin wrote:
> > > > > 
> > > > > ...
> > > > > > > > > diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
> > > > > > > > > index 78e768c2366..ef039560eb9 100644
> > > > > > > > > --- a/gcc/c/c-objc-common.h
> > > > > > > > > +++ b/gcc/c/c-objc-common.h
> > > > > > > > > @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If
> > > > > > > > > not see
> > > > > > > > >    
> > > > > > > > >    #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
> > > > > > > > >    #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
> > > > > > > > > +
> > > > > > > > > +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
> > > > > > > > > +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
> > > > > > > > >    #endif /* GCC_C_OBJC_COMMON */
> > > > > > > > 
> > > > > > > > I wonder if we even need the lang hook anymore.  ISTM that a
> > > > > > > > front-end
> > > > > > > > that wants to use the function descriptors can just set
> > > > > > > > FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor,
> > > > > > > > else we'll
> > > > > > > > use the trampoline.  Thoughts?
> > > > > > > 
> > > > > > > The lang hook also affects the minimum alignment for function
> > > > > > > pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This
> > > > > > > does
> > > > > > > not appear to change the default alignment on any architecture, but
> > > > > > > it causes a failure in i386/gcc.target/i386/attr-aligned.c when
> > > > > > > requesting a smaller alignment which is then silently ignored.
> > > > > > 
> > > > > > Ugh.  I didn't see that.
> > > > > 
> > > > > The test is new (2019-11-29 Martin Sebor), but one could
> > > > > argue that we could simply remove this specific test as 'aligned'
> > > > > is only required to increase alignment. Martin?
> > > > 
> > > > The test is meant to test that we do the right thing consistently.  If
> > > > we're failing with your patch, then that needs to be addressed.
> > > 
> > > I haven't been paying attention here and so I don't know how the test
> > > fails after the change.  It's meant to verify that attribute aligned
> > > successfully reduces the alignment of functions that have not been
> > > previously declared with one all the way down to the supported minimum
> > > (which is 1 on i386).  I agree with Jeff that removing the test would
> > > not be right unless the failure is due to some bad assumptions on my
> > > part.  If it's the built-in that fails that could be due to a bug in
> > > it (it's very new).
> > 
> > There is a choice to be made:
> > 
> > Either we activate the lang hook for C, then the minimum alignment for
> > functions on is not 1 anymore, because we need one bit to identify the
> > descriptors.  From a correcntess point of view, this is OK as 'alignas'
> > is only required to increase alignment.
> 
> alignas doesn't apply to functions.  Changing function alignment
> is a feature unique to attribute aligned.  The attribute can be
> used to decrease the alignment of functions that have not yet
> been explicitly declared with one.  GCC has historically allowed
> this, and based on my tests, the i386 target has always allowed
> functions to be completely unaligned (either by using attribute
> aligned (1) when supported or via -Os/-falign-functions=1).
> The purpose of the test is to verify that this works.
> 
> > It is also not really regression
> > in my opinion, as it is nowhere documented that one can reduce alignment
> > to '1'. The test also has just been added a couple of days ago (if I am
> > not mistaken). For these reasons, I think it would be OK to remove the test.
> 
> This wasn't clearly documented until recently.  The test was added
> to verify that GCC behaves as intended and clarified in the manual.
> The latest manual mentions that:
> 
>    The attribute cannot be used to decrease the alignment of
>    a function previously declared with a more restrictive alignment;
>    only to increase it.  Attempts to do otherwise are diagnosed.
>    Some targets specify a minimum default alignment for functions
>    that is greater than 1.  On such targets, specifying a less
>    restrictive alignment is silently ignored.
> 
> The update to the text was discussed here:
>    https://gcc.gnu.org/ml/gcc/2018-11/msg00127.html
> 
> If the i386 target should increase the minimum supported alignment
> up from 1 under some conditions the test might need to be adjusted
> (but it should not be removed).

Thank you for clarifying. Based on the discussion I think we can't
change the minimum alignment (at least without the command-line flag).

Best,
Martin

> > 
> > The other option is to decide that having an alignment of only '1'
> > is a valuable feature and should be preserved on those platforms which
> > support it. I have no idea what this could be good for, but maybe
> > there are use cases.  In this case, it makes of course sense to keep
> > the test.  We should then remove the lang hook (as it could never be
> > activated for most languages) and instead live with the fact that
> > '-fno-trampoline' and using alignof(1) on functions are simply
> > incompatible. A warning could be added if the compiler sees
> > alignof(1) when -fno-trampoline is active.
> > 
> > 
> > I am happy with both choices.
> > 
> > 
> > Best,
> > Martin
> > 
> > 
> 
>