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

Tue Jan 31 19:09:12 PST 2017

> 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/diagnose-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_object_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/dia
> gnose-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/diagnose-if-warn-system-header.h
> >> >>>>>> (added)
> >> >>>>>> +++ cfe/trunk/test/Sema/Inputs/diagnose-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/20170131/ef2af4fc/attachment.html>