r199663 - Making some minor improvements to r199626.

[Aaron Ballman]

On Fri, Jul 11, 2014 at 12:29 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Fri, Jul 11, 2014 at 12:13 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>> ----- Original Message -----
>>> From: "Aaron Ballman" <aaron at aaronballman.com>
>>> To: "Hal Finkel" <hfinkel at anl.gov>, "Ted Kremenek" <kremenek at apple.com>
>>> Cc: "llvm cfe" <cfe-commits at cs.uiuc.edu>
>>> Sent: Friday, July 11, 2014 11:05:30 AM
>>> Subject: Re: r199663 - Making some minor improvements to r199626.
>>>
>>> On Fri, Jul 11, 2014 at 10:48 AM, Hal Finkel <hfinkel at anl.gov> wrote:
>>> > ----- Original Message -----
>>> >> From: "Aaron Ballman" <aaron at aaronballman.com>
>>> >> To: cfe-commits at cs.uiuc.edu
>>> >> Sent: Monday, January 20, 2014 8:19:44 AM
>>> >> Subject: r199663 - Making some minor improvements to r199626.
>>> >>
>>> >> Author: aaronballman
>>> >> Date: Mon Jan 20 08:19:44 2014
>>> >> New Revision: 199663
>>> >>
>>> >> URL: http://llvm.org/viewvc/llvm-project?rev=199663&view=rev
>>> >> Log:
>>> >> Making some minor improvements to r199626.
>>> >>
>>> >> Modified:
>>> >>     cfe/trunk/include/clang/Basic/Attr.td
>>> >>     cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>>> >>     cfe/trunk/test/Sema/nonnull.c
>>> >>
>>> >
>>> > [snip]
>>> >
>>> >> Modified: cfe/trunk/test/Sema/nonnull.c
>>> >> URL:
>>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/nonnull.c?rev=199663&r1=199662&r2=199663&view=diff
>>> >> ==============================================================================
>>> >> --- cfe/trunk/test/Sema/nonnull.c (original)
>>> >> +++ cfe/trunk/test/Sema/nonnull.c Mon Jan 20 08:19:44 2014
>>> >> @@ -32,10 +32,10 @@ void test_baz() {
>>> >>    baz3(0); // no-warning
>>> >>  }
>>> >>
>>> >> -void test_void_returns_nonnull()
>>> >> __attribute__((returns_nonnull));
>>> >> // expected-warning {{'returns_nonnull' attribute only applies to
>>> >> return values that are pointers}}
>>> >> -int test_int_returns_nonnull() __attribute__((returns_nonnull));
>>> >> //
>>> >> expected-warning {{'returns_nonnull' attribute only applies to
>>> >> return values that are pointers}}
>>> >> -void *test_ptr_returns_nonnull()
>>> >> __attribute__((returns_nonnull));
>>> >> // no-warning
>>> >> +void test_void_returns_nonnull(void)
>>> >> __attribute__((returns_nonnull)); // expected-warning
>>> >> {{'returns_nonnull' attribute only applies to return values that
>>> >> are
>>> >> pointers}}
>>> >> +int test_int_returns_nonnull(void)
>>> >> __attribute__((returns_nonnull));
>>> >> // expected-warning {{'returns_nonnull' attribute only applies to
>>> >> return values that are pointers}}
>>> >> +void *test_ptr_returns_nonnull(void)
>>> >> __attribute__((returns_nonnull)); // no-warning
>>> >>
>>> >>  int i __attribute__((nonnull)); // expected-warning {{'nonnull'
>>> >>  attribute only applies to functions, methods, and parameters}}
>>> >>  int j __attribute__((returns_nonnull)); // expected-warning
>>> >>  {{'returns_nonnull' attribute only applies to functions and
>>> >>  methods}}
>>> >> -
>>> >> +void *test_no_fn_proto() __attribute__((returns_nonnull));  //
>>> >> expected-warning {{'returns_nonnull' attribute only applies to
>>> >> functions and methods}}
>>> >
>>> > This last check seems wrong to me. There is now a difference
>>> > between this:
>>> >
>>> > void *test_ptr_returns_nonnull(void)
>>> > __attribute__((returns_nonnull));
>>> >
>>> > which we handle and this:
>>> >
>>> > void *test_ptr_returns_nonnull() __attribute__((returns_nonnull));
>>> >
>>> > for which we ignore the attribute. However:
>>> >
>>> >  - Most people don't put void in the parameter lists, and
>>>
>>> This is a C test file, where () is different than (void). The former
>>> has no prototype and is almost equivalent to it being written (...),
>>> while the latter has a prototype accepting no arguments. In C++ mode,
>>> there is no distinction.
>>
>> I understand the distinction, but it is not relevant to an attribute on the return value.
>
> Agreed.
>
>>
>>>
>>> >  - gcc 4.9.0 does not distinguish between the two forms w.r.t. the
>>> >  application of this attribute.
>>> >
>>> > Can you please fix this?
>>>
>>> Ted was the original author of this functionality
>>> (http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140113/097562.html),
>>> this commit was a tidying of his original code but had no functional
>>> changes. Are you asking to have C code with no function prototype
>>> honor this attribute?
>>
>> Yes, this is what gcc does. Also, you clearly did change the behavior in Ted's test case because you removed this:
>>
>> -void *test_ptr_returns_nonnull() __attribute__((returns_nonnull)); // no-warning
>>
>> and then added this:
>>
>> +void *test_no_fn_proto() __attribute__((returns_nonnull));  // expected-warning {{'returns_nonnull' attribute only applies to functions and methods}}
>
> Derp! I misspoke, sorry about that.
>
> Ted's original declaration of the attribute set a subject requirement
> of "HasFunctionProto", which is I was asking for Ted's opinion in case
> there was logic there I wasn't following. That being said, I don't see
> why this would require a prototype (and gcc compat makes sense too).
> I'll look into changing this, unless Ted comes back with something.

"Looking into" turns out to yield r212827. I am pretty sure this is
explained by some likely copy/paste problems. The
getFunctionOrMethodResultType was relying on a FunctionProtoType being
available for the function declaration, but one isn't required to get
the return type. This in turn meant that the returns_nonnull attribute
declaration required HasFunctionProto. Fixing these two up resolves
the issue.

~Aaron