[PATCH] v6: C++: more location wrapper nodes (PR c++/43064, PR c++/43486)

[Jason Merrill]

On 12/18/18 4:22 PM, David Malcolm wrote:
> On Mon, 2018-12-17 at 18:30 -0500, David Malcolm wrote:
>> On Mon, 2018-12-17 at 14:33 -0500, Jason Merrill wrote:
>>> On 12/14/18 7:17 PM, David Malcolm wrote:
>>>> +      /* Since default args are effectively part of the function
>>>> type,
>>>> +	 strip location wrappers here, since otherwise the
>>>> location of
>>>> +	 one function's default arguments is arbitrarily chosen
>>>> for
>>>> +	 all functions with similar signature (due to
>>>> canonicalization
>>>> +	 of function types).  */
>>>
>>> Hmm, looking at this again, why would this happen?  I see that
>>> type_list_equal uses == to compare default arguments, so two
>>> function
>>> types with the same default argument but different location
>>> wrappers
>>> shouldn't be combined.
>>>
>>> Jason
>>
>> Thanks.
>>
>> I did some digging into this.  I added this strip to fix
>>    g++.dg/template/defarg6.C
>> but it looks like I was overzealous (the comment is correct, but it's
>> papering over a problem).
>>
>> It turns out that type_list_equal is doing more than just pointer
>> equality; it's hitting the simple_cst_equal part of the && at line
>> 7071:
>>
>> 7063	bool
>> 7064	type_list_equal (const_tree l1, const_tree l2)
>> 7065	{
>> 7066	  const_tree t1, t2;
>> 7067	
>> 7068	  for (t1 = l1, t2 = l2; t1 && t2; t1 = TREE_CHAIN (t1),
>> t2 = TREE_CHAIN (t2))
>> 7069	    if (TREE_VALUE (t1) != TREE_VALUE (t2)
>> 7070		|| (TREE_PURPOSE (t1) != TREE_PURPOSE (t2)
>> 7071		    && ! (1 == simple_cst_equal (TREE_PURPOSE
>> (t1), TREE_PURPOSE (t2))
>> 7072			  && (TREE_TYPE (TREE_PURPOSE (t1))
>> 7073			      == TREE_TYPE (TREE_PURPOSE
>> (t2))))))
>> 7074	      return false;
>> 7075	
>> 7076	  return t1 == t2;
>> 7077	}
>>
>> What's happening is that there are two different functions with
>> identical types apart from the locations of their (equal) default
>> arguments: both of the TREE_PURPOSEs are NON_LVALUE_EXPR wrappers
>> around a CONST_DECL enum value (at different source locations).
>>
>> simple_cst_equal is stripping the location wrappers here:
>>
>> 7311	  if (CONVERT_EXPR_CODE_P (code1) || code1 ==
>> NON_LVALUE_EXPR)
>> 7312	    {
>> 7313	      if (CONVERT_EXPR_CODE_P (code2)
>> 7314		  || code2 == NON_LVALUE_EXPR)
>> 7315		return simple_cst_equal (TREE_OPERAND (t1, 0),
>> TREE_OPERAND (t2, 0));
>> 7316	      else
>> 7317		return simple_cst_equal (TREE_OPERAND (t1, 0),
>> t2);
>> 7318	    }
>>
>> and thus finds them to be equal; the iteration in type_list_equal
>> continues, and runs out of parameters with t1 == t2 == NULL, and thus
>> returns true, and thus the two function types hash to the same slot,
>> and the two function types get treated as being the same.
>>
>> It's not clear to me yet what the best solution to this is:
>> - should simple_cst_equal regard different source locations as being
>> different?
>> - should function-type hashing use a custom version of
>> type_list_equal
>> when comparing params, and make different source locations of default
>> args be different?
>> - something else?
>>
>> Dave
> 
> I tried both of the above approaches, and both work.
> 
> Here's v6 of the patch:
> 
> I removed the strip of wrappers in cp_parser_late_parsing_default_args
> from earlier versions of the patch, in favor of fixing simple_cst_equal
> so that it treats location wrappers with unequal source locations as
> being unequal.  This ensures that function-types with default arguments
> don't get merged when the default argument constants have different
> spelling locations.  [I have an alternative patch which instead
> introduces a different comparator for FUNCTION_TYPE's TYPE_ARG_TYPES
> within type_cache_hasher::equal, almost identical to type_list_equal,
> but adding the requirement that  location wrappers around default
> arguments have equal source location for the params to be considered
> equal; both patches pass bootstrap&regression testing]
> 
> Doing so leads to the reported location for the bad default argument
> within a template in g++.dg/template/defarg6.C moving to the argument
> location.  Previously, the callsite of the instantiation was identified
> due to the use of input_location in convert_like_real here:
> 
> 6816	  location_t loc = cp_expr_loc_or_loc (expr, input_location);
> 
> With a location wrapper, it uses the spelling location of the
> default argument, but doesn't identify the location of the callsite
> that's instantiating the template.
> 
> So I moved the note in tsubst_default_argument about which callsite
> led to a diagnostic to after the check_default_argument call, so that
> diagnostics within that receive notes, too.
> 
> As before, this was successfully bootstrapped & regrtested on
> x86_64-pc-linux-gnu, in conjunction with the followup patch.
> 
> OK for trunk?

Ah, I hadn't seen this before my last email.  Let's go with this version.

Jason