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

Wed Jun 27 09:24:00 PDT 2018

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