Re: [RFA 1/2] Linespec rewrite (update 2)

[Keith Seitz <keiths@redhat.com>]

On 03/28/2012 01:46 PM, Tom Tromey wrote:
> This patch is pretty hard to review.  I've been reading the branch
> instead, but even there I find it hard to wrap my head around it all.

Yeah, it is pretty massive. I apologize. I simply don't know any other 
way to submit this. [I'm all eyes/ears if you would like to see it in 
any other form/format.]

> Keith>  +const char * const linespec_keywords[] = { "if", "thread", "task" };
>
> Should also be static.

Yup. Fixed.

> Keith>  +      /* If we're in list mode, and the next token is a string beginning
> Keith>  +	 with ",", we're dealing with a ranged listing.  Stop parsing
> Keith>  +	 and return.  */
> Keith>  +      if (PARSER_STATE (parser)->list_mode
> Keith>  +	&&  token.type == LSTOKEN_STRING
> Keith>  +	&&  *LS_TOKEN_STOKEN (token).ptr == ',')
> Keith>  +	return;
>
> I thought that historically a top-level comma always terminated a
> linespec -- not just in list mode.  It should be possible to write a
> test case for this in Python pretty easily.

I don't know. The whole comma thing is undocumented. The test suite does 
contain list ranges. That's how I originally discovered this. I've 
removed the list mode restriction, though, and it doesn't affect test 
results at all.

One cannot "break 30,31,32" in either branch, so I can see no 
differences in functionality.

> Keith>  +static void
> Keith>  +canonicalize_linespec (struct linespec_state *state, linespec_t ls)
> [...]
> Keith>  +      if (ls->line_offset.sign != unknown)
> Keith>  +	{
> Keith>  +	  if (need_colon)
> Keith>  +	    fputc_unfiltered (':', buf);
> Keith>  +	  fprintf_filtered (buf, "%s%d",
> Keith>  +			    (ls->line_offset.sign == none ? ""
> Keith>  +			     : ls->line_offset.sign == plus ? "+" : "-"),
> Keith>  +			    ls->line_offset.offset);
>
> I am curious when this code can trigger.
> Can we end up with a canonical form like "function:+5"?
> I was hoping to reserve that syntax for a later addition; and anyway in
> general I think relative linespecs need to be made absolute by the
> canonicalization process, since otherwise re-setting won't do the right
> thing.

Yes, we can end up with a canonical form like "function:+5" or 
"file:+5". The former is permitted (per recent maintainer request) 
because we currently ignore the offset. [It is unprocessed in 
convert_linespec_to_sals.] I'm not a fan of this,

FYI CVS HEAD creates canonical linespecs like "file:+5" today, without 
converting to absolute offset. It also rejects "function:+5".

I haven't done anything with this. AFAICT, the linespecs are identical 
in both CVS HEAD and the branch. [Or would be if either CVS HEAD ignored 
function-relative offsets or we issued an error again in my branch.]

What would you like me to do?

> Keith>  +      /* We have an expression.  No other attribute is allowed.  */
>
> It would be helpful if the constraints on the fields of 'struct
> linespec' were documented there.

I'll add something. The reason this is here because this is no inherent 
limitation on expressions (or other members of struct linespec). We 
could permit file:*foo, if we wanted to, but we simply don't.

> Keith>  +	  pspace = elem->minsym->ginfo.obj_section->objfile->pspace;
>
> Should use SYMBOL_OBJ_SECTION.  I didn't audit for other instances.

Fixed. It was the only offender.

> Keith>  +  else if (ls->minimal_symbols != NULL)
> Keith>  +    {
> Keith>  +      /* We found minimal symbols, but no normal symbols.  */
> Keith>  +      int i;
> Keith>  +      minsym_and_objfile_d *elem;
> Keith>  +
> Keith>  +      for (i = 0;
> Keith>  +	   VEC_iterate (minsym_and_objfile_d, ls->minimal_symbols, i, elem);
> Keith>  +	   ++i)
> Keith>  +	minsym_found (state, elem->objfile, elem->minsym,&sals);
>
> Why are minsyms sorted by pspace in one branch but not another?

No real good reason, other than that is the way it is done today. I 
tried to keep the codepaths as similar as possible. I've merged the two 
branches together. No need for minsyms to be singled out like this.

> Keith>  +++ b/gdb/testsuite/gdb.cp/cplabel.exp
> [...]
> Keith>  +if {[prepare_for_testing "$testfile.exp" $testfile $srcfile]} {
> Keith>  +    return -1
>
> I suspect this needs a skip_cplus_tests check.

Indeed. It was also missing the c++ and debug flags for 
prepare_for_testing, which I've also added.

I've pushed the requested changes to my archer branch.

Thank you for the feedback.

Keith