On 07/18/2012 09:34 AM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
>
> Keith> This patch causes expressions to use add_sal_to_sals, which will then
> Keith> set a canonical name for the linespec, thereby negating the need to
> Keith> double-check for missing linespecs later (in decode_line_full).
>
> Keith> @@ -842,6 +842,8 @@ add_sal_to_sals (struct linespec_state *self,
> Keith>  	  else
> Keith>  	    canonical_name = xstrprintf ("%s:%d", filename, sal->line);
> Keith>  	}
> Keith> +      else if (symname != NULL)
> Keith> +	canonical_name = xstrdup (symname);
>
> [...]
>
> Keith> +      sal = find_pc_line (ls->expr_pc, 0);
> Keith> +      sal.pc = ls->expr_pc;
> Keith> +      sal.section = find_pc_overlay (ls->expr_pc);
> Keith> +      sal.explicit_pc = 1;
> Keith> +      add_sal_to_sals (state, &sals, &sal, ls->expression);
>
> It seems to me that find_pc_line can return a sal with symtab set;
> search for "val.symtab = " in find_pc_sect_line.  In this case it seems
> that you'd wind up with the wrong canonical form.

Yes, you are correct. [As if you didn't know that already!]

As much as I hate to extend the API for add_sal_to_sals (especially for 
a special case like this), but I really want [to try?] to isolate name 
canonicalization in one place, and add_sal_to_sals seems like the best 
candidate for that.

So, what do you think of this revised patch? [No regressions on x86-64 
linux.]

Keith

ChangeLog

2012-07-18  Keith Seitz  <keiths@redhat.com>

	* linespec.c (add_sal_to_sals): Add LITERAL_CANONICAL
	parameter.  If non-zero, use SYMNAME as the canonical name
	for the SaL.
	Update all callers.
	(convert_linespec_to_sals): Use add_sal_to_sals for
	expressions, too.
	(decode_line_full): No need to "fill in missing canonical names"
	anymore. Simply make cleanups for the allocated names.