This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Tue, 2018-12-18 at 15:40 -0500, Jason Merrill wrote:
> 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.

Thanks.

I've now committed the v6 patch, and the follow-ups that were already
approved (having rebased and retested them):

r267272:
  "C++: more location wrapper nodes (PR c++/43064, PR c++/43486)"

r267273:
  "C++: improvements to binary operator diagnostics (PR c++/87504)"

r267276:
  "C++: better locations for bogus initializations (PR c++/88375)"

Dave


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]