<div dir="ltr">Here's another regression that was introduced by the patch: <a href="https://bugs.llvm.org/show_bug.cgi?id=37935" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=37935</a> I landed a test for that in r335725 (in case you're wondering why there's a new test failure when you reland).<div><br></div><div>Thanks for working on this!</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Jun 26, 2018 at 9:03 AM Michael Kruse via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thank you for your reproducer. I debugged it and found the issue.<br>
ngettext is defined as follows.<br>
<br>
extern char *ngettext (const char *__msgid1, const char *__msgid2,<br>
unsigned long int __n)<br>
throw () __attribute__ ((__format_arg__ (1))) __attribute__<br>
((__format_arg__ (2)));<br>
<br>
Indeed, two format attributes (Just not the plain "format" I<br>
expected). The code which is processing it from SemaChecking.cpp:5521<br>
<br>
if (const FormatArgAttr *FA = ND->getAttr<FormatArgAttr>()) {<br>
<br>
That is, the code as written can process at most one FormatAttr and<br>
takes the first one (which is a different one before and after my<br>
patch).<br>
<br>
I suggest to change this to:<br>
<br>
for (const auto *FA : FDecl->specific_attrs<FormatAttr>()) {<br>
<br>
such that both arguments to ngettext are checked, as defined by the<br>
attributes. That also means that emitting the warning is emitted with<br>
and without my patch.<br>
<br>
Michael<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
2018-06-25 19:29 GMT-05:00 David Jones <<a href="mailto:dlj@google.com" target="_blank">dlj@google.com</a>>:<br>
> (Sorry for the late reply...)<br>
><br>
> On Mon, Jun 25, 2018 at 2:45 PM Michael Kruse via cfe-commits<br>
> <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> I just revert if to have further discussions (r335516)<br>
>><br>
>> Michael<br>
>><br>
>> 2018-06-25 14:58 GMT-05:00 Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>>:<br>
>> ><br>
>> ><br>
>> > On Mon, Jun 25, 2018 at 12:21 PM Richard Smith via cfe-commits<br>
>> > <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>> >><br>
>> >> On 23 June 2018 at 22:34, Michael Kruse via cfe-commits<br>
>> >> <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>> >>><br>
>> >>> Hi,<br>
>> >>><br>
>> >>> multiple comments in the code indicate that the attribute order was<br>
>> >>> surprising and probably has lead to bugs, and will lead to bugs in the<br>
>> >>> future. The order had to be explicitly reversed to avoid those. This<br>
>> >>> alone for me this seems a good reason to have the attributes in the<br>
>> >>> order in which they appear in the source.<br>
>> >>><br>
>> >>> It would be nice it you could send a reproducer. At a glance, your<br>
>> >>> case does not seem related since the format strings are function<br>
>> >>> arguments, not attributes.<br>
><br>
><br>
> Well, there is a nested function call, but the format warning applies to two<br>
> different parameters: a singular phrasing string, and a plural one. A<br>
> reproducer would look like this, which is almost verbatim from the link:<br>
><br>
> $ cat /tmp/repro.cc<br>
> #include <libintl.h><br>
> #include <stdio.h><br>
><br>
> void foo(){ printf(ngettext("one frobber", "%lu frobbers", 0UL), 0UL); }<br>
> $ <snip...>/bin/clang -Wall -c -o /dev/null /tmp/repro.cc<br>
> /tmp/repro.cc:4:66: warning: data argument not used by format string<br>
> [-Wformat-extra-args]<br>
> void foo(){ printf(ngettext("one frobber", "%lu frobbers", 0UL), 0UL); }<br>
> ~~~~~~~~~~~~~ ^<br>
> 1 warning generated.<br>
><br>
><br>
> Swapping the string params works, and an older revision yields inverse<br>
> results.<br>
><br>
><br>
>><br>
>> >>> 2018-06-23 17:17 GMT-05:00 David Jones <<a href="mailto:dlj@google.com" target="_blank">dlj@google.com</a>>:<br>
>> >>> > Since the semantics of the ordering of multiple format args seems<br>
>> >>> > somewhat<br>
>> >>> > ill-defined, it seems to me like reverting may be the best near-term<br>
>> >>> > choice.<br>
>> >>> > I can imagine other ways to fix the diagnostic, but the only<br>
>> >>> > behaviour<br>
>> >>> > that<br>
>> >>> > I would believe to be expected by existing code is the old one, so a<br>
>> >>> > change<br>
>> >>> > like this one probably needs to be more carefully vetted before<br>
>> >>> > being<br>
>> >>> > (re-)committed.<br>
>> >>><br>
>> >>> Could you give more details about your concerns?<br>
><br>
><br>
><br>
> Multiple format strings (or repeating or reordering other attributes) seems<br>
> like it lacks clear semantics. I can't find any particularly well-documented<br>
> explanation of Clang's behaviour in these cases, which makes me suspect that<br>
> most people would just "fix it until it works" and move on. GCC's<br>
> documentation doesn't seem to be very decisive, either.<br>
><br>
> For example, you mentioned that multiple format attributes may be invalid;<br>
> if so, then rejecting such cases should probably be a part of the fix. But<br>
> that would require some vetting, since this (pretty clearly) doesn't mirror<br>
> the current reality.<br>
><br>
><br>
>> >> (I'm not sure what the problem is, but as a data point, Sema::checkCall<br>
>> >> iterates over the FormatAttrs in order, so it's possible that changing<br>
>> >> the<br>
>> >> order may have triggered a new warning. That may be due to a<br>
>> >> pre-existing<br>
>> >> order-dependence bug, or it may be that we're losing an attribute<br>
>> >> here.)<br>
><br>
><br>
> It's arguably a pre-existing order dependence bug in ngettext, but it would<br>
> be ... well, a fairly prevalent bug, since it could churn any such calls<br>
> through libintl/gettext/etc.<br>
><br>
>><br>
>> >>> > Please let me know your plan. (If I don't hear back in a day or so,<br>
>> >>> > I'll go<br>
>> >>> > ahead and revert for you as the safe default course of action.)<br>
>> >>><br>
>> >>> On a weekend?<br>
>> >><br>
>> >><br>
>> >> Yes; our policy is generally to revert to green if a patch causes<br>
>> >> regressions that aren't going to be fixed in a few hours. This is<br>
>> >> generally<br>
>> >> good for the committer, because it lets you figure out what's wrong and<br>
>> >> deal<br>
>> >> with it on your own schedule rather than being pressured to fix it<br>
>> >> urgently<br>
>> >> because you're blocking the work of others.<br>
><br>
><br>
> Well, the bug did repro on a weekend, so there's that. ;-)<br>
><br>
> My main goal was to give a tighter response timeout to account for any<br>
> timezone drift (with a target of Monday ~everywhere).<br>
_______________________________________________<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/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>