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