Re: [patch] diagnostics branch

[Aldy Hernandez <aldyh at redhat dot com>]

On Mon, Jun 08, 2009 at 07:26:56PM -0700, Richard Henderson wrote:
>> @@ -833,7 +833,8 @@ dw2_force_const_mem (rtx x, bool is_publ
>>   	  sprintf (ref_name, "DW.ref.%s", str);
>>  	  id = get_identifier (ref_name);
>> -	  decl = build_decl (VAR_DECL, id, ptr_type_node);
>> +	  decl = build_decl (input_location,
>> +			     VAR_DECL, id, ptr_type_node);
>>  	  DECL_ARTIFICIAL (decl) = 1;
>
> For the two changes in dwarf2asm.c, I prefer the UNKNOWN_LOCATION
> value that you use elsewhere, since these decls are totally fake.

Fixed.

> So what exactly is BUILTINS_LOCATION supposed to represent?  An
> actual built-in like __builtin_memcpy, or something that may also
> appear in a runtime library.
>
> If only real builtins, all these tinfo things are in libstdc++.  If
> external library stuff that implements builtins, then there's at
> least a half-dozen functions that are marked UNKNOWN_LOCATION.
>
> It appears as if BUILTINS_LOCATION is treated like UNKNOWN_LOCATION
> almost everywhere, except where != UNKNOWN_LOCATION, where we sometimes
> assign come "current" location.  This suggests that runtime library
> stuff should be considered BUILTIN.  Make sense?

I understood BUILTINS_LOCATION to be things like __builtin_memcpy and
other places where an error message should print "<built-in>".  But I
hadn't thought about things we reference in libstdc++ which perhaps
needs BUILTINS_LOCATION because they obviously aren't UNKNOWN_LOCATION.

I've attached BUILTINS_LOCATION to the tinfo stuff above and will make
it a point to use BUILTINS_LOCATION in anything in libstdc++ (and other
compiler generated variables).  Would that be acceptable?

> Seems like the both of these should use the same location.  This
> catches my eye because I would think that essentially no where in
> pt.c should use input_location, since almost all of it is template
> substitution and we should be copying someone else's location.

Fixed everything in pt.c.

>> @@ -3328,7 +3329,7 @@ finalize_nrv_r (tree* tp, int* walk_subt
>>  	init = build2 (INIT_EXPR, void_type_node, dp->result,
>>  		       DECL_INITIAL (dp->var));
>>        else
>> -	init = build_empty_stmt ();
>> +	init = build_empty_stmt (*EXPR_LOCUS (*tp));
>
> Is there a plan to remove EXPR_LOCUS in favour of EXPR_LOCATION,
> now that we've ditched the locus struct?  In particular, should
> you be adding new uses of EXPR_LOCUS?

It wasn't on my agenda, but perhaps we should remove it in the near
future.  The reason I used EXPR_LOCUS in the case above is because a few
lines down we used SET_EXPR_LOCUS, so I was keeping with the style.

>> @@ -169,7 +169,7 @@ lower_function_body (void)
>>        tree disp_label, disp_var, arg;
>>         /* Build 'DISP_LABEL:' and insert.  */
>> -      disp_label = create_artificial_label ();
>> +      disp_label = create_artificial_label (cfun->function_end_locus);
> ...
>>  lower_builtin_setjmp (gimple_stmt_iterator *gsi)
>>  {
>>    gimple stmt = gsi_stmt (*gsi);
>> -  tree cont_label = create_artificial_label ();
>> -  tree next_label = create_artificial_label ();
>> +  tree cont_label = create_artificial_label (UNKNOWN_LOCATION);
>> +  tree next_label = create_artificial_label (UNKNOWN_LOCATION);
>
> Surely these labels should be at the location of the setjmp call;
> after all, they're part of it's implementation.

Fixed.

> For future work, most of the labels created in tree-eh.c can be
> associated with the locus of some EH statement that we're expanding.
> But since UNKNOWN is not a regression from the no information that
> we were emitting before, I don't think this should hold up merging.

Fixed all of tree-eh.c.

> Similarly for most of gimplify.c.

However, this will have to wait.  This is a small can of worms.  The
entire gimplifier needs some loving wrt locations.  Currently, we just
take the parent tree and assign it's location to any tuples we create
from then until the next tree/statement.  What we should be doing is
using the location for the leaves (trees) when appropriate.

>> @@ -5533,7 +5580,8 @@ digest_init (tree type, tree init, tree  	    {
>>  	      if (TREE_CODE (inside_init) == STRING_CST
>>  		  || TREE_CODE (inside_init) == COMPOUND_LITERAL_EXPR)
>> -		inside_init = array_to_pointer_conversion (inside_init);
>> +		inside_init = array_to_pointer_conversion
>> +		  (input_location, inside_init);
>
> Are there any valid uses of input_location within digest_init,
> which is passed a location_t parameter?

Nope, fixed.

> Surely UNKNOWN (or BUILTIN) is better for most everything in
> coverage.c.  As far as I can see, most of the locations here are
> for the stuff that we use to communicate with gcov.

Fixed with BUILTINS_LOCATION.

>> +++ gcc/tree-switch-conversion.c	(working copy)
>> @@ -517,7 +517,7 @@ build_one_array (gimple swtch, int num,        ctor 
>> = build_constructor (array_type, info.constructors[num]);
>>        TREE_CONSTANT (ctor) = true;
>>  -      decl = build_decl (VAR_DECL, NULL_TREE, array_type);
>> +      decl = build_decl (input_location, VAR_DECL, NULL_TREE, array_type);
>
> Surely grep input_location tree-*.c should not match anything?
> In particular, this usage should be the gimple_location of the
> switch statement.

Fixed for all tree-*.c.  Well, I left a handful that were needed:

  1. In tree_rest_of_compilation() where we temporarily set input_location
  to the location of the FNDECL being compiled

  2. In tree-vrp.c where we use input_location if we don't have a
  location for a given stmt.  Ideally we should fix the incompletes STMT,
  but this was previously broken...

  I used BUILTINS_LOCATION for most of them.  This should ideally be
  fixed, but most of these places have no diagnostics associated with
  them.

  3. In the inliner where we save input_location while we do some
  transformations.  A few of the saved locations in the inliner can be
  removed, and I'd like to do that as soon as we merge.  There is some old
  code that assumes that walk_tree callbacks will clobber input_location,
  but at least with the callbacks in inline_forbidden_p (which I've fixed)
  I know it is not the case.

>>    if (c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>"))
>>      {
>> +      switch_cond_loc = c_parser_peek_token (parser)->location;
>>        expr = c_parser_expression (parser).value;
>
> Why did you need to record the switch condition explicitly, rather
> than examining EXPR_LOCATION (expr)?  There are lots of other places
> around the parser where you do similar things, so I assume there's
> a good reason but I can't think what it might be.

Imagine this:

	switch(foo) {
	}

EXPR in this case is the VAR_DECL for foo.  And we obviously don't want
the location for the declaration of foo, but for the condition "(" (or
thereabouts).

>> +  location_t loc = c_parser_peek_token (parser)->location;
>> +  location_t for_paren_loc = c_parser_peek_token (parser)->location;
>>    gcc_assert (c_parser_next_token_is_keyword (parser, RID_FOR));
>> -  loc = c_parser_peek_token (parser)->location;
>>    c_parser_consume_token (parser);
>
> for_paran_loc is incorrect, since it's looking at the FOR token.
> Though, really, what's the point of distingushing that token?

Yeah, it makes more sense to use the location of the FOR (or the entire
pragma in "#pragma omp for", since it's treated as one token IIRC).  It
now points to the FOR, and I've named the variable "for_loc".

Updated patch at http://quesejoda.com/diagnostics-patch

OK?

Aldy