Re: [PATCH v2][C][ADA] use function descriptors instead of trampolines in C
On Wed, 22 Aug 2018, Uecker, Martin wrote: > Am Dienstag, den 21.08.2018, 21:34 + schrieb Joseph Myers: > > On Tue, 21 Aug 2018, Uecker, Martin wrote: > > > > > > I don't see why this is target-specific (if it is, the documentation > > > > for > > > > users in invoke.texi should explain what targets it works for and what > > > > it > > > > doesn't work for) anyway. I'd expect it to be a target-independent > > > > feature with a target-independent test or tests. > > > > > > My understanding is that this is a target-independent feature which > > > has not yet been implemented for all targets. The existing > > > documentation does not reflect this. > > > > How does one tell which targets do or do not support it? > > There is a target hook > > TARGET_CUSTOM_FUNCTION_DESCRIPTORS > > But I am not sure how to get this information to the testsuite. Presumably through defining a check_* function in target-supports.exp that has a list of targets supporting the feature. Together with cross-references in all the relevant places: the hook documentation in target.def (and thus the generated file tm.texi) should say that the list in target-supports.exp needs to be kept in sync if changing what targets implement the hook, and the list in target-supports.exp should similarly have a comment referencing the hook, and the user-level documentation in invoke.texi should list the relevant architectures, again with comments pointing in both directions between it and the other places needing updating when a change is made to the set of architectures supporting the feature. Except: if the feature doesn't work on some targets, I'd expect an attempt to use -fno-trampolines on other targets to produce a sorry () message from the compiler, so it's immediately obvious to the user that the requested semantics are not being achieved. And once you've got that sorry (), it should be something the check_* function can reliably test for (try compiling a trivial file with -fno-trampolines and see if it works, via check_no_compiler_messages*), so you don't need the list of targets in target-supports.exp in that case (but the documentation would need to say something about the option giving an error on targets not supporting it). > there seems to be infrastructure to implement this. The information seems > to come from a "target_info" structure (?) but I do not see where this > is populated. target_info comes from DejaGnu. Sometimes a board file may set variables in target_info. But for anything that is an architecture property of GCC, the board file should not need to set it; GCC should know the information itself. The board file is more for describing board-specific information (e.g. memory available). -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH v2][C][ADA] use function descriptors instead of trampolines in C
Am Dienstag, den 21.08.2018, 21:34 + schrieb Joseph Myers: > On Tue, 21 Aug 2018, Uecker, Martin wrote: > > > > I don't see why this is target-specific (if it is, the documentation for > > > users in invoke.texi should explain what targets it works for and what it > > > doesn't work for) anyway. I'd expect it to be a target-independent > > > feature with a target-independent test or tests. > > > > My understanding is that this is a target-independent feature which > > has not yet been implemented for all targets. The existing > > documentation does not reflect this. > > How does one tell which targets do or do not support it? There is a target hook TARGET_CUSTOM_FUNCTION_DESCRIPTORS But I am not sure how to get this information to the testsuite. > For tests for features supported on some but not all targets, we use > effective-target keywords. Of course you need to be careful about how you > implement those keywords: you don't want the implementation of the keyword > to be essentially the same as the test being conditioned, to avoid a bug > in the implementation quietly causing the test to be disabled. But the > implementation of the keyword might e.g. have a blacklist of targets that > do not yet support the feature, with the expectation that the test should > run and pass on all other targets. gcc/testsuite/lib/target-supports.exp there seems to be infrastructure to implement this. The information seems to come from a "target_info" structure (?) but I do not see where this is populated. Best, Martin
Re: [PATCH v2][C][ADA] use function descriptors instead of trampolines in C
On Tue, 21 Aug 2018, Uecker, Martin wrote: > > I don't see why this is target-specific (if it is, the documentation for > > users in invoke.texi should explain what targets it works for and what it > > doesn't work for) anyway. I'd expect it to be a target-independent > > feature with a target-independent test or tests. > > My understanding is that this is a target-independent feature which > has not yet been implemented for all targets. The existing > documentation does not reflect this. How does one tell which targets do or do not support it? For tests for features supported on some but not all targets, we use effective-target keywords. Of course you need to be careful about how you implement those keywords: you don't want the implementation of the keyword to be essentially the same as the test being conditioned, to avoid a bug in the implementation quietly causing the test to be disabled. But the implementation of the keyword might e.g. have a blacklist of targets that do not yet support the feature, with the expectation that the test should run and pass on all other targets. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH v2][C][ADA] use function descriptors instead of trampolines in C
Am Montag, den 20.08.2018, 22:34 + schrieb Joseph Myers:
> On Mon, 20 Aug 2018, Uecker, Martin wrote:
>
> > This is a new version which adds proper changelog entries and
> > a test case (no actual code changes).
>
> Please include the overall description of a change in every version
> submitted. That is, the patch submission message should both include a
> description of the current version (as in a git-style commit message) and,
> if relevant, a description of what changed relative to the previous
> version of the patch (which would not go in the commit message).
Thank you. I will keep this in mind in the future.
> A key thing I'm not clear on is what the user-visible difference in
> compiler behavior is supposed to be with this patch. Whatever that
> user-visible difference is, I'd expect it to result in some change to the
> documentation of -ftrampolines in invoke.texi (describing the new feature,
> or changing a description of a limitation of an existing feature, or
> something like that).
The option -fno-trampolines already exists and is documented.
From the description one would think that using this option would
turn off generation of trampolines and replace them by descriptors.
This then eliminates the need for an executable stack for nested
functions.
The user visible change of my patch is that it now actually works for
the C language. So I don't think this patch needs to change the
documentation as it makes the behavior match the existing documentation.
Neverthless, I think the documentation of the existing option should
be improved to document the remaining limitations of this option
regarding languages and architectures. I am happy to add such
wording, and propose it as an independent patch.
> > +/* { dg-do run { target x86_64-*-* } } */
>
> It is always wrong for a test to use x86_64-*-* like that, because
> anything that should be tested for 64-bit code generation for an x86_64
> target should also be tested for i[34567]86-*-* -m64, and if you don't
> want to test for 32-bit code generation, you need to avoid testing for
> x86_64-*-* -m32, which that test would test for. Anything genuinely
> x86-specific should go in gcc.target/i386 and then be conditioned on
> effective-target keywords such as lp64 if necessary.
Thank you, I will fix this. In fact, it should be tested for all
targets which currently support custom function descriptors.
> I don't see why this is target-specific (if it is, the documentation for
> users in invoke.texi should explain what targets it works for and what it
> doesn't work for) anyway. I'd expect it to be a target-independent
> feature with a target-independent test or tests.
My understanding is that this is a target-independent feature which
has not yet been implemented for all targets. The existing
documentation does not reflect this.
> Once there is sufficient user-level documentation showing what the
> intended semantics are, then it may be possible to evaluate how the
> implementation achieves that.
Please refer to the existing documentation of -ftrampolines and
-fno-trampolines.
My original submission also gives some background information:
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00705.html
The original generic functionality was introduced with
this patch:
https://patchwork.ozlabs.org/patch/642247/
Best,
Martin
Re: [PATCH v2][C][ADA] use function descriptors instead of trampolines in C
On Mon, 20 Aug 2018, Uecker, Martin wrote:
> This is a new version which adds proper changelog entries and
> a test case (no actual code changes).
Please include the overall description of a change in every version
submitted. That is, the patch submission message should both include a
description of the current version (as in a git-style commit message) and,
if relevant, a description of what changed relative to the previous
version of the patch (which would not go in the commit message).
A key thing I'm not clear on is what the user-visible difference in
compiler behavior is supposed to be with this patch. Whatever that
user-visible difference is, I'd expect it to result in some change to the
documentation of -ftrampolines in invoke.texi (describing the new feature,
or changing a description of a limitation of an existing feature, or
something like that).
> +/* { dg-do run { target x86_64-*-* } } */
It is always wrong for a test to use x86_64-*-* like that, because
anything that should be tested for 64-bit code generation for an x86_64
target should also be tested for i[34567]86-*-* -m64, and if you don't
want to test for 32-bit code generation, you need to avoid testing for
x86_64-*-* -m32, which that test would test for. Anything genuinely
x86-specific should go in gcc.target/i386 and then be conditioned on
effective-target keywords such as lp64 if necessary.
I don't see why this is target-specific (if it is, the documentation for
users in invoke.texi should explain what targets it works for and what it
doesn't work for) anyway. I'd expect it to be a target-independent
feature with a target-independent test or tests.
Once there is sufficient user-level documentation showing what the
intended semantics are, then it may be possible to evaluate how the
implementation achieves that.
--
Joseph S. Myers
jos...@codesourcery.com
[PATCH v2][C][ADA] use function descriptors instead of trampolines in C
This is a new version which adds proper changelog entries and a test case (no actual code changes). Bootstrapped an regression tested on x86_64. gcc/ * common.opt (flag_trampolines): Change default. * calls.c (prepare_call_address): Remove check for flag_trampolines. Decision is now made in FEs. * tree-nested.c (convert_tramp_reference_op): Likewise. gcc/ada/ * gcc-interface/trans.c (Attribute_to_gnu): Add check for flag_trampolines. gcc/c/ * c-objc-common.h: Define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS. * c-typeck.c (function_to_pointer_conversion): If using descriptors instead of trampolines, amend function address with FUNC_ADDR_BY_DESCRIPTOR and calls with ALL_EXPR_BY_DESCRIPTOR. gcc/testsuite/ * gcc.dg/trampoline-2.c: New test. diff --git a/gcc/ChangeLog b/gcc/ChangeLog index ec1fb242c23..e5f3844e8fd 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,10 @@ +2018-08-20 Martin Uecker + + * common.opt (flag_trampolines): Change default. + * calls.c (prepare_call_address): Remove check for + flag_trampolines. Decision is now made in FEs. + * tree-nested.c (convert_tramp_reference_op): Likewise. + 2018-08-19 Uros Bizjak PR target/86994 diff --git a/gcc/ada/ChangeLog b/gcc/ada/ChangeLog index 792811fb989..d906909774b 100644 --- a/gcc/ada/ChangeLog +++ b/gcc/ada/ChangeLog @@ -1,3 +1,8 @@ +2018-08-20 Martin Uecker + + * gcc-interface/trans.c (Attribute_to_gnu): Add check for + flag_trampolines. + 2018-08-03 Pierre-Marie de Rodat Reverts diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc- interface/trans.c index 0371d00fce1..1b95f7070ac 100644 --- a/gcc/ada/gcc-interface/trans.c +++ b/gcc/ada/gcc-interface/trans.c @@ -1769,7 +1769,8 @@ Attribute_to_gnu (Node_Id gnat_node, tree *gnu_result_type_p, int attribute) if ((attribute == Attr_Access || attribute == Attr_Unrestricted_Access) && targetm.calls.custom_function_descriptors > 0 - && Can_Use_Internal_Rep (Etype (gnat_node))) + && Can_Use_Internal_Rep (Etype (gnat_node)) + && (flag_trampolines != 1)) FUNC_ADDR_BY_DESCRIPTOR (gnu_expr) = 1; /* Otherwise, we need to check that we are not violating the @@ -4341,7 +4342,8 @@ Call_to_gnu (Node_Id gnat_node, tree *gnu_result_type_p, tree gnu_target, /* If the access type doesn't require foreign-compatible representation, be prepared for descriptors. */ if (targetm.calls.custom_function_descriptors > 0 - && Can_Use_Internal_Rep (Etype (Prefix (Name (gnat_node) + && Can_Use_Internal_Rep (Etype (Prefix (Name (gnat_node + && (flag_trampolines != 1)) by_descriptor = true; } else if (Nkind (Name (gnat_node)) == N_Attribute_Reference) diff --git a/gcc/c/ChangeLog b/gcc/c/ChangeLog index 7bde11c1f19..028a7284e41 100644 --- a/gcc/c/ChangeLog +++ b/gcc/c/ChangeLog @@ -1,3 +1,10 @@ +2018-08-20 Martin Uecker + + * c-objc-common.h: Define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS. + * c-typeck.c (function_to_pointer_conversion): If using descriptors + instead of trampolines, amend function address with + FUNC_ADDR_BY_DESCRIPTOR and calls with ALL_EXPR_BY_DESCRIPTOR. + 2018-08-15 David Malcolm * c-objc-common.c: Include "gcc-rich-location.h". 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 */ diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index 726ea832ae1..a6a962d1e01 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -1912,7 +1912,13 @@ function_to_pointer_conversion (location_t loc, tree exp) if (TREE_NO_WARNING (orig_exp)) TREE_NO_WARNING (exp) = 1; - return build_unary_op (loc, ADDR_EXPR, exp, false); + tree r = build_unary_op (loc, ADDR_EXPR, exp, false); + + if ((TREE_CODE(r) == ADDR_EXPR) + && (flag_trampolines == 0)) + FUNC_ADDR_BY_DESCRIPTOR (r) = 1; + + return r; } /* Mark EXP as read, not just set, for set but not used -Wunused @@ -3135,6 +3141,11 @@ build_function_call_vec (location_t loc, vec arg_loc, else result = build_call_array_loc (loc, TREE_TYPE (fntype), function, nargs, argarray); + + if ((TREE_CODE (result) == CALL_EXPR) + && (flag_trampolines == 0)) +CALL_EXPR_BY_DESCRIPTOR (result) = 1; + /* If -Wnonnull warning has been diagnos
