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: [PATCH v2] Bug 17394: we cannot put a break-point at a global function for ASM file


Hello Joel,

Thank you very much for your help and guidance.
In the future I will do as you told me and I hope everything will go smoothly.

Thank you again,
Mihai

-----Original Message-----
From: Joel Brobecker [mailto:brobecker@adacore.com] 
Sent: Saturday, December 20, 2014 6:33 PM
To: Nistor Mihail-MNISTOR1
Cc: Keith Seitz; gdb-patches@sourceware.org
Subject: Re: [PATCH v2] Bug 17394: we cannot put a break-point at a global function for ASM file

On Mon, Dec 15, 2014 at 08:47:19PM +0000, mihail.nistor@freescale.com wrote:
> Thank you very much for your support.
> 
> You will find enclosed a new patch.
[...]
> > gdb/ChangeLog:
> > 2014-09-30  Keith Seitz  <keiths@redhat.com>
> > 	    Mihail-Marian Nistor  <mihail.nistor@freescale.com>
> > 
> > 	PR gdb/17394
> > 	* linespec.c (struct collect_minsyms): Add new member `symtab'.
> > 	(add_minsym): Handle cases where info.symtab is non-NULL.
> > 	(search_minsyms_for_name): Add new parameter `symtab'.
> > 	Handle limiting searches to a specific symtab.
> > 	(add_matching_symtabs_to_info): Search through minimal symbols
> > 	for language_asm files for which no new symbols are found.
> > 
> > gdb/testsuite/ChangeLog:
> > 2014-09-30  Mihail-Marian Nistor  <mihail.nistor@freescale.com>
> > 
> >       PR gdb/17394
> >       * gdb.linespec/break-asm-file.c: New file.
> >       * gdb.linespec/break-asm-file.exp: New file.
> >       * gdb.linespec/break-asm-file0.s: New file.
> >       * gdb.linespec/break-asm-file1.s: New file.

Thank you. The patch is approved.

I have a few small additional remarks that came up following the
review:

The most important one that I almost tripped over, is the fact that the patch no longer builds on today's master due to a change in struct symtab. This goes to show that, when one asks for the patch to be rebased, we should just go ahead and do not just that, but also re-test it, even if the file that it touches hasn't changed in the interim. I've made the required changes and re-done testing this time around.

In the future, it would really help if you could send the commits to us, rather than just the diff. What we're interesting in, in addition to the diff, is the commit's revision log. It should contain a detailed description of the problem you're trying to solve, and how you're solving it (usually, we try to put the "why"
in the code). We try to do that at every iteration of the review, so we can keep an eye on the contents of the revision log and make sure it's complete and accurate. And another advantage is that it allows me to push your patch in under 10 seconds rather than having to apply a diff, make sure I don't miss some changes along the way, commit, write the revision log, get your email address (as commit author), etc.

Since there is a PR and testcase, and this patch has been delayed quite a bit for lack of review, I will take care of creating the commit this time.

Attached is the commit I just pushed.

--
Joel


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