Fix/gdbstub continuation crash #1671

mikee47 · 2019-04-13T16:56:13Z

When a syscall is being processed, especially a lengthy one like reading from the console, a regular debug break or exception takes us into the debugger command loop but GDB doesn't know we've stopped (it ignores our 'T' message). Then, when the 'F' response (which completes the syscall) is received, it takes us back out of the debugger loop immediately, which is wrong: we should instead remain within the debugger loop and tell GDB why we've stopped so things can proceed as for a regular break handling.

When testing with a breakpoint in a regular system timer the stub hangs during the gdbstub_icount_ena_single_step() call. This has been fixed by raising the interrupt level before setting up the count registers. The queueBreak command has been added to test this particular code path.

See #1670 for discussion.

Also:

Define SimpleTimerCallback - used in original GDB stub PR but this change was omitted
Improve gdbstub debugging
Add missing return statements in LiveDebug sample
Simplify code now that m_printf supports flash strings directly
Add queueBreak command

slaff

@mikee47 Thanks a lot for your fantastic work on the debugger! I was able to have proper debug sessions and debug areas that were impossible with the previous versions of GdbStub.
There are small changes that need to be addressed before merging this code. Mainly removing debug lines.See the comments in this review.

slaff · 2019-04-14T08:47:05Z

Sming/gdb/gdbcmds

@@ -1,5 +1,5 @@
 # Enable this if you want to log all traffic between GDB and the stub
-# set remotelogfile gdb_rsp_logfile.txt
+set remotelogfile gdb_rsp_logfile.txt


Please, make this line commented out by default.

slaff · 2019-04-14T08:50:09Z

Sming/gdb/gdbstub-cfg.h

@@ -178,7 +178,7 @@
 * Set to 0 to wait indefinitely.
 */
 #ifndef GDBSTUB_UART_READ_TIMEOUT
-#define GDBSTUB_UART_READ_TIMEOUT 2000
+#define GDBSTUB_UART_READ_TIMEOUT 0
 #endif



Please, make GDBSTUB_ENABLE_SYSCALL to be 0 by default. This will help visual debuggers behave as expected. Allow also that setting to be changed during compilation so that adventurous developers can try it.

To keep the instructions simpler, how about we do this?

To build for normal (command-line GDB) use: make flash
To build for eclipse: make flash ENABLE_CONSOLE=0

The problem is specific to the LiveDebug sample.

Let's use ENABLE_SYS_CONSOLE instead of ENABLE_CONSOLE. In addition I would prefer the syscall stuff to be disabled by default. For the LiveSample we can enable it by default by writing in the Makefile-user.mk.
When ENABLE_SYS_CONSOLE is enabled a warning message should be shown to inform the developers that this might interfere with visual debuggers, like Eclipse. And that it can be disabled using make ENABLE_SYS_CONSOLE=0.
What do you say?

Or even better ENABLE_GDB_CONSOLE instead of ENABLE_CONSOLE. The name prompts that this is related to the special GDB console.

Allow also that setting to be changed during compilation so that adventurous developers can try it.

The simplest way is to just edit the gdbstub-cfg.h file directly to make changes as required (same goes for gdbcmds). I also have a note in the readme.md about using USER_CFLAGS to set these values for a particular project - I've done this for the LiveDebug sample as per your suggestions.

Are you propsing we add the abillty to change these directly from the make command line?

Are you proposing we add the ability to change these directly from the make command line?

Ok, I see. Then can you do the the following?

GDBSTUB_ENABLE_SYSCALL=0 in Sming/gdb/gdbstub-cfg.h

In samples/LiveDebug/Makefile-user.mk add

# Comment the line below if you want to disable the GDB system calls USER_CFLAGS += -DGDBSTUB_ENABLE_SYSCALL=1

Update the README in LiveDebug to mention that change.

slaff · 2019-04-14T08:51:11Z

Sming/gdb/gdbstub.cpp

 	 */
 	case 'F':
+		//		if(gdb_syscall_complete(data)) {


What about the commented code? Can you remove it?

Sming/gdb/gdbstub.cpp

slaff · 2019-04-14T08:55:23Z

samples/LiveDebug/app/application.cpp

@@ -268,7 +268,8 @@ void fileStat(const char* filename)
 	XX(ls, "Use `syscall_system` function to perform a directory listing on the host")                                 \
 	XX(time, "Use `syscall_gettimeofday` to get current time from host")                                               \
 	XX(log, "Show state of log file")                                                                                  \
-	XX(break, "Demonstrated `gdb_do_break()` function to pause this application and obtain a GDB command prompt")      \
+	XX(break, "Demonstrate `gdb_do_break()` function to pause this application and obtain a GDB command prompt")       \
+	XX(queueBreak, "Demonstrate `gdb_do_break()` function called via task queue")                                      \
 	XX(hang, "Enter infinite loop to force a watchdog timeout")                                                        \
 	XX(read0, "Read from invalid address")                                                                             \
 	XX(write0, "Write to invalid address")                                                                             \


A side note here for the exit command: I guess my expectations here are incorrect but I was hoping that exit will allow me to continue to control the debug process. What I am seeing instead is:

(Attached) exit 300252092 gdb_read_console() returned 5 Calling gdb_detach() - resuming normal program execution. [Inferior 1 (Remote target) exited normally] (gdb) The program is not being run.

And I have to restart the debugging session in order to do anything more. If that is what was expected may be the short description of the exit command should be changed to reflect that.

It's probably better renamed as detach. Was your expectation that it behaves like break ?

A... is that the "disconnect" function in Eclipse. If yes just rename it to disconnect with short description Terminates the connection between the debugger and the remote debug target.

mikee47 · 2019-04-14T09:46:37Z

@slaff Are you able to get eclipse to show any of the debug output from an application? i.e. stuff from Serial.print or debug_* or m_printf ?

slaff · 2019-04-14T10:02:11Z

Are you able to get eclipse to show any of the debug output from an application

Not really. I've tried with LiveDebug and with Basic_Serial . Which is strange because I see the output using the cli gdb.

mikee47 · 2019-04-14T11:12:14Z

Are you able to get eclipse to show any of the debug output from an application

Not really. I've tried with LiveDebug and with Basic_Serial . Which is strange because I see the output using the cli gdb.

Me either. I've added it to the known issues section in readme.md.

Previously observed problem with `LEVEL` interrupt when using gdbstub in debug mode was probably because there was no check for invalid register number. It appears to work without issue now.

Note: Missing return value should _always_ be reported as error; the only reason it isn't at present is because Library builds will fail.

* Set uart read timeout to 0 by default - used during testing but isn't necessary for normal use * Only optimize stub code in non-debug mode

Used in original GDB stub PR but this change was omitted

When a syscall is being processed, especially a lengthy one like reading from the console, a regular debug break or exception takes us into the debugger command loop but GDB doesn't know we've stopped (it ignores our 'T' message). Then, when the 'F' response (which completes the syscall) is received, it takes us back out of the debugger loop immediately, which is wrong; at that point we should tell GDB again why we've stopped so things can proceed as for a regular break handling. ONGOING: When testing with a breakpoint in a regular system timer the stub hangs during the `gdbstub_icount_ena_single_step()` call - this has been temporarily highlighted with the !! debug statements.

…nt_ena_single_step()`

* Comment out `remotelogfile` setting in gdbcmds * Disable system calls by default * Remove commented-out code * Update `readme.md`

slaff · 2019-04-14T11:18:46Z

This branch has conflicts that must be resolved

Make sure to rebase your branch on the latest develop. See "Rebase if needed" in CONTRIBUTING.md

* Rename `exit` command to `disconnect * Add make option `ENABLE_GDB_CONSOLE` * Rename `state` to `ledState` * Revise command map to include more detail in the description, and improve the layout of `help` * Add `consoleOff` command * Move forward declarations to top of file * Update `README.md`

…esn't work

slaff · 2019-04-14T12:03:15Z

@mikee47 All looks good to me. Is the PR ready to merge or you would like to add some small changes?

* Move source symbols into separate `gdb\symbols` directory * Provide separate rule to copy symbol files into application folder

mikee47 · 2019-04-14T12:55:26Z

@slaff That's it ~~all done~~

One last point - I'm not sure that gdb_do_break() needs to be a function. If it were defined as a macro (only when ENABLE_GDB=1) the break would be more relevant.

(I think this is a hangover from having gdbstub compiled into the framework.)

mikee47 · 2019-04-14T13:40:38Z

@slaff OK, ready to merge. Thanks for your help testing this!

slaff · 2019-04-14T15:08:00Z

Thanks for your help testing this!

@mikee47 Thank you for your hard work!

mikee47 changed the title ~~[WIP] Fix/gdbstub continuation crash~~ Fix/gdbstub continuation crash Apr 13, 2019

slaff requested changes Apr 14, 2019

View reviewed changes

slaff added this to the 3.8.0 milestone Apr 14, 2019

slaff added the 3 - Review label Apr 14, 2019

mikee47 added 9 commits April 14, 2019 12:16

Fix warnings in STRICT mode

0e294b1

Simplify code now that m_printf supports flash strings directly

8eebece

Previously observed problem with `LEVEL` interrupt when using gdbstub in debug mode was probably because there was no check for invalid register number. It appears to work without issue now.

Add missing return statements in LiveDebug sample

5d0a992

Note: Missing return value should _always_ be reported as error; the only reason it isn't at present is because Library builds will fail.

Improve gdbstub debugging

854d393

* Set uart read timeout to 0 by default - used during testing but isn't necessary for normal use * Only optimize stub code in non-debug mode

Define SimpleTimerCallback

244123f

Used in original GDB stub PR but this change was omitted

Add queueBreak command to check problem with stepping

12cf0b9

Fix single-stepping issue by raising interrupt level in `gdbstub_icou…

f5da3f9

…nt_ena_single_step()`

Updates to gdbstub code following review

54bb3c4

* Comment out `remotelogfile` setting in gdbcmds * Disable system calls by default * Remove commented-out code * Update `readme.md`

mikee47 added 2 commits April 14, 2019 12:21

Remove GDBSTUB_PAUSE_HARDWARE_TIMER option and related code - it do…

5a70f09

…esn't work

mikee47 force-pushed the fix/gdbstub_continuation_crash branch from 426a2b1 to 5a70f09 Compare April 14, 2019 11:23

slaff approved these changes Apr 14, 2019

View reviewed changes

Copy symbol files only when ENABLE_GDB=1

0860d42

* Move source symbols into separate `gdb\symbols` directory * Provide separate rule to copy symbol files into application folder

mikee47 mentioned this pull request Apr 14, 2019

Fix 'corrupt stack?' warning in GDB backtrace #1665

Merged

Check build in STRICT mode

a800a42

Redefine gdb_do_break() as macro

e422d03

slaff merged commit eddd414 into SmingHub:develop Apr 14, 2019

slaff mentioned this pull request Apr 14, 2019

Debugger crashes after continuation #1670

Closed

mikee47 deleted the fix/gdbstub_continuation_crash branch April 14, 2019 16:12

slaff removed the 3 - Review label Jun 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/gdbstub continuation crash #1671

Fix/gdbstub continuation crash #1671

mikee47 commented Apr 13, 2019 •

edited

Loading

slaff left a comment

slaff Apr 14, 2019

slaff Apr 14, 2019

mikee47 Apr 14, 2019 •

edited

Loading

slaff Apr 14, 2019

slaff Apr 14, 2019

mikee47 Apr 14, 2019

slaff Apr 14, 2019

slaff Apr 14, 2019

slaff Apr 14, 2019

mikee47 Apr 14, 2019

slaff Apr 14, 2019

mikee47 commented Apr 14, 2019

slaff commented Apr 14, 2019 •

edited

Loading

mikee47 commented Apr 14, 2019

slaff commented Apr 14, 2019

slaff commented Apr 14, 2019

mikee47 commented Apr 14, 2019 •

edited

Loading

mikee47 commented Apr 14, 2019

slaff commented Apr 14, 2019

Fix/gdbstub continuation crash #1671

Fix/gdbstub continuation crash #1671

Conversation

mikee47 commented Apr 13, 2019 • edited Loading

slaff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikee47 Apr 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikee47 commented Apr 14, 2019

slaff commented Apr 14, 2019 • edited Loading

mikee47 commented Apr 14, 2019

slaff commented Apr 14, 2019

slaff commented Apr 14, 2019

mikee47 commented Apr 14, 2019 • edited Loading

mikee47 commented Apr 14, 2019

slaff commented Apr 14, 2019

mikee47 commented Apr 13, 2019 •

edited

Loading

mikee47 Apr 14, 2019 •

edited

Loading

slaff commented Apr 14, 2019 •

edited

Loading

mikee47 commented Apr 14, 2019 •

edited

Loading