public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jeff Law <law@redhat.com>
To: "Uecker, Martin" <Martin.Uecker@med.uni-goettingen.de>,
	"msebor@gmail.com" <msebor@gmail.com>,
	"joseph@codesourcery.com" <joseph@codesourcery.com>
Cc: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
	"ebotcazou@adacore.com" <ebotcazou@adacore.com>
Subject: Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C
Date: Sun, 16 Dec 2018 16:13:00 -0000	[thread overview]
Message-ID: <af880225-7417-5204-87cd-f9707fde554e@redhat.com> (raw)
In-Reply-To: <1544967934.14155.1.camel@med.uni-goettingen.de>

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. 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.
> 
> 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.
There's certainly targets where 1 byte function alignment is important
from a code space perspective -- it's likely that function descriptors
will never be supported on those targets.

It's also important to remember that not every target which uses
function descriptors uses the LSB.  On some targets the LSB may switch
between modes (arm vs thumb for example).  So on those targets the use
of descriptors may imply an even larger minimum alignment.

Ultimately using function descriptors is an ABI breaking choice and we
might declare that function descriptors imply higher function
alignments.  The ABI implications also likely mean that function
descriptors aren't likely going to achieve widespread use.  Sigh.

jeff

  reply	other threads:[~2018-12-16 16:13 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-11 16:41 [RFC] [PATCH][C][ADA] " Uecker, Martin
2018-08-18 16:33 ` Uecker, Martin
2018-08-20 14:07   ` [PATCH v2][C][ADA] " Uecker, Martin
2018-08-20 22:35     ` Joseph Myers
2018-08-21  6:17       ` Uecker, Martin
2018-08-21 21:34         ` Joseph Myers
2018-08-22  6:09           ` Uecker, Martin
2018-08-22 15:49             ` Joseph Myers
2018-11-04 20:49               ` [PATCH v3][C][ADA] " Uecker, Martin
2018-12-03 10:29                 ` Uecker, Martin
2018-12-03 21:56                 ` Jeff Law
2018-12-12 18:12                   ` [PATCH v4][C][ADA] " Uecker, Martin
2018-12-13 23:35                     ` Jeff Law
2018-12-14 10:05                       ` Uecker, Martin
2018-12-14 23:36                         ` Jeff Law
2018-12-15  1:20                           ` Martin Sebor
2018-12-16 13:46                             ` Uecker, Martin
2018-12-16 16:13                               ` Jeff Law [this message]
2018-12-16 22:46                                 ` Uecker, Martin
2018-12-17 15:26                                   ` Szabolcs Nagy
2018-12-17 18:22                                     ` Uecker, Martin
2018-12-17 19:24                                       ` Szabolcs Nagy
2018-12-18 15:23                                         ` Paul Koning
2018-12-18 15:32                                           ` Jakub Jelinek
2018-12-18 16:03                                             ` Jeff Law
2018-12-18 16:25                                               ` Jakub Jelinek
2018-12-18 16:29                                                 ` Uecker, Martin
2018-12-18 16:33                                                   ` Uecker, Martin
2018-12-18 16:42                                                     ` Jakub Jelinek
2018-12-19 19:53                                                       ` Uecker, Martin
2018-12-19 20:08                                                         ` Jakub Jelinek
2018-12-19 21:28                                                           ` Wilco Dijkstra
2018-12-21 21:41                                                     ` Hans-Peter Nilsson
2018-12-21 22:07                                                       ` Uecker, Martin
2018-12-20 13:29                                                   ` Wilco Dijkstra
2018-12-18 16:27                                               ` Uecker, Martin
2018-12-17 17:29                                   ` Jeff Law
2018-12-17 18:07                                     ` Uecker, Martin
2018-12-17 18:41                                       ` Andreas Schwab
2018-12-21  8:03                                     ` [PATCH v5][C][ADA] " Uecker, Martin
2019-01-13 21:19                                       ` [PING] " Uecker, Martin
2019-01-14 20:16                                         ` Jeff Law
2019-06-24 21:35                                     ` [PATCH v6][C][ADA] " Uecker, Martin
2019-08-09 23:42                                       ` Jeff Law
2019-08-10 10:16                                         ` Uecker, Martin
2018-12-19 19:11                                 ` [PATCH v4][C][ADA] " Uecker, Martin
2018-12-17 17:31                               ` Martin Sebor
2018-12-17 18:09                                 ` Uecker, Martin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=af880225-7417-5204-87cd-f9707fde554e@redhat.com \
    --to=law@redhat.com \
    --cc=Martin.Uecker@med.uni-goettingen.de \
    --cc=ebotcazou@adacore.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=joseph@codesourcery.com \
    --cc=msebor@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).