

Other format: [Raw text]

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

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
+	 strip location wrappers here, since otherwise the
location of
+	 one function's default arguments is arbitrarily chosen
+	 all functions with similar signature (due to
+	 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
types with the same default argument but different location
shouldn't be combined.



I did some digging into this.  I added this strip to fix
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

7063	bool
7064	type_list_equal (const_tree l1, const_tree l2)
7065	{
7066	  const_tree t1, t2;
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
7074	      return false;
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 ==
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),
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
- should function-type hashing use a custom version of
when comparing params, and make different source locations of default
args be different?
- something else?


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.

