This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB 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: [RFA 1/2] Linespec rewrite (update 2)


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


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