Function signatures in extern "C".

Function signatures in extern "C".

[Iain Sandoe]

Hi

g++.dg/abi/guard3.C

has:

extern "C" int __cxa_guard_acquire();

Which might not be a suitable declaration, depending on how the ‘extern  
“C”’ is supposed to affect the function signature generated.

IF, the extern C should make this parse as a “K&R” style function - then  
the TYPE_ARG_TYPES should be NULL (and the testcase is OK).

However, we are parsing the decl as int __cxa_guard_acquire(void) (i.e. C++  
rules on the empty parens), which makes the testcase not OK.

This means that the declaration is now misleading (and it’s just luck that  
expand_call happens to count the length of the TYPE_ARG_TYPES  list without  
looking to see what the types are) - in this case it happens to work out  
from this luck - since there’s only one arg so the length of the void args  
list agrees with what we want.

——

So .. the question is “which is wrong, the test-case or the assignment of  
the TYPE_ARG_TYPES”?

[we can’t easily diagnose this at this point, but I do have a patch to  
diagnose the case where we pass a void-list to expand_call and then try to  
expand a call to the callee with an inappropriate set of parms]

(it’s trivial to fix the test-case  as extern "C" int  
__cxa_guard_acquire(__UINT64_TYPE__ *);, I guess)

thanks
Iain

Re: Function signatures in extern "C".

[Jonathan Wakely]

On Sun, 6 Sep 2020 at 16:23, Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> Hi
>
> g++.dg/abi/guard3.C
>
> has:
>
> extern "C" int __cxa_guard_acquire();
>
> Which might not be a suitable declaration, depending on how the ‘extern
> “C”’ is supposed to affect the function signature generated.
>
> IF, the extern C should make this parse as a “K&R” style function - then
> the TYPE_ARG_TYPES should be NULL (and the testcase is OK).
>
> However, we are parsing the decl as int __cxa_guard_acquire(void) (i.e. C++
> rules on the empty parens), which makes the testcase not OK.

That is the correct parse. Using extern "C" doesn't mean the code is
C, it only affects mangling. It still has to follow C++ rules.

In practice you can still link to the definition, because its name is
just "__cxa_guard_acquire" irrespective of what parameter list is
present in the declaration.


> This means that the declaration is now misleading (and it’s just luck that
> expand_call happens to count the length of the TYPE_ARG_TYPES  list without
> looking to see what the types are) - in this case it happens to work out
> from this luck - since there’s only one arg so the length of the void args
> list agrees with what we want.
>
> ——
>
> So .. the question is “which is wrong, the test-case or the assignment of
> the TYPE_ARG_TYPES”?
>
> [we can’t easily diagnose this at this point, but I do have a patch to
> diagnose the case where we pass a void-list to expand_call and then try to
> expand a call to the callee with an inappropriate set of parms]
>
> (it’s trivial to fix the test-case  as extern "C" int
> __cxa_guard_acquire(__UINT64_TYPE__ *);, I guess)

But PR 45603 is ice-on-invalid triggered by the incorrect declaration
of __cxa_guard_acquire. So the incorrect declaration is what
originally reproduced the bug, and "fixing" it would make the test
useless. It's probably worth adding a comment about that in the test.

Maybe the test should give a compile-time error and XFAIL, but fixing
the declaration doesn't seem right.

Re: Function signatures in extern "C".

[Iain Sandoe]

Jonathan Wakely via Gcc <gcc@gcc.gnu.org> wrote:

> On Sun, 6 Sep 2020 at 16:23, Iain Sandoe <iain@sandoe.co.uk> wrote:

>> g++.dg/abi/guard3.C
>>
>> has:
>>
>> extern "C" int __cxa_guard_acquire();
>>
>> Which might not be a suitable declaration, depending on how the ‘extern
>> “C”’ is supposed to affect the function signature generated.
>>
>> IF, the extern C should make this parse as a “K&R” style function - then
>> the TYPE_ARG_TYPES should be NULL (and the testcase is OK).
>>
>> However, we are parsing the decl as int __cxa_guard_acquire(void) (i.e.  
>> C++
>> rules on the empty parens), which makes the testcase not OK.
>
> That is the correct parse. Using extern "C" doesn't mean the code is
> C, it only affects mangling. It still has to follow C++ rules.
>
> In practice you can still link to the definition, because its name is
> just "__cxa_guard_acquire" irrespective of what parameter list is
> present in the declaration.

Linking isn’t the problem in this case.

The problem is that we arrive at “expand_call” with a function decl that
says  f(void) .. and a call parmeter list containing a pointer type.

We happily pass the pointer in the place of the ‘void’ - because the code
only counts the number of entries and there’s one - so it happens to work.

.. that’s not true in the general case and for all calling conventions.

(this is what I mean by it happens to work by luck below).

>> This means that the declaration is now misleading (and it’s just luck that
>> expand_call happens to count the length of the TYPE_ARG_TYPES  list  
>> without
>> looking to see what the types are) - in this case it happens to work out
>> from this luck - since there’s only one arg so the length of the void args
>> list agrees with what we want.
>>
>> ——
>>
>> So .. the question is “which is wrong, the test-case or the assignment of
>> the TYPE_ARG_TYPES”?
>>
>> [we can’t easily diagnose this at this point, but I do have a patch to
>> diagnose the case where we pass a void-list to expand_call and then try to
>> expand a call to the callee with an inappropriate set of parms]
>>
>> (it’s trivial to fix the test-case  as extern "C" int
>> __cxa_guard_acquire(__UINT64_TYPE__ *);, I guess)
>
> But PR 45603 is ice-on-invalid triggered by the incorrect declaration
> of __cxa_guard_acquire. So the incorrect declaration is what
> originally reproduced the bug, and "fixing" it would make the test
> useless.

Ah OK.

> It's probably worth adding a comment about that in the test.

Yes - that would help (will add it to my TODO).
>
> Maybe the test should give a compile-time error and XFAIL, but fixing
> the declaration doesn't seem right.

I guess (because the code is invalid) there’s not much motivation to make it
more robust - e.g. diagnose the mismatch in the call(s) synthesized to
__cxa_guard_acquire.

It seems we only try to build these function decl(s) once - lazily - so  
that a
wrong one will persist for the whole TU (and we don’t seem to check that
the decl matches the itanium ABI - perhaps that’s intentional tho).

cheers
Iain

Re: Function signatures in extern "C".

[Nathan Sidwell]

GCC has an extension on machaines with cxx_implicit_extern_c (what used to be 
!NO_IMPLICIT_EXTERN_C).

On such targets we'll treat 'extern "C" void Foo ()' as-if the argument list is 
variadic.  (or something approximating that)

perhaps that is confusing things?

nathan

On 9/6/20 4:43 PM, Iain Sandoe wrote:
> Jonathan Wakely via Gcc <gcc@gcc.gnu.org> wrote:
> 
>> On Sun, 6 Sep 2020 at 16:23, Iain Sandoe <iain@sandoe.co.uk> wrote:
> 
>>> g++.dg/abi/guard3.C
>>>
>>> has:
>>>
>>> extern "C" int __cxa_guard_acquire();
>>>
>>> Which might not be a suitable declaration, depending on how the ‘extern
>>> “C”’ is supposed to affect the function signature generated.
>>>
>>> IF, the extern C should make this parse as a “K&R” style function - then
>>> the TYPE_ARG_TYPES should be NULL (and the testcase is OK).
>>>
>>> However, we are parsing the decl as int __cxa_guard_acquire(void) (i.e. C++
>>> rules on the empty parens), which makes the testcase not OK.
>>
>> That is the correct parse. Using extern "C" doesn't mean the code is
>> C, it only affects mangling. It still has to follow C++ rules.
>>
>> In practice you can still link to the definition, because its name is
>> just "__cxa_guard_acquire" irrespective of what parameter list is
>> present in the declaration.
> 
> Linking isn’t the problem in this case.
> 
> The problem is that we arrive at “expand_call” with a function decl that
> says  f(void) .. and a call parmeter list containing a pointer type.
> 
> We happily pass the pointer in the place of the ‘void’ - because the code
> only counts the number of entries and there’s one - so it happens to work.
> 
> .. that’s not true in the general case and for all calling conventions.
> 
> (this is what I mean by it happens to work by luck below).
> 
>>> This means that the declaration is now misleading (and it’s just luck that
>>> expand_call happens to count the length of the TYPE_ARG_TYPES  list without
>>> looking to see what the types are) - in this case it happens to work out
>>> from this luck - since there’s only one arg so the length of the void args
>>> list agrees with what we want.
>>>
>>> ——
>>>
>>> So .. the question is “which is wrong, the test-case or the assignment of
>>> the TYPE_ARG_TYPES”?
>>>
>>> [we can’t easily diagnose this at this point, but I do have a patch to
>>> diagnose the case where we pass a void-list to expand_call and then try to
>>> expand a call to the callee with an inappropriate set of parms]
>>>
>>> (it’s trivial to fix the test-case  as extern "C" int
>>> __cxa_guard_acquire(__UINT64_TYPE__ *);, I guess)
>>
>> But PR 45603 is ice-on-invalid triggered by the incorrect declaration
>> of __cxa_guard_acquire. So the incorrect declaration is what
>> originally reproduced the bug, and "fixing" it would make the test
>> useless.
> 
> Ah OK.
> 
>> It's probably worth adding a comment about that in the test.
> 
> Yes - that would help (will add it to my TODO).
>>
>> Maybe the test should give a compile-time error and XFAIL, but fixing
>> the declaration doesn't seem right.
> 
> I guess (because the code is invalid) there’s not much motivation to make it
> more robust - e.g. diagnose the mismatch in the call(s) synthesized to
> __cxa_guard_acquire.
> 
> It seems we only try to build these function decl(s) once - lazily - so that a
> wrong one will persist for the whole TU (and we don’t seem to check that
> the decl matches the itanium ABI - perhaps that’s intentional tho).
> 
> cheers
> Iain
> 
> 


-- 
Nathan Sidwell

Re: Function signatures in extern "C".

[Iain Sandoe]

Nathan Sidwell <nathan@acm.org> wrote:

> GCC has an extension on machaines with cxx_implicit_extern_c (what used  
> to be !NO_IMPLICIT_EXTERN_C).
>
> On such targets we'll treat 'extern "C" void Foo ()' as-if the argument  
> list is variadic.  (or something approximating that)
>
> perhaps that is confusing things?

maybe that’s the underlying reason for failing to diagnose the wrong code.

> On 9/6/20 4:43 PM, Iain Sandoe wrote:
>> Jonathan Wakely via Gcc <gcc@gcc.gnu.org> wrote:
>>> On Sun, 6 Sep 2020 at 16:23, Iain Sandoe <iain@sandoe.co.uk> wrote:
>>>> g++.dg/abi/guard3.C
>>>>
>>>> has:
>>>>
>>>> extern "C" int __cxa_guard_acquire();
>>>>
>>>> Which might not be a suitable declaration, depending on how the ‘extern
>>>> “C”’ is supposed to affect the function signature generated.
>>>>
>>>> IF, the extern C should make this parse as a “K&R” style function - then
>>>> the TYPE_ARG_TYPES should be NULL (and the testcase is OK).
>>>>
>>>> However, we are parsing the decl as int __cxa_guard_acquire(void)  
>>>> (i.e. C++
>>>> rules on the empty parens), which makes the testcase not OK.
>>>
>>> That is the correct parse. Using extern "C" doesn't mean the code is
>>> C, it only affects mangling. It still has to follow C++ rules.
>>>
>>> In practice you can still link to the definition, because its name is
>>> just "__cxa_guard_acquire" irrespective of what parameter list is
>>> present in the declaration.

>> Linking isn’t the problem in this case.
>> The problem is that we arrive at “expand_call” with a function decl that
>> says  f(void) .. and a call parmeter list containing a pointer type.
>> We happily pass the pointer in the place of the ‘void’ - because the code
>> only counts the number of entries and there’s one - so it happens to work.
>> .. that’s not true in the general case and for all calling conventions.

that is, “expand_call” does not expect to have to handle the case that the
compiler is telling it conflicting information.  AFAICT, that’s reasonable,  
I was
unable to find a way to write normal user code [at least, C-family] that the
compiler would accept producing this set of conditions (it seems that cases
in this category have to be generated by the compiler internally).

>>> But PR 45603 is ice-on-invalid triggered by the incorrect declaration
>>> of __cxa_guard_acquire. So the incorrect declaration is what
>>> originally reproduced the bug, and "fixing" it would make the test
>>> useless.
>> Ah OK.

So, IIUC we’ve replaced an ICE-on-invalid with an “accepts invalid”, it  
seems?

>>> It's probably worth adding a comment about that in the test.
>> Yes - that would help (will add it to my TODO).

Perhaps the PR should be reopened with “accepts invalid”?

thanks
Iain

Re: Function signatures in extern "C".

[Jonathan Wakely]

On Mon, 7 Sep 2020 at 09:18, Iain Sandoe wrote:
>
> Perhaps the PR should be reopened with “accepts invalid”?

My impression from the PR is that the reporter was using a different
ABI, where the name isn't reserved. Maybe the testcase should only be
accepted with -fno-threadsafe-statics or -ffreestanding or something
to say "I'm doing things differently".

Or we could just say that G++ reserves the Itanium ABI names
unconditionally, even if it doesn't need to use them, in which case it
would be accepts-invalid.

Re: Function signatures in extern "C".

[Jakub Jelinek]

On Mon, Sep 07, 2020 at 10:27:13AM +0100, Jonathan Wakely via Gcc wrote:
> On Mon, 7 Sep 2020 at 09:18, Iain Sandoe wrote:
> >
> > Perhaps the PR should be reopened with “accepts invalid”?
> 
> My impression from the PR is that the reporter was using a different
> ABI, where the name isn't reserved. Maybe the testcase should only be
> accepted with -fno-threadsafe-statics or -ffreestanding or something
> to say "I'm doing things differently".
> 
> Or we could just say that G++ reserves the Itanium ABI names
> unconditionally, even if it doesn't need to use them, in which case it
> would be accepts-invalid.

All identifiers starting with two underscores are reserved for the
implementation already.

	Jakub

Re: Function signatures in extern "C".

[Iain Sandoe]

Jonathan Wakely via Gcc <gcc@gcc.gnu.org> wrote:

> On Mon, 7 Sep 2020 at 09:18, Iain Sandoe wrote:
>> Perhaps the PR should be reopened with “accepts invalid”?
>
> My impression from the PR is that the reporter was using a different
> ABI, where the name isn't reserved. Maybe the testcase should only be
> accepted with -fno-threadsafe-statics or -ffreestanding or something
> to say "I'm doing things differently".
>
> Or we could just say that G++ reserves the Itanium ABI names
> unconditionally, even if it doesn't need to use them, in which case it
> would be accepts-invalid.

Well, it’s a name in the implementation reserved namespace.

The majority of GCC platforms executing this test will cause the compiler
to generate a call to the function (and that call will have mismatched
params).  Not sure how many non-itanium ABI platforms we have at
present.

We say nothing for "-Wall -Wextra -pedantic"

(In the end, I don’t have much of an axe to grind here - this fail came up
  when I added diagnostic code to expand_call to catch cases like this
  emitted accidentally by the Fortran FE).

it seemed worth commenting at least.

cheers
Iain

Re: Function signatures in extern "C".

[Jonathan Wakely]

On Mon, 7 Sep 2020, 10:34 Jakub Jelinek, <jakub@redhat.com> wrote:

> On Mon, Sep 07, 2020 at 10:27:13AM +0100, Jonathan Wakely via Gcc wrote:
> > On Mon, 7 Sep 2020 at 09:18, Iain Sandoe wrote:
> > >
> > > Perhaps the PR should be reopened with “accepts invalid”?
> >
> > My impression from the PR is that the reporter was using a different
> > ABI, where the name isn't reserved. Maybe the testcase should only be
> > accepted with -fno-threadsafe-statics or -ffreestanding or something
> > to say "I'm doing things differently".
> >
> > Or we could just say that G++ reserves the Itanium ABI names
> > unconditionally, even if it doesn't need to use them, in which case it
> > would be accepts-invalid.
>
> All identifiers starting with two underscores are reserved for the
> implementation already.
>

Doh, of course. So they'd have to be using some other unsupported ABI which
uses that name for a different meaning, which seems like a badly designed
ABI given that the "__cxa_" prefix is already claimed by the Itanium ABI.

So we shouldn't ICE but any other outcome for that testcase would be ok,
including rejecting it.

Re: Function signatures in extern "C".

[Florian Weimer]

* Jakub Jelinek via Gcc:

> On Mon, Sep 07, 2020 at 10:27:13AM +0100, Jonathan Wakely via Gcc wrote:
>> On Mon, 7 Sep 2020 at 09:18, Iain Sandoe wrote:
>> >
>> > Perhaps the PR should be reopened with “accepts invalid”?
>> 
>> My impression from the PR is that the reporter was using a different
>> ABI, where the name isn't reserved. Maybe the testcase should only be
>> accepted with -fno-threadsafe-statics or -ffreestanding or something
>> to say "I'm doing things differently".
>> 
>> Or we could just say that G++ reserves the Itanium ABI names
>> unconditionally, even if it doesn't need to use them, in which case it
>> would be accepts-invalid.
>
> All identifiers starting with two underscores are reserved for the
> implementation already.

But which implementation?

__ identifiers are used heavily across the GNU project, not just in GCC
and glibc (as one would expect).  A lot of C software outside the GNU
project is similar.  I think this attempt at namespace management has
failed.

For the Itanium C++ ABI symbols, it would be useful to document which
ones can be user-defined (which can be very interesting to avoid a
dependency on libstdc++).  I do not know how much value there is in
supporting a semantically different definition, or a declaration with
different types (probably not much).

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill