r335084 - Append new attributes to the end of an AttributeList.

Wed Jun 27 05:12:06 PDT 2018

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 at 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 <dlj at google.com>:
> > (Sorry for the late reply...)
> >
> > On Mon, Jun 25, 2018 at 2:45 PM Michael Kruse via cfe-commits
> > <cfe-commits at lists.llvm.org> wrote:
> >>
> >> I just revert if to have further discussions (r335516)
> >>
> >> Michael
> >>
> >> 2018-06-25 14:58 GMT-05:00 Eric Christopher <echristo at gmail.com>:
> >> >
> >> >
> >> > On Mon, Jun 25, 2018 at 12:21 PM Richard Smith via cfe-commits
> >> > <cfe-commits at lists.llvm.org> wrote:
> >> >>
> >> >> On 23 June 2018 at 22:34, Michael Kruse via cfe-commits
> >> >> <cfe-commits at 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 <dlj at 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180627/ef32fef2/attachment.html>