Re: [RFC PATCH] avoid applying attributes to explicit specializations (PR 83871)

[Jason Merrill <jason@redhat.com>]

On Mon, Feb 12, 2018 at 11:59 AM, Martin Sebor <msebor@gmail.com> wrote:
> On 02/12/2018 09:30 AM, Jason Merrill wrote:
>>
>> On Fri, Feb 9, 2018 at 6:57 PM, Martin Sebor <msebor@gmail.com> wrote:
>>>
>>> On 02/09/2018 12:52 PM, Jason Merrill wrote:
>>>>
>>>> On 02/08/2018 04:52 PM, Martin Sebor wrote:
>>>>>
>>>>>
>>>>> I took me a while to find DECL_TEMPLATE_RESULT.  Hopefully
>>>>> that's the right way to get the primary from a TEMPLATE_DECL.
>>>>
>>>>
>>>> Yes.
>>>>
>>>>>>> Attached is an updated patch.  It hasn't gone through full
>>>>>>> testing yet but please let me know if you'd like me to make
>>>>>>> some changes.
>>>>>>
>>>>>>
>>>>>>
>>>>>>> +  const char* const whitelist[] = {
>>>>>>> +    "error", "noreturn", "warning"
>>>>>>> +  };
>>>>>>
>>>>>>
>>>>>>
>>>>>> Why whitelist noreturn?  I would expect to want that to be consistent.
>>>>>
>>>>>
>>>>> I expect noreturn to be used on a primary whose definition
>>>>> is provided but that's not meant to be used the way the API
>>>>> is otherwise expected to be.  As in:
>>>>>
>>>>>    template <class T>
>>>>>    T [[noreturn]] foo () { throw "not implemented"; }
>>>>>
>>>>>    template <> int foo<int>();   // implemented elsewhere
>>>>
>>>>
>>>> Marking that template as noreturn seems pointless, and possibly harmful;
>>>> the deprecated, warning, or error attributes would be better for this
>>>> situation.
>>>
>>>
>>> I meant either:
>>>
>>>   template <class T>
>>>   T __attribute__ ((noreturn)) foo () { throw "not implemented"; }
>>>
>>>   template <> int foo<int>();   // implemented elsewhere
>>>
>>> or (sigh)
>>>
>>>   template <class T>
>>>   [[noreturn]] T foo () { throw "not implemented"; }
>>>
>>>   template <> int foo<int>();   // implemented elsewhere
>>>
>>> It lets code like this
>>>
>>>   int bar ()
>>>   {
>>>      return foo<char>();
>>>   }
>>>
>>> be diagnosed because it's likely a bug (as Clang does with
>>> -Wunreachable-code).  It doesn't stop code like the following
>>> from compiling (which is good) but it instead lets them throw
>>> at runtime which is what foo's author wants.
>>>
>>>   void bar ()
>>>   {
>>>      foo<char>();
>>>   }
>>>
>>> It's the same as having an "unimplemented" base virtual function
>>> throw an exception when it's called rather than making it pure
>>> and having calls to it abort.  Declaring the base virtual function
>>> noreturn is useful for the same reason (and also diagnosed by
>>> Clang).  I should remember to add the same warning in GCC 9.
>>
>>
>> Yes, I understood the patterns you had in mind, but I disagree with
>> them.  My point about harmful is that declaring a function noreturn
>> because it's unimplemented could be a problem for when the function is
>> later implemented, and callers were optimized inappropriately.  This
>> seems like a rather roundabout way to get a warning about calling an
>> unimplemented function, and not worth overriding the normal behavior.
>
>
> Removing noreturn from the whitelist means having to prevent
> the attribute from causing conflicts with the attributes on
> the blacklist.  E.g., in this:
>
>   template <class T> [[malloc]] void* allocate (int);
>
>   template <> [[noreturn]] void* allocate<void> (int);
>
> -Wmissing-attributes would warn for the missing malloc but
> -Wattributes will warn once malloc is added.  Ditto for all
> other attributes noreturn is considered to conflict with such
> as alloc_size and warn_unused_result.

This example seems rather unlikely, and the solution is to remove
[[noreturn]].  I don't think this is worth worrying about for GCC 8.

> I anticipate the warning code to ultimately end up in
> the middle-end so it can handle Joseph's case as well, and
> so it can also be integrated with the attribute conflict
> machinery.  It also needs to be in the middle-end to become
> usable by -Wsuggest-attribute.  But I wasn't thinking of
> making any of these bigger changes until GCC 9.

> Do you want me to integrate it with the conflict stuff now?

No, leaving it for GCC 9 makes sense to me.

Jason

>>> I actually had some misgivings about both warning and deprecated
>>> for the white-listing, but not for noreturn.  My (only mild)
>>> concern is that both warning and deprecated functions can and
>>> likely will in some cases still be called, and so using them to
>>> suppress the warning runs the risk that their calls might be
>>> wrong and no one will notice.  Warning cannot be suppressed
>>> so it seems unlikely to be ignored, but deprecated can be.
>>> So I wonder if the white-listing for deprecated should be
>>> conditional on -Wdeprecated being enabled.
>>>
>>>>> -      /* Merge the type qualifiers.  */
>>>>> -      if (TREE_READONLY (newdecl))
>>>>> -    TREE_READONLY (olddecl) = 1;
>>>>>        if (TREE_THIS_VOLATILE (newdecl))
>>>>>      TREE_THIS_VOLATILE (olddecl) = 1;
>>>>> -      if (TREE_NOTHROW (newdecl))
>>>>> -    TREE_NOTHROW (olddecl) = 1;
>>>>> +
>>>>> +      if (merge_attr)
>>>>> +    {
>>>>> +      /* Merge the type qualifiers.  */
>>>>> +      if (TREE_READONLY (newdecl))
>>>>> +        TREE_READONLY (olddecl) = 1;
>>>>> +    }
>>>>> +      else
>>>>> +    {
>>>>> +      /* Set the bits that correspond to the const function
>>>>> attributes.  */
>>>>> +      TREE_READONLY (olddecl) = TREE_READONLY (newdecl);
>>>>> +    }
>>>>
>>>>
>>>>
>>>> Let's limit the const/volatile handling here to non-functions, and
>>>> handle the const/noreturn attributes for functions in the later hunk
>>>> along with nothrow/malloc/pure.
>>>
>>>
>>>
>>> I had to keep the TREE_HAS_VOLATILE handling as is since it
>>> applies to functions too (has side-effects).  Otherwise the
>>> attr-nothrow-2.C test fails.
>>
>>
>> When I mentioned "noreturn" above I was referring to
>> TREE_HAS_VOLATILE; sorry I wasn't clear.  For functions it should be
>> handled along with nothrow/readonly/malloc/pure.
>>
>> Jason
>>
>