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

[Jason Merrill <jason@redhat.com>]

On 12/17/18 6:30 PM, 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?

I'd experiment with removing the simple_cst_equal bit in 
type_list_equal.  But I think that can wait, and you can go ahead and 
commit the v5 patch.

Jason