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,
>>> +                  "%<ifunc%> 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)
>>> +          || 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.
>
> It turns out this code was superfluous with the C++ correction
> and I was able to remove it with no impact on the tests.
>
>>
>>> +        {
>>> +          funcptr = build_pointer_type (funcptr);
>>> +
>>> +          error ("%<ifunc%> 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 (targtype)
>>> +       && !types_compatible_p (altype, targtype))))
>>
>> Similar to above, already noted.
>>
>> 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.
>
> Martin