r199663 - Making some minor improvements to r199626.

[Hal Finkel]

----- Original Message -----
> From: "Aaron Ballman" <aaron at aaronballman.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "llvm cfe" <cfe-commits at cs.uiuc.edu>, "Ted Kremenek" <kremenek at apple.com>
> Sent: Friday, July 11, 2014 11:42:10 AM
> Subject: Re: r199663 - Making some minor improvements to r199626.
> 
> 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.

Great, thanks! I really appreciate the prompt fix.

 -Hal

> 
> ~Aaron
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory