Ping:
https://gcc.gnu.org/ml/gcc-patches/2017-12/msg01494.html
(FWIW, the only other patch still needing review in the kit is:
"[v2 of PATCH 03/14] C++: add location_t wrapper nodes during parsing
(minimal impl)"
https://gcc.gnu.org/ml/gcc-patches/2017-12/msg01594.html )
On Fri, 2017-12-22 at 14:10 -0500, David Malcolm wrote:
> On Thu, 2017-12-21 at 00:00 -0500, Jason Merrill wrote:
> > On Wed, Dec 20, 2017 at 12:33 PM, David Malcolm <dmalc...@redhat.co
> > m>
> > wrote:
> > > On Tue, 2017-12-19 at 14:55 -0500, Jason Merrill wrote:
> > > > On 12/17/2017 11:29 AM, David Malcolm wrote:
> > > > > On Mon, 2017-12-11 at 18:45 -0500, Jason Merrill wrote:
> > > > > > On 11/10/2017 04:45 PM, David Malcolm wrote:
> > > > > > > + STRIP_ANY_LOCATION_WRAPPER (format_tree);
> > > > > > > +
> > > > > > > if (VAR_P (format_tree))
> > > > > > > {
> > > > > > > /* Pull out a constant value if the front end
> > > > > > > didn't. */
> > > > > >
> > > > > > It seems like we want fold_for_warn here instead of the
> > > > > > special
> > > > > > variable
> > > > > > handling. That probably makes sense for the other places
> > > > > > you
> > > > > > change in
> > > > > > this patch, too.
> > > > >
> > > > > Here's an updated version of the patch which uses
> > > > > fold_for_warn,
> > > > > rather than STRIP_ANY_LOCATION_WRAPPER.
> > > > >
> > > > > In one place it was necessary to add a STRIP_NOPS, since the
> > > > > fold_for_warn can add a cast around a:
> > > > > ADDR_EXPR <POINTER_TYPE(ARRAY_TYPE(char))> (
> > > > > STRING_CST)
> > > > > turning it into a:
> > > > > NOP_EXPR<POINTER_TYPE(char))> (
> > > > > ADDR_EXPR <POINTER_TYPE(ARRAY_TYPE(char))> (
> > > > > STRING_CST))
> > > >
> > > > Hmm, that seems like a bug. fold_for_warn shouldn't change the
> > > > type of
> > > > the result.
> > >
> > > In a similar vein, is the result of decl_constant_value (decl)
> > > required
> > > to be the same type as that of the decl?
> > >
> > > What's happening for this testcase (g++.dg/warn/Wformat-1.C) is
> > > that we
> > > have a VAR_DECL with a DECL_INITIAL, but the types of the two
> > > don't
> > > quite match up.
> > >
> > > decl_constant_value returns an unshare_expr clone of the
> > > DECL_INITIAL,
> > > and this is what's returned from fold_for_warn.
> > >
> > > Am I right in thinking
> > >
> > > (a) that the bug here is that a DECL_INITIAL ought to have the
> > > same
> > > type as its decl, and so there's a missing cast where that
> > > gets
> > > set up, or
> >
> > This seems right.
> >
> > > (b) should decl_constant_value handle such cases, and introduce a
> > > cast
> > > if it uncovers them?
> >
> > decl_constant_value should probably assert that the types match
> > closely enough.
>
> Thanks.
>
> You describe the types needing to match "closely enough" (as opposed
> to *exactly*), and that may be the key here: what I didn't say in my
> message above is that the decl is "const" whereas the value isn't.
>
> To identify where a DECL_INITIAL can have a different type to its
> decl
> (and thus fold_for_warn "changing type"), I tried adding this
> assertion:
>
> --- a/gcc/cp/typeck2.c
> +++ b/gcc/cp/typeck2.c
> @@ -857,6 +857,7 @@ store_init_value (tree decl, tree init,
> vec<tree, va_gc>** cleanups, int flags)
> /* If the value is a constant, just put it in DECL_INITIAL. If
> DECL
> is an automatic variable, the middle end will turn this into
> a
> dynamic initialization later. */
> + gcc_assert (TREE_TYPE (value) == TREE_TYPE (decl));
> DECL_INITIAL (decl) = value;
> return NULL_TREE;
> }
>
> The assertion fires when a "const" decl is initialized with a value
> of
> a non-const type; e.g. in g++.dg/warn/Wformat-1.C:
>
> const char *const msg = "abc";
>
> but I can also reproduce it with:
>
> const int i = 42;
>
> What happens is that the type of the decl has the "readonly_flag"
> set,
> whereas the type of the value has that flag clear.
>
> For the string case, convert_for_initialization finishes here:
>
> 8894 return convert_for_assignment (type, rhs, errtype,
> fndecl, parmnum,
> 8895 complain, flags);
>
> and convert_for_assignment finishes by calling:
>
> 8801 return perform_implicit_conversion_flags
> (strip_top_quals (type), rhs,
> 8802 complain,
> flags);
>
> The "strip_top_quals" in that latter call strips away the
> "readonly_flag" from type (the decl's type), giving the type of the
> rhs, and hence this does nothing.
>
> So we end up with a decl of type
> const char * const
> being initialized with a value of type
> const char *
> (or a decl of type "const int" being initialized with a value of
> type "int").
>
> So the types are slightly inconsistent (const vs non-const), but
> given
> your wording of types needing to match "closely enough" it's not
> clear
> to me that it's a problem - if I'm reading things right, it's coming
> from
> that strip_top_quals in the implicit conversion in
> convert_for_assignment.
>
> This also happens with a pristine build of trunk.
>
> Rather than attempting to change decl_constant_value or the folding
> code,
> the following patch simply updates the:
>
> if (VAR_P (format_tree))
> {
> /* Pull out a constant value if the front end didn't. */
> format_tree = decl_constant_value (format_tree);
> STRIP_NOPS (format_tree);
> }
>
> that you added in r219964 to fix PR c++/64629 to instead be:
>
> format_tree = fold_for_warn (format_tree);
> STRIP_NOPS (format_tree);
>
> and adds a little test coverage for the pointer addition case.
>
> Successfully bootstrapped®rtested on x86_64-pc-linux-gnu, as
> part of the kit.
>
> OK for trunk once the rest of the kit is approved?
>
>
> gcc/c-family/ChangeLog:
> * c-format.c (check_format_arg): Convert VAR_P check to a
> fold_for_warn.
>
> gcc/testsuite/ChangeLog:
> * g++.dg/warn/Wformat-1.C: Add tests of pointer arithmetic on
> format strings.
> ---
> gcc/c-family/c-format.c | 10 ++++------
> gcc/testsuite/g++.dg/warn/Wformat-1.C | 2 ++
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c
> index 164d035..1fcac2f 100644
> --- a/gcc/c-family/c-format.c
> +++ b/gcc/c-family/c-format.c
> @@ -1536,12 +1536,10 @@ check_format_arg (void *ctx, tree
> format_tree,
>
> location_t fmt_param_loc = EXPR_LOC_OR_LOC (format_tree,
> input_location);
>
> - if (VAR_P (format_tree))
> - {
> - /* Pull out a constant value if the front end didn't. */
> - format_tree = decl_constant_value (format_tree);
> - STRIP_NOPS (format_tree);
> - }
> + /* Pull out a constant value if the front end didn't, and handle
> location
> + wrappers. */
> + format_tree = fold_for_warn (format_tree);
> + STRIP_NOPS (format_tree);
>
> if (integer_zerop (format_tree))
> {
> diff --git a/gcc/testsuite/g++.dg/warn/Wformat-1.C
> b/gcc/testsuite/g++.dg/warn/Wformat-1.C
> index 6094a9c..f2e772a 100644
> --- a/gcc/testsuite/g++.dg/warn/Wformat-1.C
> +++ b/gcc/testsuite/g++.dg/warn/Wformat-1.C
> @@ -7,4 +7,6 @@ foo (void)
> {
> const char *const msg = "abc";
> bar (1, msg);
> + bar (1, msg + 1);
> + bar (1, 1 + msg);
> }
