Re: [v2 of PATCH 03/14] C++: add location_t wrapper nodes during parsing (minimal impl)

[David Malcolm <dmalcolm at redhat dot com>]

On Fri, 2018-01-05 at 15:29 -0500, Jason Merrill wrote:
> On 12/29/2017 12:06 PM, David Malcolm wrote:
> > One issue I ran into was that fold_for_warn doesn't eliminate
> > location wrappers when processing_template_decl, leading to
> > failures of the template-based cases in
> > g++.dg/warn/Wmemset-transposed-args-1.C.
> > 
> > This is due to the early bailout when processing_template_decl
> > within cp_fold:
> > 
> > 2078	  if (processing_template_decl
> > 2079	      || (EXPR_P (x) && (!TREE_TYPE (x) || TREE_TYPE
> > (x) == error_mark_node)))
> > 2080	    return x;
> > 
> > which dates back to the merger of the C++ delayed folding branch.
> > 
> > I've fixed that in this version of the patch by removing that
> > "processing_template_decl ||" condition from that cp_fold early
> > bailout.
> 
> Hmm, that makes me nervous.  We might want to fold in templates when 
> called from fold_for_warn, but not for other occurrences.  But I see 
> that we check processing_template_decl in cp_fully_fold and in the
> call 
> to cp_fold_function, so I guess this is fine.

(I wondered if it would be possible to add a new flag to the various
fold* calls to request folding in templates, but given that the API is
partially shared with C doing so doesn't seem to make sense)

> > +    case VIEW_CONVERT_EXPR:
> > +    case NON_LVALUE_EXPR:
> >      case CAST_EXPR:
> >      case REINTERPRET_CAST_EXPR:
> >      case CONST_CAST_EXPR:
> > @@ -14937,6 +14940,15 @@ tsubst_copy (tree t, tree args,
> > tsubst_flags_t complain, tree in_decl)
> >      case CONVERT_EXPR:
> >      case NOP_EXPR:
> >        {
> > +	if (location_wrapper_p (t))
> > +	  {
> > +	    /* Handle location wrappers by substituting the
> > wrapped node
> > +	       first, *then* reusing the resulting type.  Doing
> > the type
> > +	       first ensures that we handle template parameters
> > and
> > +	       parameter pack expansions.  */
> > +	    tree op0 = tsubst_copy (TREE_OPERAND (t, 0), args,
> > complain, in_decl);
> > +	    return build1 (code, TREE_TYPE (op0), op0);
> > +	  }
> 
> I'd rather handle location wrappers separately, and abort if 
> VIEW_CONVERT_EXPR or NON_LVALUE_EXPR appear other than as wrappers.

OK.  I'm testing an updated version which does so.

> > @@ -24998,6 +25018,8 @@ build_non_dependent_expr (tree expr)
> >        && !expanding_concept ())
> >      fold_non_dependent_expr (expr);
> >  
> > +  STRIP_ANY_LOCATION_WRAPPER (expr);
> 
> Why is this needed?

I added this to fix a failure:

template argument deduction/substitution failed:
note:   cannot convert ‘0’ (type ‘int’) to type ‘char**’

seen building the precompiled header, where build_non_dependent_expr
adds a NON_DEPENDENT_EXPR around a location wrapper around an
INTEGER_CST, whereas without location wrappers we'd just have an
INTEGER_CST.

build_non_dependent_expr performs various tests for exact TREE_CODEs,
special-casing constants; without the stripping these TREE_CODE tests
don't match when the constants have location wrappers, leading to the
failure.

Here are the gory details:

In file included from ./src/libstdc++-
v3/include/precompiled/extc++.h:82:
./build/x86_64-pc-linux-gnu/libstdc++-
v3/include/ext/codecvt_specializations.h: In member function ‘virtual
std::codecvt_base::result std::codecvt<_InternT, _ExternT,
__gnu_cxx::__cxx11::encoding_state>::do_unshift(std::codecvt<_InternT,
_ExternT, __gnu_cxx::__cxx11::encoding_state>::state_type&,
std::codecvt<_InternT, _ExternT,
__gnu_cxx::__cxx11::encoding_state>::extern_type*,
std::codecvt<_InternT, _ExternT,
__gnu_cxx::__cxx11::encoding_state>::extern_type*,
std::codecvt<_InternT, _ExternT,
__gnu_cxx::__cxx11::encoding_state>::extern_type*&) const’:
./build/x86_64-pc-linux-gnu/libstdc++-
v3/include/ext/codecvt_specializations.h:392:58: error: no matching
function for call to ‘__iconv_adaptor(size_t (&)(iconv_t, char**,
size_t*, char**, size_t*), void* const&, int, int, char**,
std::size_t*)’
                                           &__cto, &__tlen);
                                                          ^
./build/x86_64-pc-linux-gnu/libstdc++-
v3/include/ext/codecvt_specializations.h:301:5: note: candidate:
‘template<class _Tp> std::size_t std::__iconv_adaptor(std::size_t
(*)(iconv_t, _Tp, std::size_t*, char**, std::size_t*), iconv_t, char**,
std::size_t*, char**, std::size_t*)’
     __iconv_adaptor(size_t(*__func)(iconv_t, _Tp, size_t*, char**,
size_t*),
     ^~~~~~~~~~~~~~~
./build/x86_64-pc-linux-gnu/libstdc++-
v3/include/ext/codecvt_specializations.h:301:5: note:   template
argument deduction/substitution failed:
./build/x86_64-pc-linux-gnu/libstdc++-
v3/include/ext/codecvt_specializations.h:392:58: note:   cannot convert
‘0’ (type ‘int’) to type ‘char**’
                                           &__cto, &__tlen);
                                                          ^



(gdb) call debug_tree (arg)
 <non_dependent_expr 0x7fffe7402820
    type <integer_type 0x7ffff0e4f5e8 int asm_written public type_6 SI
        size <integer_cst 0x7ffff0e520c0 constant 32>
        unit-size <integer_cst 0x7ffff0e520d8 constant 4>
        align:32 warn_if_not_align:0 symtab:-291372496 alias-set -1
canonical-type 0x7ffff0e4f5e8 precision:32 min <integer_cst
0x7ffff0e52078 -2147483648> max <integer_cst 0x7ffff0e52090 2147483647>
        pointer_to_this <pointer_type 0x7ffff0e57a80> reference_to_this
<reference_type 0x7fffee916e70>>
   
    arg:0 <non_lvalue_expr 0x7fffe7402720 type <integer_type
0x7ffff0e4f5e8 int>
        constant
        arg:0 <integer_cst 0x7ffff0e52210 constant 0>
        ./build/x86_64-pc-linux-gnu/libstdc++-
v3/include/ext/codecvt_specializations.h:391:50 start: ./build/x86_64-
pc-linux-gnu/libstdc++-v3/include/ext/codecvt_specializations.h:391:50
finish: ./build/x86_64-pc-linux-gnu/libstdc++-
v3/include/ext/codecvt_specializations.h:391:50>>


...when for a unpatched build we'd just have an INTEGER_CST.

Given the various TREE_CODE comparisons, using the
STRIP_ANY_LOCATION_WRAPPER macro seemed most appropriate; I put it
before the first TREE_CODE test.

I also added selftest::test_build_non_dependent_expr for this, to
verify that location wrappers don't affect the result of
build_non_dependent_expr (for integer constants and on string
literals).

Thanks
Dave