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

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

Am Montag, den 20.08.2018, 22:34 +0000 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