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

Wed Feb 1 09:25:56 PST 2017

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/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/diagnose-if-warn-system-header.h
>>> >> >>>>>> URL:
>>> >> >>>>>>
>>> >> >>>>>>
>>> >> >>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/Inputs/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/diagnose_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
>>> >> >>>>>
>>> >> >>>>>
>>> >> >>>>
>>> >
>>> >
>>
>>
>