[RFA] Cleanup: Use add_sal_to_sals for expressions

[RFA] Cleanup: Use add_sal_to_sals for expressions

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

Hi,

Currently convert_linespec_to_sals treats expressions a little 
differently than other linespec conversions. It allocates a SaL, fills 
it in, and then relies on decode_line_full to fill in the "missing" 
canonical name.

This patch causes expressions to use add_sal_to_sals, which will then 
set a canonical name for the linespec, thereby negating the need to 
double-check for missing linespecs later (in decode_line_full).

[This would not be committed until after the 7.5 branch is cut. While I 
haven't run into the new assert, I would feel better with more 
people/platforms using this before pushing this to the public.]

Keith

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

	* linespec.c (add_sal_to_sals): Use SYMNAME as
	canonical if it is non-NULL and no symtab is set in
	the SaL.
	(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.

[-- Attachment #2: use-add_sal_to_sals-for-expressions.patch --]
[-- Type: text/x-patch, Size: 1951 bytes --]

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 4156694..91cacb5 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -842,6 +842,8 @@ add_sal_to_sals (struct linespec_state *self,
 	  else
 	    canonical_name = xstrprintf ("%s:%d", filename, sal->line);
 	}
+      else if (symname != NULL)
+	canonical_name = xstrdup (symname);
 
       self->canonical_names[sals->nelts - 1] = canonical_name;
     }
@@ -1837,13 +1839,14 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
 
   if (ls->expression != NULL)
     {
+      struct symtab_and_line sal;
+
       /* We have an expression.  No other attribute is allowed.  */
-      sals.sals = XMALLOC (struct symtab_and_line);
-      sals.nelts = 1;
-      sals.sals[0] = find_pc_line (ls->expr_pc, 0);
-      sals.sals[0].pc = ls->expr_pc;
-      sals.sals[0].section = find_pc_overlay (ls->expr_pc);
-      sals.sals[0].explicit_pc = 1;
+      sal = find_pc_line (ls->expr_pc, 0);
+      sal.pc = ls->expr_pc;
+      sal.section = find_pc_overlay (ls->expr_pc);
+      sal.explicit_pc = 1;
+      add_sal_to_sals (state, &sals, &sal, ls->expression);
     }
   else if (ls->labels.label_symbols != NULL)
     {
@@ -2284,19 +2287,15 @@ decode_line_full (char **argptr, int flags,
   gdb_assert (canonical->addr_string != NULL);
   canonical->pre_expanded = 1;
 
-  /* Fill in the missing canonical names.  */
+  /* Arrange for allocated canonical names to be freed.  */
   if (result.nelts > 0)
     {
       int i;
 
-      if (state->canonical_names == NULL)
-	state->canonical_names = xcalloc (result.nelts, sizeof (char *));
       make_cleanup (xfree, state->canonical_names);
       for (i = 0; i < result.nelts; ++i)
 	{
-	  if (state->canonical_names[i] == NULL)
-	    state->canonical_names[i] = savestring (arg_start,
-						    *argptr - arg_start);
+	  gdb_assert (state->canonical_names[i] != NULL);
 	  make_cleanup (xfree, state->canonical_names[i]);
 	}
     }

Re: [RFA] Cleanup: Use add_sal_to_sals for expressions

[Tom Tromey]

>>>>> "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.

Tom

Re: [RFA] Cleanup: Use add_sal_to_sals for expressions

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]

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.


[-- Attachment #2: use-add_sal_to_sals-for-expressions-2.patch --]
[-- Type: text/x-patch, Size: 4746 bytes --]

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 4156694..4c9c110 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -810,13 +810,16 @@ add_sal_to_sals_basic (struct symtabs_and_lines *sals,
 
 /* Add SAL to SALS, and also update SELF->CANONICAL_NAMES to reflect
    the new sal, if needed.  If not NULL, SYMNAME is the name of the
-   symbol to use when constructing the new canonical name.  */
+   symbol to use when constructing the new canonical name.
+
+   If LITERAL_CANONICAL is non-zero, SYMNAME will be used as the
+   canonical name for the SAL.  */
 
 static void
 add_sal_to_sals (struct linespec_state *self,
 		 struct symtabs_and_lines *sals,
 		 struct symtab_and_line *sal,
-		 const char *symname)
+		 const char *symname, int literal_canonical)
 {
   add_sal_to_sals_basic (sals, sal);
 
@@ -826,7 +829,7 @@ add_sal_to_sals (struct linespec_state *self,
 
       self->canonical_names = xrealloc (self->canonical_names,
 					sals->nelts * sizeof (char *));
-      if (sal->symtab && sal->symtab->filename)
+      if (!literal_canonical && sal->symtab && sal->symtab->filename)
 	{
 	  char *filename = sal->symtab->filename;
 
@@ -842,6 +845,8 @@ add_sal_to_sals (struct linespec_state *self,
 	  else
 	    canonical_name = xstrprintf ("%s:%d", filename, sal->line);
 	}
+      else if (symname != NULL)
+	canonical_name = xstrdup (symname);
 
       self->canonical_names[sals->nelts - 1] = canonical_name;
     }
@@ -1809,7 +1814,7 @@ create_sals_line_offset (struct linespec_state *self,
 	       found.  */
 	    intermediate_results.sals[i].line = val.line;
 	    add_sal_to_sals (self, &values, &intermediate_results.sals[i],
-			     sym ? SYMBOL_NATURAL_NAME (sym) : NULL);
+			     sym ? SYMBOL_NATURAL_NAME (sym) : NULL, 0);
 	  }
 
       do_cleanups (cleanup);
@@ -1837,13 +1842,14 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
 
   if (ls->expression != NULL)
     {
+      struct symtab_and_line sal;
+
       /* We have an expression.  No other attribute is allowed.  */
-      sals.sals = XMALLOC (struct symtab_and_line);
-      sals.nelts = 1;
-      sals.sals[0] = find_pc_line (ls->expr_pc, 0);
-      sals.sals[0].pc = ls->expr_pc;
-      sals.sals[0].section = find_pc_overlay (ls->expr_pc);
-      sals.sals[0].explicit_pc = 1;
+      sal = find_pc_line (ls->expr_pc, 0);
+      sal.pc = ls->expr_pc;
+      sal.section = find_pc_overlay (ls->expr_pc);
+      sal.explicit_pc = 1;
+      add_sal_to_sals (state, &sals, &sal, ls->expression, 1);
     }
   else if (ls->labels.label_symbols != NULL)
     {
@@ -1856,7 +1862,7 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
 	{
 	  symbol_to_sal (&sal, state->funfirstline, sym);
 	  add_sal_to_sals (state, &sals, &sal,
-			   SYMBOL_NATURAL_NAME (sym));
+			   SYMBOL_NATURAL_NAME (sym), 0);
 	}
     }
   else if (ls->function_symbols != NULL || ls->minimal_symbols != NULL)
@@ -1882,7 +1888,8 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
 	      set_current_program_space (pspace);
 	      symbol_to_sal (&sal, state->funfirstline, sym);
 	      if (maybe_add_address (state->addr_set, pspace, sal.pc))
-		add_sal_to_sals (state, &sals, &sal, SYMBOL_NATURAL_NAME (sym));
+		add_sal_to_sals (state, &sals, &sal,
+				 SYMBOL_NATURAL_NAME (sym), 0);
 	    }
 	}
 
@@ -2284,19 +2291,15 @@ decode_line_full (char **argptr, int flags,
   gdb_assert (canonical->addr_string != NULL);
   canonical->pre_expanded = 1;
 
-  /* Fill in the missing canonical names.  */
+  /* Arrange for allocated canonical names to be freed.  */
   if (result.nelts > 0)
     {
       int i;
 
-      if (state->canonical_names == NULL)
-	state->canonical_names = xcalloc (result.nelts, sizeof (char *));
       make_cleanup (xfree, state->canonical_names);
       for (i = 0; i < result.nelts; ++i)
 	{
-	  if (state->canonical_names[i] == NULL)
-	    state->canonical_names[i] = savestring (arg_start,
-						    *argptr - arg_start);
+	  gdb_assert (state->canonical_names[i] != NULL);
 	  make_cleanup (xfree, state->canonical_names[i]);
 	}
     }
@@ -3110,7 +3113,7 @@ decode_digits_list_mode (struct linespec_state *self,
       val.pc = 0;
       val.explicit_line = 1;
 
-      add_sal_to_sals (self, values, &val, NULL);
+      add_sal_to_sals (self, values, &val, NULL, 0);
     }
 }
 
@@ -3254,7 +3257,7 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
     skip_prologue_sal (&sal);
 
   if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc))
-    add_sal_to_sals (self, result, &sal, SYMBOL_NATURAL_NAME (msymbol));
+    add_sal_to_sals (self, result, &sal, SYMBOL_NATURAL_NAME (msymbol), 0);
 }
 
 /* A helper struct to pass some data through

Re: [RFA] Cleanup: Use add_sal_to_sals for expressions

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

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

Seems reasonable to me.

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

Ok.

Tom

Re: [RFA] Cleanup: Use add_sal_to_sals for expressions

[Keith Seitz]

On 07/18/2012 01:11 PM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
> Keith> So, what do you think of this revised patch? [No regressions on x86-64
> Keith> linux.]
>
> Ok.

Again, thank you, Tom, for taking a look.

This has been committed.

Keith