Re: correct attribute ifunc C++ type safety (PR 82301)
On 10/11/2017 05:14 AM, Nathan Sidwell wrote:
> On 10/04/2017 03:40 PM, Martin Sebor wrote:
>> On 09/28/2017 08:25 AM, Nathan Sidwell wrote:
>
>
>> Since the original tests (where the resolver returns void*) pass
>> across the board I assume it must work for all supported ABIs.
>> Or is there some subtlety between the before and after code that
>> I'm missing?
>
> I had a vague notion of some (IBM?) target that might do something
> different. Perhaps it's gone. Your comment explains it fine.
>
>>> oh, I think it's trying to spot the pointer to NON-static member
>>> function internal record type. But brokenly. I think pmf record_types
>>> have DECL_ARTIFICIAL and BUILTIN_LOCATION, that might be useful.
>>
>> It turns out this code was superfluous with the C++ correction
>> and I was able to remove it with no impact on the tests.
>
> Great.
>
>>> Is this function called before we know whether we've enabled the
>>> appropriate warnings? It would be better to bail out sooner if the
>>> warnings are disabled.
>>
>> I'm not sure I understand the question or suggestion here but
>> warnings in general are certainly enabled at this point. The
>> function issues both errors and warnings so it can't very well
>> exit early without checking the compatibility. Let me know if
>> I misunderstood your comment.
>
> Oh, forgot it issued errors too, so my queston is moot.
>
>> -signedness.
>> +signedness. In C++ where incompatible pointer conversions ordinarily
>> cause
> Missing comma: In C++, where incompatible ...
>
>> +errors, the warning detects such conversions in GCC extensions that
>> aren't
>> +part of the standard language. An example of a construct that might
>> perform
>> +such a conversion is the @code{alias} attribute. @xref{Function
>> Attributes,,Declaring Attributes of Functions}.
>
> Looks fine to me. (pedantically, I don't think I can formally approve
> this, because it's not part of the C++ FE.)
That's good enough for me :-) So I'll make it an official ACK.
jeff
Re: [PING] Re: correct attribute ifunc C++ type safety (PR 82301)
On 10/11/2017 10:32 AM, Nathan Sidwell wrote: On 10/11/2017 12:21 PM, Martin Sebor wrote: Ping? Since I submitted the updated patch it has been suggested to me that providing a new option to control the warning rather than using an existing one would be preferable (see bug 82435 for the background). The attached update to the patch adds -Wattribute-alias for this purpose and restores the original disposition for -Wincompatible-pointer-types. Makes sense to me. Did you mis my review earlier today? Thanks. For some reason I got both your replies at the same time. Since you're comfortable with the C++ aspects let me see if Joseph is willing to approve the updated patch. Martin
Re: [PING] Re: correct attribute ifunc C++ type safety (PR 82301)
On 10/11/2017 12:21 PM, Martin Sebor wrote: Ping? Since I submitted the updated patch it has been suggested to me that providing a new option to control the warning rather than using an existing one would be preferable (see bug 82435 for the background). The attached update to the patch adds -Wattribute-alias for this purpose and restores the original disposition for -Wincompatible-pointer-types. Makes sense to me. Did you mis my review earlier today? nathan -- Nathan Sidwell
[PING] Re: correct attribute ifunc C++ type safety (PR 82301)
Ping?
Since I submitted the updated patch it has been suggested to me
that providing a new option to control the warning rather than
using an existing one would be preferable (see bug 82435 for
the background). The attached update to the patch adds
-Wattribute-alias for this purpose and restores the original
disposition for -Wincompatible-pointer-types.
Thanks
Martin
On 10/04/2017 01:40 PM, Martin Sebor wrote:
On 09/28/2017 08:25 AM, Nathan Sidwell wrote:
On 09/24/2017 06:03 PM, Martin Sebor wrote:
r253041 enhanced type checking for alias and ifunc attributes to
detect declarations of incompatible aliases, or ifunc resolvers
that return pointers to functions of an incompatible type. More
extensive testing exposed a bug in the implementation of the ifunc
attribute handling in C++ where the checker expected the ifunc
resolver to return a pointer to a member function when the
implementation actually expects it return a pointer to a non-
member function.
In a discussion of the test suite failures, Jakub also suggested
to break the enhanced warning out of -Wattributes and issue it
under a different option.
The attached patch corrects the C++ problem and moves the warning
under -Wincompatible-pointer-types. Since this is a C-only option,
the patch also enables for it C++. Since the option is enabled by
default, the patch further requires -Wextra to issue the warning
for ifunc resolvers returning void*. However, the patched checker
diagnoses other incompatibilities without it.
Martin
I find the maybe_diag_incompatible_alias function confusing.
+/* Check declaration of the type of ALIAS for compatibility with its
TARGET
+ (which may be an ifunc resolver) and issue a diagnostic when they
are
+ not compatible according to language rules (plus a C++ extension for
+ non-static member functions). */
+
+static void
+maybe_diag_incompatible_alias (tree alias, tree target)
+{
+ tree altype = TREE_TYPE (alias);
+ tree targtype = TREE_TYPE (target);
+
+ bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
+ if (ifunc)
I think it might be clearer if this was broken out into a diag_ifunc
function? But see below ...
Thanks for the review. I've updated the patch to incorporate
your suggestions. My responses (mostly agreeing with your
comments or clarifying things, plus one question) are inline.
+{
+ /* Handle attribute ifunc first. */
+
+ tree funcptr = altype;
+
+ /* Set FUNCPTR to the type of the alias target. If the type
+ is a non-static member function of class C, construct a type
+ of an ordinary function taking C* as the first argument,
+ followed by the member function argument list, and use it
+ instead to check for compatibilties. FUNCPTR is used only
+ in diagnostics. */
This comment is self-contradictory.
1 Set FUNCPTR
2 Do some method-type shenanigans
3 Use it to check for incompatibilites
4 FUNCPTR is only used in diags
Which of #3 and #4 is true?
Both. It's used to control diagnostics (as opposed to something
else). But the comment is from an earlier version of the patch
where the function body was still a part of its caller, so it's
redundant now that all the code in maybe_diag_incompatible_alias
is only used to control diagnostics.
+
+ if (TREE_CODE (altype) == METHOD_TYPE)
+{
IMHO put the description of the METHOD_TYPE chicanery inside the block
doing it? FWIW, although the change being made works on many (most?)
ABIs, it's not formally correct and I think fails on some where 'this'
is passed specially. You might want to note that?
Sure. I've added a comment.
Since the original tests (where the resolver returns void*) pass
across the board I assume it must work for all supported ABIs.
Or is there some subtlety between the before and after code that
I'm missing?
+ tree rettype = TREE_TYPE (altype);
+ tree args = TYPE_ARG_TYPES (altype);
+ altype = build_function_type (rettype, args);
+ funcptr = altype;
+}
+
+ if ((!FUNC_OR_METHOD_TYPE_P (targtype)
+ || (prototype_p (altype)
+ && prototype_p (targtype)
+ && !types_compatible_p (altype, targtype
+{
+ funcptr = build_pointer_type (funcptr);
+
+ if (warning_at (DECL_SOURCE_LOCATION (target),
+ OPT_Wincompatible_pointer_types,
+ "% resolver for %qD should return %qT",
+ alias, funcptr))
+inform (DECL_SOURCE_LOCATION (alias),
+"resolver indirect function declared here");
+}
this block is almost the same as the non-ifunc block. Surely they can
be the same code? (by generalizing one of the cases until it turns into
the other?)
The existing code does that but in this patch I made the warnings
and informational notes distinct. It feels like a tossup between
parameterizing the code and making the flow more complex and harder
to follow and keeping the two
Re: correct attribute ifunc C++ type safety (PR 82301)
On 10/04/2017 03:40 PM, Martin Sebor wrote:
On 09/28/2017 08:25 AM, Nathan Sidwell wrote:
Since the original tests (where the resolver returns void*) pass
across the board I assume it must work for all supported ABIs.
Or is there some subtlety between the before and after code that
I'm missing?
I had a vague notion of some (IBM?) target that might do something
different. Perhaps it's gone. Your comment explains it fine.
oh, I think it's trying to spot the pointer to NON-static member
function internal record type. But brokenly. I think pmf record_types
have DECL_ARTIFICIAL and BUILTIN_LOCATION, that might be useful.
It turns out this code was superfluous with the C++ correction
and I was able to remove it with no impact on the tests.
Great.
Is this function called before we know whether we've enabled the
appropriate warnings? It would be better to bail out sooner if the
warnings are disabled.
I'm not sure I understand the question or suggestion here but
warnings in general are certainly enabled at this point. The
function issues both errors and warnings so it can't very well
exit early without checking the compatibility. Let me know if
I misunderstood your comment.
Oh, forgot it issued errors too, so my queston is moot.
-signedness.
+signedness. In C++ where incompatible pointer conversions ordinarily cause
Missing comma: In C++, where incompatible ...
+errors, the warning detects such conversions in GCC extensions that aren't
+part of the standard language. An example of a construct that might perform
+such a conversion is the @code{alias} attribute. @xref{Function
Attributes,,Declaring Attributes of Functions}.
Looks fine to me. (pedantically, I don't think I can formally approve
this, because it's not part of the C++ FE.)
nathan
--
Nathan Sidwell
Re: correct attribute ifunc C++ type safety (PR 82301)
On 09/28/2017 08:25 AM, Nathan Sidwell wrote:
On 09/24/2017 06:03 PM, Martin Sebor wrote:
r253041 enhanced type checking for alias and ifunc attributes to
detect declarations of incompatible aliases, or ifunc resolvers
that return pointers to functions of an incompatible type. More
extensive testing exposed a bug in the implementation of the ifunc
attribute handling in C++ where the checker expected the ifunc
resolver to return a pointer to a member function when the
implementation actually expects it return a pointer to a non-
member function.
In a discussion of the test suite failures, Jakub also suggested
to break the enhanced warning out of -Wattributes and issue it
under a different option.
The attached patch corrects the C++ problem and moves the warning
under -Wincompatible-pointer-types. Since this is a C-only option,
the patch also enables for it C++. Since the option is enabled by
default, the patch further requires -Wextra to issue the warning
for ifunc resolvers returning void*. However, the patched checker
diagnoses other incompatibilities without it.
Martin
I find the maybe_diag_incompatible_alias function confusing.
+/* Check declaration of the type of ALIAS for compatibility with its
TARGET
+ (which may be an ifunc resolver) and issue a diagnostic when they are
+ not compatible according to language rules (plus a C++ extension for
+ non-static member functions). */
+
+static void
+maybe_diag_incompatible_alias (tree alias, tree target)
+{
+ tree altype = TREE_TYPE (alias);
+ tree targtype = TREE_TYPE (target);
+
+ bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
+ if (ifunc)
I think it might be clearer if this was broken out into a diag_ifunc
function? But see below ...
Thanks for the review. I've updated the patch to incorporate
your suggestions. My responses (mostly agreeing with your
comments or clarifying things, plus one question) are inline.
+{
+ /* Handle attribute ifunc first. */
+
+ tree funcptr = altype;
+
+ /* Set FUNCPTR to the type of the alias target. If the type
+ is a non-static member function of class C, construct a type
+ of an ordinary function taking C* as the first argument,
+ followed by the member function argument list, and use it
+ instead to check for compatibilties. FUNCPTR is used only
+ in diagnostics. */
This comment is self-contradictory.
1 Set FUNCPTR
2 Do some method-type shenanigans
3 Use it to check for incompatibilites
4 FUNCPTR is only used in diags
Which of #3 and #4 is true?
Both. It's used to control diagnostics (as opposed to something
else). But the comment is from an earlier version of the patch
where the function body was still a part of its caller, so it's
redundant now that all the code in maybe_diag_incompatible_alias
is only used to control diagnostics.
+
+ if (TREE_CODE (altype) == METHOD_TYPE)
+{
IMHO put the description of the METHOD_TYPE chicanery inside the block
doing it? FWIW, although the change being made works on many (most?)
ABIs, it's not formally correct and I think fails on some where 'this'
is passed specially. You might want to note that?
Sure. I've added a comment.
Since the original tests (where the resolver returns void*) pass
across the board I assume it must work for all supported ABIs.
Or is there some subtlety between the before and after code that
I'm missing?
+ tree rettype = TREE_TYPE (altype);
+ tree args = TYPE_ARG_TYPES (altype);
+ altype = build_function_type (rettype, args);
+ funcptr = altype;
+}
+
+ if ((!FUNC_OR_METHOD_TYPE_P (targtype)
+ || (prototype_p (altype)
+ && prototype_p (targtype)
+ && !types_compatible_p (altype, targtype
+{
+ funcptr = build_pointer_type (funcptr);
+
+ if (warning_at (DECL_SOURCE_LOCATION (target),
+ OPT_Wincompatible_pointer_types,
+ "% resolver for %qD should return %qT",
+ alias, funcptr))
+inform (DECL_SOURCE_LOCATION (alias),
+"resolver indirect function declared here");
+}
this block is almost the same as the non-ifunc block. Surely they can
be the same code? (by generalizing one of the cases until it turns into
the other?)
The existing code does that but in this patch I made the warnings
and informational notes distinct. It feels like a tossup between
parameterizing the code and making the flow more complex and harder
to follow and keeping the two cases (ifunc and alias) separate from
one another. But I don't feel strongly one way or the other so
I changed it as you suggest.
+ /* Deal with static member function pointers. */
I do not understand this comment or condition. We seem to have dealt
with pointers already and the conditions seem confused.
+ if (TREE_CODE (targtype) != RECORD_TYPE
+ || TYPE_FIELDS (targtype)
+ ||
Re: correct attribute ifunc C++ type safety (PR 82301)
On 09/28/2017 05:23 AM, Pedro Alves wrote:
On 09/25/2017 02:03 AM, Martin Sebor wrote:
+a @option{-Wincompatible-pointer-types} warning for mismatches. To suppress
+a warning for the necessary cast from a pointer to the implementation member
+function to the type of the corresponding non-member function use the
+@option{-Wno-pmf-conversions} option. For example:
FWIW, it seems odd to me to tell users they need to suppress warnings, when
the compiler surely could provide better/safer means to avoid needing
to use the reinterpret_cast hammer. See below.
+
+@smallexample
+class S
+@{
+private:
+ int debug_impl (int);
+ int optimized_impl (int);
+
+ typedef int Func (S*, int);
+
+ static Func* resolver ();
+public:
+
+ int interface (int);
+@};
+
+int S::debug_impl (int) @{ /* @r{@dots{}} */ @}
+int S::optimized_impl (int) @{ /* @r{@dots{}} */ @}
+
+S::Func* S::resolver ()
+@{
+ int (S::*pimpl) (int)
+= getenv ("DEBUG") ? ::debug_impl : ::optimized_impl;
+
+ // Cast triggers -Wno-pmf-conversions.
+ return reinterpret_cast(pimpl);
+@}
+
If I were writing code like this, I'd write a reinterpret_cast-like
function specifically for pointer-to-member-function to free-function
casting, and only suppress the warning there instead of disabling
the warning for the whole translation unit. Something like:
#include
template struct pmf_as_func;
template
struct pmf_as_func
{
typedef Ret (func_type) (S *, Args...);
typedef S struct_type;
};
template
typename pmf_as_func::func_type *
pmf_as_func_cast (Pmf pmf)
{
static_assert (!std::is_polymorphic::struct_type>::value,
"");
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpmf-conversions"
return reinterpret_cast::func_type *> (pmf);
#pragma GCC diagnostic pop
}
and then write:
return pmf_as_func_cast (pimpl);
instead of:
return reinterpret_cast(pimpl);
The point being of course to make it harder to misuse the casts.
But that may be a bit too much for the manual.
Yes, that's a much nicer solution but as you say, more code than
the manual typically includes for warnings. (It would be a good
example for a Users Guide if GCC had one.)
It also wouldn't work as is with C++03 (because variatic templates).
Which leads me to think that if GCC guarantees this cast works, then
it'd be nice to have GCC provide it (like a __pmf_as_func_cast function)
as builtin. Then it'd work on C++03 as well, and the compiler of course
can precisely validate whether the cast is valid. (It's quite possible
that there's a better check than is_polymorphic as I've written above.)
I also don't like the cast and have been thinking about how to
suppress the warning in this case (the ifunc resolver). But
the only way I can think to make it possible is to defer
-Wno-pmf-conversions until after all function declarations in
the translation unit have been processed. In other words, run
it in a separate pass and suppress it for functions that have
been aliased via attribute ifunc. I don't know if there already
is a pass like that it could be hooked into. If there is, it
should be feasible.
Nathan or Jason, is there something like this?
Martin
Re: correct attribute ifunc C++ type safety (PR 82301)
On 09/24/2017 06:03 PM, Martin Sebor wrote:
r253041 enhanced type checking for alias and ifunc attributes to
detect declarations of incompatible aliases, or ifunc resolvers
that return pointers to functions of an incompatible type. More
extensive testing exposed a bug in the implementation of the ifunc
attribute handling in C++ where the checker expected the ifunc
resolver to return a pointer to a member function when the
implementation actually expects it return a pointer to a non-
member function.
In a discussion of the test suite failures, Jakub also suggested
to break the enhanced warning out of -Wattributes and issue it
under a different option.
The attached patch corrects the C++ problem and moves the warning
under -Wincompatible-pointer-types. Since this is a C-only option,
the patch also enables for it C++. Since the option is enabled by
default, the patch further requires -Wextra to issue the warning
for ifunc resolvers returning void*. However, the patched checker
diagnoses other incompatibilities without it.
Martin
I find the maybe_diag_incompatible_alias function confusing.
+/* Check declaration of the type of ALIAS for compatibility with its TARGET
+ (which may be an ifunc resolver) and issue a diagnostic when they are
+ not compatible according to language rules (plus a C++ extension for
+ non-static member functions). */
+
+static void
+maybe_diag_incompatible_alias (tree alias, tree target)
+{
+ tree altype = TREE_TYPE (alias);
+ tree targtype = TREE_TYPE (target);
+
+ bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
+ if (ifunc)
I think it might be clearer if this was broken out into a diag_ifunc
function? But see below ...
+{
+ /* Handle attribute ifunc first. */
+
+ tree funcptr = altype;
+
+ /* Set FUNCPTR to the type of the alias target. If the type
+is a non-static member function of class C, construct a type
+of an ordinary function taking C* as the first argument,
+followed by the member function argument list, and use it
+instead to check for compatibilties. FUNCPTR is used only
+in diagnostics. */
This comment is self-contradictory.
1 Set FUNCPTR
2 Do some method-type shenanigans
3 Use it to check for incompatibilites
4 FUNCPTR is only used in diags
Which of #3 and #4 is true?
+
+ if (TREE_CODE (altype) == METHOD_TYPE)
+ {
IMHO put the description of the METHOD_TYPE chicanery inside the block
doing it? FWIW, although the change being made works on many (most?)
ABIs, it's not formally correct and I think fails on some where 'this'
is passed specially. You might want to note that?
+ tree rettype = TREE_TYPE (altype);
+ tree args = TYPE_ARG_TYPES (altype);
+ altype = build_function_type (rettype, args);
+ funcptr = altype;
+ }
+
+ if ((!FUNC_OR_METHOD_TYPE_P (targtype)
+ || (prototype_p (altype)
+ && prototype_p (targtype)
+ && !types_compatible_p (altype, targtype
+ {
+ funcptr = build_pointer_type (funcptr);
+
+ if (warning_at (DECL_SOURCE_LOCATION (target),
+ OPT_Wincompatible_pointer_types,
+ "% resolver for %qD should return %qT",
+ alias, funcptr))
+ inform (DECL_SOURCE_LOCATION (alias),
+ "resolver indirect function declared here");
+ }
this block is almost the same as the non-ifunc block. Surely they can
be the same code? (by generalizing one of the cases until it turns into
the other?)
+ /* Deal with static member function pointers. */
I do not understand this comment or condition. We seem to have dealt
with pointers already and the conditions seem confused.
+ if (TREE_CODE (targtype) != RECORD_TYPE
+ || TYPE_FIELDS (targtype)
+ || TREE_CODE (TREE_TYPE (TYPE_FIELDS (targtype))) != POINTER_TYPE
+ || (TREE_CODE (TREE_TYPE (TREE_TYPE (TYPE_FIELDS (targtype
+ != METHOD_TYPE))
if
not a record,
or has TYPE_FIELDS non-NULL
or the first field doesn't have pointer type (we can't get here)
or something else about the first field
oh, I think it's trying to spot the pointer to NON-static member
function internal record type. But brokenly. I think pmf record_types
have DECL_ARTIFICIAL and BUILTIN_LOCATION, that might be useful.
+ {
+ funcptr = build_pointer_type (funcptr);
+
+ error ("% resolver for %qD must return %qT",
+alias, funcptr);
+
+ inform (DECL_SOURCE_LOCATION (alias),
+ "resolver indirect function declared here");
+ }
+ }
+
+ if ((!FUNC_OR_METHOD_TYPE_P (targtype)
+ || (prototype_p (altype)
+ && prototype_p
Re: correct attribute ifunc C++ type safety (PR 82301)
On 09/25/2017 02:03 AM, Martin Sebor wrote:
> +a @option{-Wincompatible-pointer-types} warning for mismatches. To suppress
> +a warning for the necessary cast from a pointer to the implementation member
> +function to the type of the corresponding non-member function use the
> +@option{-Wno-pmf-conversions} option. For example:
FWIW, it seems odd to me to tell users they need to suppress warnings, when
the compiler surely could provide better/safer means to avoid needing
to use the reinterpret_cast hammer. See below.
> +
> +@smallexample
> +class S
> +@{
> +private:
> + int debug_impl (int);
> + int optimized_impl (int);
> +
> + typedef int Func (S*, int);
> +
> + static Func* resolver ();
> +public:
> +
> + int interface (int);
> +@};
> +
> +int S::debug_impl (int) @{ /* @r{@dots{}} */ @}
> +int S::optimized_impl (int) @{ /* @r{@dots{}} */ @}
> +
> +S::Func* S::resolver ()
> +@{
> + int (S::*pimpl) (int)
> += getenv ("DEBUG") ? ::debug_impl : ::optimized_impl;
> +
> + // Cast triggers -Wno-pmf-conversions.
> + return reinterpret_cast(pimpl);
> +@}
> +
If I were writing code like this, I'd write a reinterpret_cast-like
function specifically for pointer-to-member-function to free-function
casting, and only suppress the warning there instead of disabling
the warning for the whole translation unit. Something like:
#include
template struct pmf_as_func;
template
struct pmf_as_func
{
typedef Ret (func_type) (S *, Args...);
typedef S struct_type;
};
template
typename pmf_as_func::func_type *
pmf_as_func_cast (Pmf pmf)
{
static_assert (!std::is_polymorphic::struct_type>::value,
"");
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpmf-conversions"
return reinterpret_cast::func_type *> (pmf);
#pragma GCC diagnostic pop
}
and then write:
return pmf_as_func_cast (pimpl);
instead of:
return reinterpret_cast(pimpl);
The point being of course to make it harder to misuse the casts.
But that may be a bit too much for the manual.
It also wouldn't work as is with C++03 (because variatic templates).
Which leads me to think that if GCC guarantees this cast works, then
it'd be nice to have GCC provide it (like a __pmf_as_func_cast function)
as builtin. Then it'd work on C++03 as well, and the compiler of course
can precisely validate whether the cast is valid. (It's quite possible
that there's a better check than is_polymorphic as I've written above.)
Just a passing thought.
Thanks,
Pedro Alves
Re: correct attribute ifunc C++ type safety (PR 82301)
Hi Nathan, This patch is a tweak for the C++ part of the type safety enhancement to attribute ifunc committed in r253041. It touches the C++ ifunc tests you added some years ago and I recently broke with the initial commits of the feature. The notable difference between r253041 and this update (other than correcting the type expected to be returned by the resolver for a member function) is issuing the incompatibility warning under -Wincompatible- pointer-types rather than -Wattributes. When you and/or Jason have a minute, can you please review it? https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01603.html Thanks Martin On 09/24/2017 07:03 PM, Martin Sebor wrote: r253041 enhanced type checking for alias and ifunc attributes to detect declarations of incompatible aliases, or ifunc resolvers that return pointers to functions of an incompatible type. More extensive testing exposed a bug in the implementation of the ifunc attribute handling in C++ where the checker expected the ifunc resolver to return a pointer to a member function when the implementation actually expects it return a pointer to a non- member function. In a discussion of the test suite failures, Jakub also suggested to break the enhanced warning out of -Wattributes and issue it under a different option. The attached patch corrects the C++ problem and moves the warning under -Wincompatible-pointer-types. Since this is a C-only option, the patch also enables for it C++. Since the option is enabled by default, the patch further requires -Wextra to issue the warning for ifunc resolvers returning void*. However, the patched checker diagnoses other incompatibilities without it. Martin
correct attribute ifunc C++ type safety (PR 82301)
r253041 enhanced type checking for alias and ifunc attributes to
detect declarations of incompatible aliases, or ifunc resolvers
that return pointers to functions of an incompatible type. More
extensive testing exposed a bug in the implementation of the ifunc
attribute handling in C++ where the checker expected the ifunc
resolver to return a pointer to a member function when the
implementation actually expects it return a pointer to a non-
member function.
In a discussion of the test suite failures, Jakub also suggested
to break the enhanced warning out of -Wattributes and issue it
under a different option.
The attached patch corrects the C++ problem and moves the warning
under -Wincompatible-pointer-types. Since this is a C-only option,
the patch also enables for it C++. Since the option is enabled by
default, the patch further requires -Wextra to issue the warning
for ifunc resolvers returning void*. However, the patched checker
diagnoses other incompatibilities without it.
Martin
PR c/82301 - Updated test case g++.dg/ext/attr-ifunc-1.C (and others) in r253041 segfault on powerpc64
gcc/ChangeLog:
PR other/82301
* cgraphunit.c (maybe_diag_incompatible_alias): New function.
(handle_alias_pairs): Call it.
* doc/extend.texi (ifunc attribute): Discuss C++ specifics.
* doc/invoke.texi (-Wincompatible-pointer-types): Ditto.
gcc/c-family/ChangeLog:
PR other/82301
* c.opt (-Wincompatible-pointer-types): Enable for C++.
gcc/testsuite/ChangeLog:
PR other/82301
* g++.dg/ext/attr-ifunc-1.C: Update.
* g++.dg/ext/attr-ifunc-2.C: Same.
* g++.dg/ext/attr-ifunc-3.C: Same.
* g++.dg/ext/attr-ifunc-4.C: Same.
* g++.dg/ext/attr-ifunc-5.C: Same.
* g++.dg/ext/attr-ifunc-6.C: New test.
* gcc.dg/attr-ifunc-6.c: New test.
* gcc.dg/attr-ifunc-7.c: New test.
* gcc.dg/pr81854.c: Update.
* lib/target-supports.exp: Update.
Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt (revision 253099)
+++ gcc/c-family/c.opt (working copy)
@@ -592,7 +592,7 @@ C C++ Var(warn_ignored_attributes) Init(1) Warning
Warn whenever attributes are ignored.
Wincompatible-pointer-types
-C ObjC Var(warn_incompatible_pointer_types) Init(1) Warning
+C C++ ObjC ObjcC++ Var(warn_incompatible_pointer_types) Init(1) Warning
Warn when there is a conversion between pointers that have incompatible types.
Winit-self
Index: gcc/cgraphunit.c
===
--- gcc/cgraphunit.c (revision 253099)
+++ gcc/cgraphunit.c (working copy)
@@ -1296,6 +1296,106 @@ analyze_functions (bool first_time)
input_location = saved_loc;
}
+/* Check declaration of the type of ALIAS for compatibility with its TARGET
+ (which may be an ifunc resolver) and issue a diagnostic when they are
+ not compatible according to language rules (plus a C++ extension for
+ non-static member functions). */
+
+static void
+maybe_diag_incompatible_alias (tree alias, tree target)
+{
+ tree altype = TREE_TYPE (alias);
+ tree targtype = TREE_TYPE (target);
+
+ bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
+ if (ifunc)
+{
+ /* Handle attribute ifunc first. */
+
+ tree funcptr = altype;
+
+ /* Set FUNCPTR to the type of the alias target. If the type
+ is a non-static member function of class C, construct a type
+ of an ordinary function taking C* as the first argument,
+ followed by the member function argument list, and use it
+ instead to check for compatibilties. FUNCPTR is used only
+ in diagnostics. */
+
+ if (TREE_CODE (altype) == METHOD_TYPE)
+ {
+ tree rettype = TREE_TYPE (altype);
+ tree args = TYPE_ARG_TYPES (altype);
+ altype = build_function_type (rettype, args);
+ funcptr = altype;
+ }
+
+ targtype = TREE_TYPE (targtype);
+
+ if (POINTER_TYPE_P (targtype))
+ {
+ targtype = TREE_TYPE (targtype);
+
+ /* Only issue Wincompatible-pointer-types for conversions
+ to void* with -Wextra. */
+ if (VOID_TYPE_P (targtype) && !extra_warnings)
+ return;
+
+ if ((!FUNC_OR_METHOD_TYPE_P (targtype)
+ || (prototype_p (altype)
+ && prototype_p (targtype)
+ && !types_compatible_p (altype, targtype
+ {
+ funcptr = build_pointer_type (funcptr);
+
+ if (warning_at (DECL_SOURCE_LOCATION (target),
+ OPT_Wincompatible_pointer_types,
+ "% resolver for %qD should return %qT",
+ alias, funcptr))
+ inform (DECL_SOURCE_LOCATION (alias),
+ "resolver indirect function declared here");
+ }
+ }
+ else
+ {
+ /* Deal with static member function pointers. */
+ if (TREE_CODE (targtype) != RECORD_TYPE
+ || TYPE_FIELDS (targtype)
+ || TREE_CODE (TREE_TYPE (TYPE_FIELDS (targtype))) != POINTER_TYPE
+ || (TREE_CODE (TREE_TYPE (TREE_TYPE (TYPE_FIELDS (targtype
+ != METHOD_TYPE))
+ {
+ funcptr = build_pointer_type (funcptr);
+
+ error ("% resolver for %qD
