RFC: Support non-standard extension (call via casted function pointer)
Hello gcc developers,
as discussed in https://ghc.haskell.org/trac/ghc/ticket/11395 (and
forwarded as PR c/69221), ghc generates non-compliant C code that is not
compiled as intended on m68k. This is because its internal Cmm (a C--
dialect) to C compiler needs to declare external functions (no varargs)
without fixing the type. It currently uses "unspecified-function-type*
function();" which is a quite good fit, because the argument list is
still left open, but it fixes the return type to some pointer type.
Before calling that function, the function address is put into a
function pointer of the correct type and invoked via that pointer. This
model currently works fine (possibly by accident) on all architectures
ghc supports except m68k.
I developed a gcc patch that does not change the code generation for
conforming programs but fixes this non-conforming use-case by always
taking the actual return type in the call expression into account, even
if the function declaration to be called is known. Please comment
whether such a patch has any chance to be applied to either gcc upstream
or gcc in Debian.
Regards,
Michael Karcher
>From 50c0af571f829e54cb3274c05d7be3da41114162 Mon Sep 17 00:00:00 2001
From: Michael Karcher
Date: Mon, 25 Jan 2016 22:07:37 +0100
Subject: [PATCH 1/1] Extend gcc on m68k to allow calling incorrectly declared
externals through a function pointer of the correct type.
There are users of gcc (one of them is ghc) that expect to be able to declare
external functions with a dummy type (ghc currently uses
"ptr-to-function fn();") and call them through a function pointer of the
actual type of this function. I reported a ticket on ghc[1] which was
forwarded as PR c/69221 and subsequently closed as INVALID. While I agree
that the use case is way off the standard, and gcc emits the deserved
warnings about incompatible types, this extension seems useful, and at
least for m68k does no harm for conforming code.
---
gcc/config/m68k/m68k.c | 40 ++
gcc/testsuite/gcc.target/m68k/cast-fptr1.c | 11
gcc/testsuite/gcc.target/m68k/cast-fptr2.c | 12 +
3 files changed, 53 insertions(+), 10 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/m68k/cast-fptr1.c
create mode 100644 gcc/testsuite/gcc.target/m68k/cast-fptr2.c
diff --git a/gcc/config/m68k/m68k.c b/gcc/config/m68k/m68k.c
index 03f474e..da7a5f0 100644
--- a/gcc/config/m68k/m68k.c
+++ b/gcc/config/m68k/m68k.c
@@ -5288,22 +5288,42 @@ m68k_function_value (const_tree valtype, const_tree func ATTRIBUTE_UNUSED)
/* If the function returns a pointer, push that into %a0. */
if (func && POINTER_TYPE_P (TREE_TYPE (TREE_TYPE (func
+ {
/* For compatibility with the large body of existing code which
does not always properly declare external functions returning
pointer types, the m68k/SVR4 convention is to copy the value
returned for pointer functions from a0 to d0 in the function
epilogue, so that callers that have neglected to properly
declare the callee can still find the correct return value in
- d0. */
-return gen_rtx_PARALLEL
- (mode,
- gen_rtvec (2,
- gen_rtx_EXPR_LIST (VOIDmode,
- gen_rtx_REG (mode, A0_REG),
- const0_rtx),
- gen_rtx_EXPR_LIST (VOIDmode,
- gen_rtx_REG (mode, D0_REG),
- const0_rtx)));
+ d0.
+ expand_call uses the first register in the PARALLEL rtx. Order
+ the registers such that expand_call uses the same register as
+ if func was not given. */
+if (POINTER_TYPE_P (valtype))
+{
+ return gen_rtx_PARALLEL
+(mode,
+ gen_rtvec (2,
+gen_rtx_EXPR_LIST (VOIDmode,
+ gen_rtx_REG (mode, A0_REG),
+ const0_rtx),
+gen_rtx_EXPR_LIST (VOIDmode,
+ gen_rtx_REG (mode, D0_REG),
+ const0_rtx)));
+}
+else
+{
+ return gen_rtx_PARALLEL
+(mode,
+ gen_rtvec (2,
+gen_rtx_EXPR_LIST (VOIDmode,
+ gen_rtx_REG (mode, D0_REG),
+ const0_rtx),
+gen_rtx_EXPR_LIST (VOIDmode,
+ gen_rtx_REG (mode, A0_REG),
+ const0_rtx)));
+}
+ }
else if (POINTER_TYPE_P (valtype))
return gen_rtx_REG (mode, A0_REG);
else
diff --git a/gcc/testsuite/gcc.target/m68k/cast-fptr1.c b/gcc/testsuite/gcc.target/m68k/cast-fptr1.c
new file mode 100644
index 000..3116167
--- /dev/null
+++ b/gcc/testsuite/gcc.target/m68k/cast-fptr1.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wno-incompatible-pointer-types" } */
+/* { dg-final { scan-assembler "cmp.l %d0,%d1" }
Re: RFC: Support non-standard extension (call via casted function pointer)
On 26.01.2016 08:25, Jeff Law wrote: > I believe if they make the return type a pointer type, then the m68k > backend ought to return the value in a0 and d0 to make broken code > like this work. It may also be the case that we're not doing this > properly for indirect calls from a quick scan of m68k_function_arg. It works properly on the callee side: A function declared to return a pointer returns in both a0 and d0. On the caller side, a the result of a function returning a pointer is expected in a0 (obviously correct for the SVR4/m68k ABI), even if a "pointer-to-function returning int" is used to call that function. (which is obviously undefined behaviour. See the linked ghc problem report for chapter and verse). My patch makes ghc retrieve the value from d0 instead of a0, if the actually returned value in the call-expression has a non-pointer type, even if the called function is known and declared as returning a pointer. Regards, Michael Karcher
Re: RFC: Support non-standard extension (call via casted function pointer)
On 26.01.2016 16:40, Richard Biener wrote: > No, the patch looks somewhat broken to me. A complete fix would replace > the target macro FUNCTION_VALUE implementation by implementing the > function_value hook instead (to also receive the function type called if no > decl is available and to be able to distinguish caller vs. callee). > > I also don't see how changing the called function code will fix anything. I definitely agree that the approach to move from the FUNCTION_VALUE macro to the function_value hook is the most clean way to do it. If there is a consensus among the gcc developers that a patch to support this non-conforming code should be applied, I would be willing to prepare a patch as suggested. The current patch does not change the called code at all. The only time at which my suggested patch changes gcc behaviour is if a function declaration is given, that declaration has a pointer type as return type, but valtype is no pointer type. This (unverified claim) can not happen in the callee, as the valtype when compiling the return statement or assembling the epilogue is taken from the function declaration. > In fact the frobbing of the return value into %d0 should only happen > if the 'outgoing' arg is true (in the hook implementation) and in the > 'outgoing' == false case simply disregard 'func' and always use > 'valtype'. This frobbing of a pointer return value in %d0 only happens in the outgoing case already now, because in the non-outgoing case, m68k_function_value is (unverified claim) only called from expand_call, which replaces the PARALLEL rtx by the first list element, i.e. %a0. (or %d0 if !POINTER_TYPE_P(valtype) after my patch). > So, hookize and change to > > if (outgoing && POINTER_TYPE_P (TREE_TYPE (TREE_TYPE (func > ... > else if (POINTER_TYPE_P (valtype)) >... > else > ... Looks good and clean to me, but I expect my patch to have the same effect. Regards, Michael Karcher
Re: RFC: Support non-standard extension (call via casted function pointer)
On 26.01.2016 21:47, Richard Biener wrote: >>> So, hookize and change to >>> >>> if (outgoing && POINTER_TYPE_P (TREE_TYPE (TREE_TYPE (func >>> ... >>> else if (POINTER_TYPE_P (valtype)) >>>... >>> else >>>... >> Looks good and clean to me, but I expect my patch to have the same >> effect. > I would still prefer the more obvious approach of using the target hook > transition. I intended to express in my last email: Me too, and I will prepare a patch with a target hook if there is consensus that this patch can be accepted. I do not want the current patch committed as-is, I just posted it to get the idea across and start discussion. Regards, Michael Karcher
Re: RFC: Support non-standard extension (call via casted function pointer)
On 27.01.2016 16:10, Thorsten Otto wrote: > > This frobbing of a pointer return value in %d0 only happens in the > > outgoing case already now, because in the non-outgoing case, > > m68k_function_value is (unverified claim) only called from expand_call, > > which replaces the PARALLEL rtx by the first list element, i.e. %a0. (or > > %d0 if !POINTER_TYPE_P(valtype) after my patch). > > Wether pointer values are returned in d0 or a0 only depend on the ABI, > not the machine type. Well, this is kind-of correct. Of course the ABI defines whether pointers are returned in d0, a0 or rax. But the ABI depends on the machine type and is implemented by the machine-specific backend. > > BTW what does ghc expect on x86-64? The ABIs on linux and e.g Win64 > are also quite different with respect to which registers are used to > pass/ parameters. The translation from Cmm to C only happens in "unregisterized" builds of ghc. In this kind of build, ghc has no specific expectations about what registers will be used for passing or returning value. The only thing ghc expects is that the code generated *by gcc* to retrieve the returned value (from a temporary memory location, an integer register, a floating-point register, an address register) for a function of type X called through a pointer-to-function-of-type-X matches the code generated *by gcc* to put that value into some registers. I trust gcc on Win64 and gcc on linux to know the specific ABIs and generate correct code. > And in the m68k-case, you will have similar problems with double > values. Depending on the ABI, they are returned in either fp0 or d0/d1. I already checked that, first experimentally, after that by reading the source code. There are no problems with returning double values, because the register picked for reading the return value from is determined by m68k_function_value using only the valtype parameter. This parameter indicates the actual return type when m68k_function_value is used while compiling the function and this parameter indicates the return type of the function-expression that acutally got called when compiling the caller. In the case of ghc, these types match, so a consistent register is returned (which depends on the ABI, though). > Conclusion: IMHO, If ghc fetches the return value from the wrong > register, then ghc is broken, not gcc. ghc does not fetch anything from any register. ghc generates C code that is known to be non-conforming, and gcc decides what register to fetch from. Regards, Michael Karcher
Re: RFC: Support non-standard extension (call via casted function pointer)
On 27.01.2016 16:40, Thorsten Otto wrote: > > We are trying to support ... a direct call to a function with a > (wrong) prototype via a function > > pointer cast to the correct type and having a matching implementation of > > that function elsewhere. > > Yes, i did understand that. But you are trying to do that by hacking > gcc's m68k backend. I still think that's the wrong place to fix. The non-support of "direct call to a function with a (wrong) prototype via a function pointer cast" on m68k is not caused by a general issue in gcc (gcc tracks the return type of the function pointer used to call the function), but because of a pecularity of the m68k backend (the backend ignores the return type of the function pointer in certain circumstances). I don't see how this can be catered for outside of the backend. The effect of the patch I posted is reducing the effect of the "return pointer in both a0 and d0" hack, which interferes with the usual decision of the return value register. We do agree that the way the patch achieves this result is ugly and a better way has been proposed. Regards, Michael Karcher
Re: RFC: Support non-standard extension (call via casted function pointer)
On 27.01.2016 11:33, Richard Biener wrote: > On Tue, Jan 26, 2016 at 9:54 PM, Michael Karcher > wrote: >> On 26.01.2016 21:47, Richard Biener wrote: >> >>> I would still prefer the more obvious approach of using the target hook >>> transition. >> I intended to express in my last email: Me too, and I will prepare a >> patch with a target hook if there is consensus that this patch can be >> accepted. I do not want the current patch committed as-is, I just posted >> it to get the idea across and start discussion. > Yes, I think such patch would be accepted. Great, that is at least one "yes". I still see a lot of doubt on adjusting the m68k backend to not disturb the call of a function declared with a wrong prototype when casted to the right type. This includes Thorsten Otto, who suggests that the m68k backend is the wrong place to patch, and Andreas Schwab (I took this discussion part off gcc@gcc.gnu.org, probably a bad idea in hindsight) who points out that ghc is lying to the compiler (I agree with that) and thus the compiler does not need to be made compatible with that lie. I don't know the process of patch acceptence in the gcc project, but one positive vote against two doubtful or negative votes is not what I would call "consensus". Regards, Michael Karcher
Re: RFC: Support non-standard extension (call via casted function pointer)
On 27.01.2016 22:49, Andrew Pinski wrote:
> On Wed, Jan 27, 2016 at 7:17 AM, Richard Biener
> wrote:
>>
>> We are trying to support
>>
>> t.c
>> ---
>> void *foo();
>>
>> int bar()
>> {
>> return ((int (*)())foo) ();
>> }
> Why can't ghc produce code like:
> int bar ()
> {
> int (*foo1)() = foo;
> asm("":"+r"(foo1));
> return foo1();
> }
Thank you for the first suggestion about what ghc can do to avoid the
problem without the need to change the internally used Cmm language (as
would be needed to avoid lying about the type of foo).
> Yes it no longer produces an direct function call but it might be
> better in the long run anyways.
I don't really care about the direct function call. Using ghc on a
target where it can't generate native machine code is doomed to be slow
anyway.
But I don't see how it "might be better in the long rung anyway". The
first thing I note is that this workaround is specific to gcc. I didn't
check the gcc internals manual, but I am unsure whether the constraint
"r" is portable to be used on function pointers on all architectures gcc
supports. Furthermore, this hides the fact that the use case is not
supported by playing games with the optimizer, whereas Jeff and Richard
try to get this use case supported.
If gcc decides that the m68k backend should not be adjusted to use the
parameter "valtype" to determine the register used by the currently
selected ABI for the return value on caller side, but keep using the
function declaration for that, I will nevertheless propose this change
to ghc.
> Thanks,
> Andrew
Regards,
Michael Karcher
Re: RFC: Support non-standard extension (call via casted function pointer)
On 28.01.2016 11:04, Andreas Schwab wrote: > John Paul Adrian Glaubitz writes: > >> [suggestion to use "void" as dummy return type] >> >> Wait. Do you think this would actually allow ghc to determine the >> return type later? If I remember correctly, ghc currently initially >> declares the function prototype with return type void*, doesn't it? > Replace a lie with a different lie. Spot the pattern? > > Andreas. I am sorry, I fail to spot any pattern. As I understood you, you are opposed to changing gcc because a program that lies to gcc fails to get the result this program expects. But I don't see any pattern in "replacing a lie with a different lie". Please be more specific in what your message should tell the recipients. In case you refer to the ppc64el issue of ghc, and try to imply that changing lies at it fits has precedence in ghc history (and thus ghc either needs to stop lying or find a new lie that just happens to work), I strongly disagree. In that case, ghc developers not "replace a lie with a different lie", but replaced a lie "(empty parameter list)" with the truth "(unknown parameter list)". Regards, Michael Karcher
