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

Tue Jan 31 19:17:47 PST 2017

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/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/cdab8c37/attachment-0001.html>