From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17752 invoked by alias); 29 Mar 2012 19:18:45 -0000 Received: (qmail 17741 invoked by uid 22791); 29 Mar 2012 19:18:43 -0000 X-SWARE-Spam-Status: No, hits=-5.6 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 29 Mar 2012 19:18:30 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q2TJIUPO005192 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 29 Mar 2012 15:18:30 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q2TJIRDL001711 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 29 Mar 2012 15:18:29 -0400 Message-ID: <4F74B583.6090008@redhat.com> Date: Thu, 29 Mar 2012 19:18:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.1) Gecko/20120209 Thunderbird/10.0.1 MIME-Version: 1.0 To: Tom Tromey CC: "gdb-patches@sourceware.org ml" Subject: Re: [RFA 1/2] Linespec rewrite (update 2) References: <4F70F8F7.503@redhat.com> <87zkb0wj1x.fsf@fleche.redhat.com> In-Reply-To: <87zkb0wj1x.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-03/txt/msg01014.txt.bz2 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