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

[David Jones via cfe-commits]

(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).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180625/4d5bf671/attachment-0001.html>