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

[Michael Kruse via cfe-commits]

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).