[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: r291963 - [clang] Emit `diagnose_if` warnings from system headers
From: George Burgess IV via cfe-commits <cfe-commits () lists ! llvm ! org>
Date: 2017-01-31 19:19:13
Message-ID: CAKh6zBFTZPBO2sHwgT8Z8enkNy0FnOOL_1rqJbb_E2mmMpV0vg () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> If not, perhaps we should disable the attribute for the Clang 4 release
instead
FWIW, I'd strongly prefer to do this over letting diagnose_if go into Clang
4 unpatched. So, if my patch does feel too big, I'm happy to let
diagnose_if be a new-in-clang-5 attribute. :)
On Tue, Jan 31, 2017 at 11:11 AM, Richard Smith <richardsmith@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@chromium.org> wrote:
>
>> Ping?
>>
>> On Thu, Jan 26, 2017 at 10:21 AM, Hans Wennborg <hans@chromium.org>
>> wrote:
>> > Ping?
>> >
>> > On Mon, Jan 23, 2017 at 4:27 PM, Hans Wennborg <hans@chromium.org>
>> wrote:
>> >> Ping?
>> >>
>> >> On Tue, Jan 17, 2017 at 4:16 PM, Hans Wennborg <hans@chromium.org>
>> wrote:
>> >>> Richard, what do you think?
>> >>>
>> >>> On Fri, Jan 13, 2017 at 3:16 PM, Eric Fiselier <eric@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@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@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/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@lists.llvm.org
>> >>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >>>>>
>> >>>>>
>> >>>>
>>
>
>
[Attachment #5 (text/html)]
<div dir="ltr">> <span style="font-size:12.8px">If not, perhaps we should disable \
the attribute for the Clang 4 release instead</span><div><span \
style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">FWIW, \
I'd strongly prefer to do this over letting diagnose_if go into Clang 4 \
unpatched. So, if my patch does feel too big, I'm happy to let diagnose_if be a \
new-in-clang-5 attribute. :)</span></div><div class="gmail_extra"><br><div \
class="gmail_quote">On Tue, Jan 31, 2017 at 11:11 AM, Richard Smith <span \
dir="ltr"><<a href="mailto:richardsmith@google.com" \
target="_blank">richardsmith@google.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr">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.<div><div \
class="m_2789079959360023611m_-775506219070591665m_8538711055367444204h5"><br><div \
class="gmail_extra"><br><div class="gmail_quote">On 31 January 2017 at 10:36, Hans \
Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" \
target="_blank">hans@chromium.org</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">Ping?<br> <br>
On Thu, Jan 26, 2017 at 10:21 AM, Hans Wennborg <<a \
href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>> wrote:<br> \
> Ping?<br> ><br>
> On Mon, Jan 23, 2017 at 4:27 PM, Hans Wennborg <<a \
href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>> wrote:<br> \
>> Ping?<br> >><br>
>> On Tue, Jan 17, 2017 at 4:16 PM, Hans Wennborg <<a \
href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>> wrote:<br> \
>>> Richard, what do you think?<br> >>><br>
>>> On Fri, Jan 13, 2017 at 3:16 PM, Eric Fiselier <<a \
href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>> wrote:<br> \
>>>> I would love to see this merged. It would make it easier to write \
libc++<br> >>>> tests if the tests didn't have to worry about the old \
4.0 behavior.<br> >>>><br>
>>>> CC'ing Richard: Would merging this be OK?<br>
>>>><br>
>>>> On Fri, Jan 13, 2017 at 3:46 PM, George Burgess IV<br>
>>>> <<a href="mailto:george.burgess.iv@gmail.com" \
target="_blank">george.burgess.iv@gmail.com</a>> wrote:<br> \
>>>>><br> >>>>> Do we want to consider merging this \
into the release branch? Seems like<br> >>>>> more of a bugfix than a \
feature to me.<br> >>>>><br>
>>>>> On Fri, Jan 13, 2017 at 2:11 PM, Eric Fiselier via \
cfe-commits<br> >>>>> <<a href="mailto:cfe-commits@lists.llvm.org" \
target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br> \
>>>>>><br> >>>>>> Author: ericwf<br>
>>>>>> Date: Fri Jan 13 16:11:40 2017<br>
>>>>>> New Revision: 291963<br>
>>>>>><br>
>>>>>> URL: <a \
href="http://llvm.org/viewvc/llvm-project?rev=291963&view=rev" rel="noreferrer" \
target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=291963&view=rev</a><br>
>>>>>> Log:<br>
>>>>>> [clang] Emit `diagnose_if` warnings from system headers<br>
>>>>>><br>
>>>>>> Summary: In order for libc++ to meaningfully use \
`diagnose_if` warnings<br> >>>>>> they need to be emitted from \
system headers by default. This patch changes<br> >>>>>> the \
`diagnose_if` warning diagnostic to be shown in system headers.<br> \
>>>>>><br> >>>>>> Reviewers: george.burgess.iv, \
rsmith, aaron.ballman<br> >>>>>><br>
>>>>>> Subscribers: cfe-commits<br>
>>>>>><br>
>>>>>> Differential Revision: <a \
href="https://reviews.llvm.org/D28703" rel="noreferrer" \
target="_blank">https://reviews.llvm.org/D2870<wbr>3</a><br> \
>>>>>><br> >>>>>> Added:<br>
>>>>>> \
cfe/trunk/test/Sema/Inputs/di<wbr>agnose-if-warn-system-header.h<br> \
>>>>>> Modified:<br> >>>>>> \
cfe/trunk/include/clang/Basic<wbr>/DiagnosticSemaKinds.td<br> \
>>>>>> cfe/trunk/test/Sema/diagnose_<wbr>if.c<br> \
>>>>>><br> >>>>>> Modified: \
cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td<br> \
>>>>>> URL:<br> >>>>>> <a \
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=291963&r1=291962&r2=291963&view=diff" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/in \
clude/clang/<wbr>Basic/DiagnosticSemaKinds.td?r<wbr>ev=291963&r1=291962&r2=291963&<wbr>view=diff</a><br>
>>>>>><br>
>>>>>> \
==============================<wbr>==============================<wbr>==================<br>
>>>>>> --- \
cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td (original)<br> \
>>>>>> +++ \
cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td Fri Jan 13<br> \
>>>>>> 16:11:40 2017<br> >>>>>> @@ -3380,7 \
+3380,8 @@ def note_ovl_candidate_has_pass_ob<wbr>ject_s<br> >>>>>> \
"candidate address cannot be taken because parameter %0 has "<br> \
>>>>>> "pass_object_size attribute">;<br> \
>>>>>> def err_diagnose_if_succeeded : \
Error<"%0">;<br> >>>>>> -def \
warn_diagnose_if_succeeded : Warning<"%0">,<br> \
>>>>>> InGroup<UserDefinedWarnings>;<br> \
>>>>>> +def warn_diagnose_if_succeeded : \
Warning<"%0">,<br> >>>>>> \
InGroup<UserDefinedWarnings>,<br> >>>>>> + \
ShowInSystemHeader;<br> >>>>>> def \
note_ovl_candidate_disabled_by<wbr>_function_cond_attr : Note<<br> \
>>>>>> "candidate disabled: %0">;<br> \
>>>>>> def note_ovl_candidate_disabled_by<wbr>_extension : \
Note<<br> >>>>>><br>
>>>>>> Added: \
cfe/trunk/test/Sema/Inputs/dia<wbr>gnose-if-warn-system-header.h<br> \
>>>>>> URL:<br> >>>>>> <a \
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/Inputs/diagnose-if-warn-system-header.h?rev=291963&view=auto" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/te \
st/Sema/Inpu<wbr>ts/diagnose-if-warn-system-hea<wbr>der.h?rev=291963&view=auto</a><br>
>>>>>><br>
>>>>>> \
==============================<wbr>==============================<wbr>==================<br>
>>>>>> --- \
cfe/trunk/test/Sema/Inputs/dia<wbr>gnose-if-warn-system-header.h (added)<br> \
>>>>>> +++ \
cfe/trunk/test/Sema/Inputs/dia<wbr>gnose-if-warn-system-header.h Fri Jan<br> \
>>>>>> 13 16:11:40 2017<br> >>>>>> @@ -0,0 +1,11 \
@@<br> >>>>>> +#pragma GCC system_header<br>
>>>>>> +<br>
>>>>>> +inline int system_header_func(int x)<br>
>>>>>> + __attribute__((diagnose_if(x == x, "system header \
warning",<br> >>>>>> "warning"))) // expected-note \
{{from 'diagnose_if' attribute}}<br> >>>>>> +{<br>
>>>>>> + return 0;<br>
>>>>>> +}<br>
>>>>>> +<br>
>>>>>> +void test_system_header() {<br>
>>>>>> + system_header_func(0); // expected-warning {{system \
header warning}}<br> >>>>>> +}<br>
>>>>>><br>
>>>>>> Modified: cfe/trunk/test/Sema/diagnose_i<wbr>f.c<br>
>>>>>> URL:<br>
>>>>>> <a \
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/diagnose_if.c?rev=291963&r1=291962&r2=291963&view=diff" \
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/te \
st/Sema/diag<wbr>nose_if.c?rev=291963&r1=291962<wbr>&r2=291963&view=diff</a><br>
>>>>>><br>
>>>>>> \
==============================<wbr>==============================<wbr>==================<br>
>>>>>> --- cfe/trunk/test/Sema/diagnose_i<wbr>f.c (original)<br>
>>>>>> +++ cfe/trunk/test/Sema/diagnose_i<wbr>f.c Fri Jan 13 \
16:11:40 2017<br> >>>>>> @@ -150,3 +150,6 @@ void \
alwaysWarnWithArg(int a) _diagnose_<br> >>>>>> void \
runAlwaysWarnWithArg(int a) {<br> >>>>>> alwaysWarnWithArg(a); \
// expected-warning{{alwaysWarn}}<br> >>>>>> }<br>
>>>>>> +<br>
>>>>>> +// Test that diagnose_if warnings generated in system \
headers are not<br> >>>>>> ignored.<br>
>>>>>> +#include \
"Inputs/diagnose-if-warn-syste<wbr>m-header.h"<br> \
>>>>>><br> >>>>>><br>
>>>>>> ______________________________<wbr>_________________<br>
>>>>>> cfe-commits mailing list<br>
>>>>>> <a href="mailto:cfe-commits@lists.llvm.org" \
target="_blank">cfe-commits@lists.llvm.org</a><br> >>>>>> <a \
href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" \
target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
>>>>><br>
>>>>><br>
>>>><br>
</blockquote></div><br></div></div></div></div>
</blockquote></div><br></div></div>
[Attachment #6 (text/plain)]
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic