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

Tue Jan 31 18:31:43 PST 2017

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
>> >>>>>
>> >>>>>
>> >>>>
>
>