Re: [patch] fix for PR2424

[Vladimir Prus <vladimir@codesourcery.com>]

On Monday 10 March 2008 17:29:05 Aleksandar Ristovski wrote:
> Vladimir Prus wrote:
> > The GDB output you have provided above actually includes thread id, so what problem
> > does CDT have with figuring thread? In fact, CDT4's RxThread.java has the following:
> > 
> > 		// We were stopped for some unknown reason, for example
> > 		// GDB for temporary breakpoints will not send the
> > 		// "reason" ??? still fire a stopped event.
> > 		if (list.isEmpty()) {
> > 			if (session.getMIInferior().isRunning()) {
> > 				session.getMIInferior().setSuspended();
> > 				MIEvent event = new MIStoppedEvent(session, rr);
> > 				session.fireEvent(event);
> > 			}
> > 		}
> > 
> >> in addition to  
> >> not knowing the reason (I am not working on CDT but I was explained that missing 
> >> "reason" is to blame, and after the patch I proposed I was told things now work 
> >> as expected).
> > 
> > So, could it be a CDT issue, after all?
> 
> I believe the problem is that all threads will be reported as suspended, but 
> there is no distinction between the thread that hit the breakpoint and other 
> threads suspended due to stop. Normally, ide will show something like 
> (Breakpoint-hit) beside "Suspended" next to thread number that hit the 
> breakpoint. But I will double check with people working on CDT.
> 
> > 
> >> @Nick: I think the breakpoint should be reported. The fact that it is temporary 
> >> doesn't make it much different than a regular breakpoint... but maybe I'm 
> >> missing something.
> > 
> > Independent of actually CDT issue, I still think accurately reporting stop reason
> > would be good. Can we probably look at breakpoints 'disp' field and either
> > print "Breakpoint" or "Temporary breakpoint", and likewise either "breakpoint-hit" or
> > "temporary-breakpoint-hit", in breakpoint.c:print_it_typical?
> 
> I made the changes, it is not a problem. I have, however, made two versions. The 
> first as suggested by Vladimir, the second slightly different but with the same 
> goal.
> 
> ------------- version 1 -------------------
> CLI:
> 
> (gdb) tbreak main
> Temporary breakpoint 1 at 0x80483a0: file ./main.c, line 15.
> (gdb) r
> Starting program: /space/src/testcases/sigsegv/main
> 
> Temporary breakpoint 1, main () at ./main.c:15
> 15        foo (p);
> 
> MI:
> (gdb)
> -break-insert -t main
> ^done,bkpt={number="1",type="breakpoint",disp="del",enabled="y",addr="0x080483a0",func="main",file="./main.c",fullname="/space/src/testcases/sigsegv/main.c",line="15",times="0"}
> (gdb)
> -exec-run
> ^running
> (gdb)
> *stopped,reason="temporary-breakpoint-hit",bkptno="1",thread-id="0",frame={addr="0x080483a0",func="main",args=[],file="./main.c",fullname="/space/src/testcases/sigsegv/main.c",line="15"}
> ---------------------------------------------
> 
> But now that I am sifting through testcases to replace all "Breakpoint..." with 
> "Temporary breakpoint..." I am thinking: we need to communicate to the user that 
> the breakpoint is of temporary nature, but should not introduce new "look" or 
> type for the temporary breakpoint. The same applies for the reason, it should 
> still be "breakpoint-hit". 

I think that for CLI, it's better to say "Temporary breakpoint hit", since "Temporary"
is an important bit of information, and the message gdb print is all that user sees.
But whatever CLI output that is acceptable to others is fine with me :-)

> This is particularly true for MI. 
> 
> How about this output:
> *stopped,reason="breakpoint-hit",disp="del"
> ---------------------------------------------------------
> 
> I think the second version would introduce less impact to existing frontends 
> while clarifying the output in a more consistent way.

I think this is good. You did not attach your patches, so I cannot comment on
the code itself.

Also, since this code touches infrun.c and affects CLI, I think some of the
global maintainers will have to approve it.

- Volodya