r291963 - [clang] Emit `diagnose_if` warnings from system headers

Wed Feb 1 10:20:33 PST 2017

Awesome. Thank you all! :)

On Wed, Feb 1, 2017 at 9:25 AM, Hans Wennborg <hans at chromium.org> wrote:

> Merged this (r291963) in r293783.
>
> And the others (r293360 + r293369) in r293784.
>
> Thanks,
> Hans
>
> On Tue, Jan 31, 2017 at 7:17 PM, Richard Smith <richardsmith at google.com>
> wrote:
> > I'm fine with these patches being merged. Hopefully we still have plenty
> of
> > time to shake out any problems between now and the release.
> >
> > On 31 January 2017 at 19:09, George Burgess IV <
> george.burgess.iv at gmail.com>
> > wrote:
> >>
> >> > IIUC the major risk is that diagnose_if itself turns out to be broken,
> >> > not that we'd miscompile anything?
> >>
> >> Correct. These patches should be NFC to code that doesn't use
> diagnose_if.
> >>
> >> If something about that patch *had* to break existing
> >> non-diagnose_if-aware code, we're now calling Sema::CheckFunctionCall
> (which
> >> checks diagnose_if attributes, checks we're not passing a nullptr into
> >> _Nonnull args, ...) for more kinds of C++ function calls than before. We
> >> weren't calling it before in these places as an optimization: in
> theory, it
> >> wasn't possible for CheckFunctionCall to find issues in calls to things
> like
> >> conversion functions. Now that we have diagnose_if, it is.
> >>
> >> On Tue, Jan 31, 2017 at 6:31 PM, Hans Wennborg <hans at chromium.org>
> wrote:
> >>>
> >>> I'm Ok with taking the larger patch (r293360 + r293369) too. It's been
> >>> in tree for a bit, there is still a number of weeks before the
> >>> release, and IIUC the major risk is that diagnose_if itself turns out
> >>> to be broken, not that we'd miscompile anything?
> >>>
> >>> On Tue, Jan 31, 2017 at 11:11 AM, Richard Smith <
> richardsmith at google.com>
> >>> wrote:
> >>> > Yes, this makes sense. We should also decide what we're going to do
> >>> > about
> >>> > the larger diagnose_if patch that George has asked to be ported to
> >>> > Clang 4.
> >>> > Are you comfortable taking a patch of that size? If not, perhaps we
> >>> > should
> >>> > disable the attribute for the Clang 4 release instead.
> >>> >
> >>> >
> >>> > On 31 January 2017 at 10:36, Hans Wennborg <hans at chromium.org>
> wrote:
> >>> >>
> >>> >> Ping?
> >>> >>
> >>> >> On Thu, Jan 26, 2017 at 10:21 AM, Hans Wennborg <hans at chromium.org>
> >>> >> wrote:
> >>> >> > Ping?
> >>> >> >
> >>> >> > On Mon, Jan 23, 2017 at 4:27 PM, Hans Wennborg <hans at chromium.org
> >
> >>> >> > wrote:
> >>> >> >> Ping?
> >>> >> >>
> >>> >> >> On Tue, Jan 17, 2017 at 4:16 PM, Hans Wennborg <
> hans at chromium.org>
> >>> >> >> wrote:
> >>> >> >>> Richard, what do you think?
> >>> >> >>>
> >>> >> >>> On Fri, Jan 13, 2017 at 3:16 PM, Eric Fiselier <eric at efcs.ca>
> >>> >> >>> wrote:
> >>> >> >>>> I would love to see this merged. It would make it easier to
> write
> >>> >> >>>> libc++
> >>> >> >>>> tests if the tests didn't have to worry about the old 4.0
> >>> >> >>>> behavior.
> >>> >> >>>>
> >>> >> >>>> CC'ing Richard: Would merging this be OK?
> >>> >> >>>>
> >>> >> >>>> On Fri, Jan 13, 2017 at 3:46 PM, George Burgess IV
> >>> >> >>>> <george.burgess.iv at gmail.com> wrote:
> >>> >> >>>>>
> >>> >> >>>>> Do we want to consider merging this into the release branch?
> >>> >> >>>>> Seems
> >>> >> >>>>> like
> >>> >> >>>>> more of a bugfix than a feature to me.
> >>> >> >>>>>
> >>> >> >>>>> On Fri, Jan 13, 2017 at 2:11 PM, Eric Fiselier via cfe-commits
> >>> >> >>>>> <cfe-commits at lists.llvm.org> wrote:
> >>> >> >>>>>>
> >>> >> >>>>>> Author: ericwf
> >>> >> >>>>>> Date: Fri Jan 13 16:11:40 2017
> >>> >> >>>>>> New Revision: 291963
> >>> >> >>>>>>
> >>> >> >>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=291963&view=rev
> >>> >> >>>>>> Log:
> >>> >> >>>>>> [clang] Emit `diagnose_if` warnings from system headers
> >>> >> >>>>>>
> >>> >> >>>>>> Summary: In order for libc++ to meaningfully use
> `diagnose_if`
> >>> >> >>>>>> warnings
> >>> >> >>>>>> they need to be emitted from system headers by default. This
> >>> >> >>>>>> patch
> >>> >> >>>>>> changes
> >>> >> >>>>>> the `diagnose_if` warning diagnostic to be shown in system
> >>> >> >>>>>> headers.
> >>> >> >>>>>>
> >>> >> >>>>>> Reviewers: george.burgess.iv, rsmith, aaron.ballman
> >>> >> >>>>>>
> >>> >> >>>>>> Subscribers: cfe-commits
> >>> >> >>>>>>
> >>> >> >>>>>> Differential Revision: https://reviews.llvm.org/D28703
> >>> >> >>>>>>
> >>> >> >>>>>> Added:
> >>> >> >>>>>>     cfe/trunk/test/Sema/Inputs/di
> agnose-if-warn-system-header.h
> >>> >> >>>>>> Modified:
> >>> >> >>>>>>     cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> >>> >> >>>>>>     cfe/trunk/test/Sema/diagnose_if.c
> >>> >> >>>>>>
> >>> >> >>>>>> Modified: cfe/trunk/include/clang/Basic/
> DiagnosticSemaKinds.td
> >>> >> >>>>>> URL:
> >>> >> >>>>>>
> >>> >> >>>>>>
> >>> >> >>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/
> Basic/DiagnosticSemaKinds.td?rev=291963&r1=291962&r2=291963&view=diff
> >>> >> >>>>>>
> >>> >> >>>>>>
> >>> >> >>>>>>
> >>> >> >>>>>> ============================================================
> ==================
> >>> >> >>>>>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> >>> >> >>>>>> (original)
> >>> >> >>>>>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri
> >>> >> >>>>>> Jan 13
> >>> >> >>>>>> 16:11:40 2017
> >>> >> >>>>>> @@ -3380,7 +3380,8 @@ def note_ovl_candidate_has_pass_ob
> ject_s
> >>> >> >>>>>>      "candidate address cannot be taken because parameter %0
> >>> >> >>>>>> has "
> >>> >> >>>>>>      "pass_object_size attribute">;
> >>> >> >>>>>>  def err_diagnose_if_succeeded : Error<"%0">;
> >>> >> >>>>>> -def warn_diagnose_if_succeeded : Warning<"%0">,
> >>> >> >>>>>> InGroup<UserDefinedWarnings>;
> >>> >> >>>>>> +def warn_diagnose_if_succeeded : Warning<"%0">,
> >>> >> >>>>>> InGroup<UserDefinedWarnings>,
> >>> >> >>>>>> +    ShowInSystemHeader;
> >>> >> >>>>>>  def note_ovl_candidate_disabled_by_function_cond_attr :
> Note<
> >>> >> >>>>>>      "candidate disabled: %0">;
> >>> >> >>>>>>  def note_ovl_candidate_disabled_by_extension : Note<
> >>> >> >>>>>>
> >>> >> >>>>>> Added:
> >>> >> >>>>>> cfe/trunk/test/Sema/Inputs/diagnose-if-warn-system-header.h
> >>> >> >>>>>> URL:
> >>> >> >>>>>>
> >>> >> >>>>>>
> >>> >> >>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/Inpu
> ts/diagnose-if-warn-system-header.h?rev=291963&view=auto
> >>> >> >>>>>>
> >>> >> >>>>>>
> >>> >> >>>>>>
> >>> >> >>>>>> ============================================================
> ==================
> >>> >> >>>>>> --- cfe/trunk/test/Sema/Inputs/dia
> gnose-if-warn-system-header.h
> >>> >> >>>>>> (added)
> >>> >> >>>>>> +++ cfe/trunk/test/Sema/Inputs/dia
> gnose-if-warn-system-header.h
> >>> >> >>>>>> Fri
> >>> >> >>>>>> Jan
> >>> >> >>>>>> 13 16:11:40 2017
> >>> >> >>>>>> @@ -0,0 +1,11 @@
> >>> >> >>>>>> +#pragma GCC system_header
> >>> >> >>>>>> +
> >>> >> >>>>>> +inline int system_header_func(int x)
> >>> >> >>>>>> +  __attribute__((diagnose_if(x == x, "system header
> warning",
> >>> >> >>>>>> "warning"))) // expected-note {{from 'diagnose_if'
> attribute}}
> >>> >> >>>>>> +{
> >>> >> >>>>>> +  return 0;
> >>> >> >>>>>> +}
> >>> >> >>>>>> +
> >>> >> >>>>>> +void test_system_header() {
> >>> >> >>>>>> +  system_header_func(0); // expected-warning {{system header
> >>> >> >>>>>> warning}}
> >>> >> >>>>>> +}
> >>> >> >>>>>>
> >>> >> >>>>>> Modified: cfe/trunk/test/Sema/diagnose_if.c
> >>> >> >>>>>> URL:
> >>> >> >>>>>>
> >>> >> >>>>>>
> >>> >> >>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/diag
> nose_if.c?rev=291963&r1=291962&r2=291963&view=diff
> >>> >> >>>>>>
> >>> >> >>>>>>
> >>> >> >>>>>>
> >>> >> >>>>>> ============================================================
> ==================
> >>> >> >>>>>> --- cfe/trunk/test/Sema/diagnose_if.c (original)
> >>> >> >>>>>> +++ cfe/trunk/test/Sema/diagnose_if.c Fri Jan 13 16:11:40
> 2017
> >>> >> >>>>>> @@ -150,3 +150,6 @@ void alwaysWarnWithArg(int a) _diagnose_
> >>> >> >>>>>>  void runAlwaysWarnWithArg(int a) {
> >>> >> >>>>>>    alwaysWarnWithArg(a); // expected-warning{{alwaysWarn}}
> >>> >> >>>>>>  }
> >>> >> >>>>>> +
> >>> >> >>>>>> +// Test that diagnose_if warnings generated in system
> headers
> >>> >> >>>>>> are
> >>> >> >>>>>> not
> >>> >> >>>>>> ignored.
> >>> >> >>>>>> +#include "Inputs/diagnose-if-warn-system-header.h"
> >>> >> >>>>>>
> >>> >> >>>>>>
> >>> >> >>>>>> _______________________________________________
> >>> >> >>>>>> cfe-commits mailing list
> >>> >> >>>>>> cfe-commits at lists.llvm.org
> >>> >> >>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >>> >> >>>>>
> >>> >> >>>>>
> >>> >> >>>>
> >>> >
> >>> >
> >>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170201/9108875b/attachment.html>