Re: [C++ PATCH] Fix -Winvalid-offsetof warning (PR c++/68727)

[Jason Merrill <jason at redhat dot com>]

OK.

On Tue, Jan 24, 2017 at 5:43 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> r215070 moved -Winvalid-offsetof pedwarn from build_class_member_access_expr
> (where it warned even about offsetof-like hand-written constructs) to
> finish_offsetof.  That is fine, the problem is that while
> build_class_member_access_expr saw the original first argument to
> __builtin_offsetof (the object type), finish_offsetof can see a different
> type and the first argument of COMPONENT_REF is a result of various folding
> (e.g. build_base_path (prematurely?) folds trees etc.), so it would be too
> hard to reliably discover the original type.
> Now, if the type finish_offsetof sees is e.g. a std layout type,
> while the original type is not, we don't emit a warning that we should.
> Instead of trying to discover the original type from the folded expression,
> the following patch passes the type (well, a cast of null pointer to
> pointer to that type, as that is something we have handy) to finish_offsetof
> and for template instantiation saves it in a new OFFSETOF_EXPR argument.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2017-01-24  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/68727
>         * cp-tree.def (OFFSETOF_EXPR): Bump number of operands to 2.
>         * cp-tree.h (finish_offsetof): Add OBJECT_PTR argument.
>         * parser.c (cp_parser_builtin_offsetof): Pass result of
>         build_static_cast of null_pointer_node to finish_offsetof.
>         * semantics.c (finish_offsetof): Add OBJECT_PTR argument, use
>         it for -Winvalid-offsetof pedwarn instead of trying to guess
>         original offsetof type from EXPR.  Save OBJECT_PTR as a new
>         second operand to OFFSETOF_EXPR.
>         * pt.c (tsubst_copy_and_build) <case OFFSETOF_EXPR>: Adjust
>         finish_offsetof caller, pass the second operand of OFFSETOF_EXPR
>         as OBJECT_PTR.
>
>         * g++.dg/other/offsetof8.C: Add expected error.
>         * g++.dg/other/offsetof9.C: New test.
>
> --- gcc/cp/cp-tree.def.jj       2017-01-10 08:12:46.000000000 +0100
> +++ gcc/cp/cp-tree.def  2017-01-24 14:27:00.907820968 +0100
> @@ -333,7 +333,7 @@ DEFTREECODE (EXPR_STMT, "expr_stmt", tcc
>  DEFTREECODE (TAG_DEFN, "tag_defn", tcc_expression, 0)
>
>  /* Represents an 'offsetof' expression during template expansion.  */
> -DEFTREECODE (OFFSETOF_EXPR, "offsetof_expr", tcc_expression, 1)
> +DEFTREECODE (OFFSETOF_EXPR, "offsetof_expr", tcc_expression, 2)
>
>  /* Represents an '__builtin_addressof' expression during template
>     expansion.  This is similar to ADDR_EXPR, but it doesn't invoke
> --- gcc/cp/cp-tree.h.jj 2017-01-21 02:26:07.000000000 +0100
> +++ gcc/cp/cp-tree.h    2017-01-24 14:14:51.132548587 +0100
> @@ -6485,7 +6485,7 @@ extern tree finish_underlying_type
>  extern tree calculate_bases                     (tree);
>  extern tree finish_bases                        (tree, bool);
>  extern tree calculate_direct_bases              (tree);
> -extern tree finish_offsetof                    (tree, location_t);
> +extern tree finish_offsetof                    (tree, tree, location_t);
>  extern void finish_decl_cleanup                        (tree, tree);
>  extern void finish_eh_cleanup                  (tree);
>  extern void emit_associated_thunks             (tree);
> --- gcc/cp/parser.c.jj  2017-01-21 02:26:06.000000000 +0100
> +++ gcc/cp/parser.c     2017-01-24 14:18:22.482716966 +0100
> @@ -9498,11 +9498,12 @@ cp_parser_builtin_offsetof (cp_parser *p
>    token = cp_lexer_peek_token (parser->lexer);
>
>    /* Build the (type *)null that begins the traditional offsetof macro.  */
> -  expr = build_static_cast (build_pointer_type (type), null_pointer_node,
> -                            tf_warning_or_error);
> +  tree object_ptr
> +    = build_static_cast (build_pointer_type (type), null_pointer_node,
> +                        tf_warning_or_error);
>
>    /* Parse the offsetof-member-designator.  We begin as if we saw "expr->".  */
> -  expr = cp_parser_postfix_dot_deref_expression (parser, CPP_DEREF, expr,
> +  expr = cp_parser_postfix_dot_deref_expression (parser, CPP_DEREF, object_ptr,
>                                                  true, &dummy, token->location);
>    while (true)
>      {
> @@ -9554,7 +9555,7 @@ cp_parser_builtin_offsetof (cp_parser *p
>    loc = make_location (loc, start_loc, finish_loc);
>    /* The result will be an INTEGER_CST, so we need to explicitly
>       preserve the location.  */
> -  expr = cp_expr (finish_offsetof (expr, loc), loc);
> +  expr = cp_expr (finish_offsetof (object_ptr, expr, loc), loc);
>
>   failure:
>    parser->integral_constant_expression_p = save_ice_p;
> --- gcc/cp/semantics.c.jj       2017-01-21 02:26:06.000000000 +0100
> +++ gcc/cp/semantics.c  2017-01-24 14:30:05.024371882 +0100
> @@ -3995,13 +3995,13 @@ finish_bases (tree type, bool direct)
>     fold_offsetof.  */
>
>  tree
> -finish_offsetof (tree expr, location_t loc)
> +finish_offsetof (tree object_ptr, tree expr, location_t loc)
>  {
>    /* If we're processing a template, we can't finish the semantics yet.
>       Otherwise we can fold the entire expression now.  */
>    if (processing_template_decl)
>      {
> -      expr = build1 (OFFSETOF_EXPR, size_type_node, expr);
> +      expr = build2 (OFFSETOF_EXPR, size_type_node, expr, object_ptr);
>        SET_EXPR_LOCATION (expr, loc);
>        return expr;
>      }
> @@ -4031,19 +4031,15 @@ finish_offsetof (tree expr, location_t l
>      }
>    if (REFERENCE_REF_P (expr))
>      expr = TREE_OPERAND (expr, 0);
> -  if (TREE_CODE (expr) == COMPONENT_REF)
> -    {
> -      tree object = TREE_OPERAND (expr, 0);
> -      if (!complete_type_or_else (TREE_TYPE (object), object))
> -       return error_mark_node;
> -      if (warn_invalid_offsetof
> -         && CLASS_TYPE_P (TREE_TYPE (object))
> -         && CLASSTYPE_NON_STD_LAYOUT (TREE_TYPE (object))
> -         && cp_unevaluated_operand == 0)
> -       pedwarn (loc, OPT_Winvalid_offsetof,
> -                "offsetof within non-standard-layout type %qT is undefined",
> -                TREE_TYPE (object));
> -    }
> +  if (!complete_type_or_else (TREE_TYPE (TREE_TYPE (object_ptr)), object_ptr))
> +    return error_mark_node;
> +  if (warn_invalid_offsetof
> +      && CLASS_TYPE_P (TREE_TYPE (TREE_TYPE (object_ptr)))
> +      && CLASSTYPE_NON_STD_LAYOUT (TREE_TYPE (TREE_TYPE (object_ptr)))
> +      && cp_unevaluated_operand == 0)
> +    pedwarn (loc, OPT_Winvalid_offsetof,
> +            "offsetof within non-standard-layout type %qT is undefined",
> +            TREE_TYPE (TREE_TYPE (object_ptr)));
>    return fold_offsetof (expr);
>  }
>
> --- gcc/cp/pt.c.jj      2017-01-23 23:57:14.000000000 +0100
> +++ gcc/cp/pt.c 2017-01-24 15:03:34.490626712 +0100
> @@ -17707,8 +17707,15 @@ tsubst_copy_and_build (tree t,
>        }
>
>      case OFFSETOF_EXPR:
> -      RETURN (finish_offsetof (RECUR (TREE_OPERAND (t, 0)),
> -                              EXPR_LOCATION (t)));
> +      {
> +       tree object_ptr
> +         = tsubst_copy_and_build (TREE_OPERAND (t, 1), args, complain,
> +                                  in_decl, /*function_p=*/false,
> +                                  /*integral_constant_expression_p=*/false);
> +       RETURN (finish_offsetof (object_ptr,
> +                                RECUR (TREE_OPERAND (t, 0)),
> +                                EXPR_LOCATION (t)));
> +      }
>
>      case ADDRESSOF_EXPR:
>        RETURN (cp_build_addressof (EXPR_LOCATION (t),
> --- gcc/testsuite/g++.dg/other/offsetof8.C.jj   2015-12-09 14:38:58.000000000 +0100
> +++ gcc/testsuite/g++.dg/other/offsetof8.C      2017-01-24 14:47:19.255588890 +0100
> @@ -9,4 +9,4 @@ struct B: virtual A { };
>  int a[]  = {
>    !&((B*)0)->i,    // { dg-error "invalid access to non-static data member" }
>    __builtin_offsetof (B, i)   // { dg-error "invalid access to non-static" }
> -};
> +};                           // { dg-error "offsetof within non-standard-layout type" "" { target *-*-* } .-1 }
> --- gcc/testsuite/g++.dg/other/offsetof9.C.jj   2017-01-24 14:44:28.447867139 +0100
> +++ gcc/testsuite/g++.dg/other/offsetof9.C      2017-01-24 14:43:04.000000000 +0100
> @@ -0,0 +1,17 @@
> +// PR c++/68727
> +// { dg-do compile }
> +// { dg-options "-Winvalid-offsetof" }
> +
> +struct A { int i; };
> +struct B : virtual A { };
> +__SIZE_TYPE__ s = __builtin_offsetof (B, A::i);        // { dg-warning "offsetof within non-standard-layout type" }
> +
> +template <typename T>
> +__SIZE_TYPE__
> +foo ()
> +{
> +  return __builtin_offsetof (T, A::i)          // { dg-warning "offsetof within non-standard-layout type" }
> +        + __builtin_offsetof (B, A::i);        // { dg-warning "offsetof within non-standard-layout type" }
> +}
> +
> +__SIZE_TYPE__ t = foo<B> ();
>
>         Jakub