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
