Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GDB and exception handling improvements #1655

Merged
merged 50 commits into from
Apr 8, 2019

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Mar 28, 2019

Makefile changes:

  • Remove esp-gdbstub as third-party module
  • Remove all GDB stuff from Makefile - gdbstub is compiled into application, not framework
  • gdbstub can be used with release or debug builds.
  • Add support in all makefiles for assembly code (both .s and .S files)
  • Remove all COM port settings from Makefile - application code does all this now
  • Bring Makefile-project.mkinline with Makefile-rboot.mk (compiler warnings, etc.)
  • Add new 'gdb' phony target to make it easier to start debugger with correct port, baud rate and other settings

HardwareSerial

  • Fix so that end() and systemDebugOutput() don't screw up initialisation of other serial port(s)
  • Put callback handling code into static methods - saves IRAM and simplifies code
  • Revise interrupt callback to prevent task queue flooding, e.g. on receive overflow
  • Add getStatus() method to report serial errors

HardwareTimer

  • Make HardwareTimer mode selectable (maskable or NMI)

user_main.cpp

  • Don't change serial port baud rates - do that in gdb_init() instead which is compiled into application

system/crash_handler.c

  • Remove stack dumping code and all serial port access - moved into gdb_hooks.cpp
  • Include text for exception code in report

system/SerialBuffer.h

  • Move non-trivial code into .cpp module
  • Add getReadData() and skipRead() methods for efficient block reading

esp_systemapi.h

  • Remove uart prototypes - driver must be used instead
  • Ensure wdt functions are available

uart.h, uart.cpp

  • Add UART2 and provide generic notification callback handling
  • Add UART_OPT_CALLBACK_RAW option
  • Save some IRAM by moving uart_get_uart, uart_set_callback, uart_peek_last_char, uart_rx_available and moving uart_detach into flash
  • Use inline functions in preference to macros
  • Default debug port should be undefined, not UART0
  • Use bit manipulation macros to simplify/clarify code
  • Disable interrupts in uart_tx_free() and uart_rx_available() to ensure result is accurate
  • Fix incorrect RX FIFO size reading, causes problems in FIFO overflow situation
  • Add uart_get_status() to support reporting of errors and Rx BREAK condition
  • Add mode parameter to uart_flush() so tx/rx can be flushed independently, e.g. if a receive overrun occurs.
  • Add uart_set_break() function

gdbstub, exception handling and stack dumping

  • appspecific/gdb/ code is always compiled into application, contains system exception handler and stack dumper
  • gdb/ code is only compiled when ENABLE_GDB=1
  • gdbstub revised for readability and checking
  • uart handling code in separate module (gdbuart.cpp)
  • Add build option to support both patched and unpatched GDB versions
  • Extend GDBSTUB_CTRLC_BREAK to prevent accidental breaking
  • Add gdb_enable() function
  • Include options for stack dumping, with defaults to provide consistent behaviour (i.e. OFF in release builds, ON in debug builds)
  • Add debugging code to stub which can be optionally enabled to see realtime behaviour
  • Implement register read/write (p & P) and binary write (X) commands
  • Provide API for GDB system functions

LiveDebug sample

  • Revise to highlight possible debugging issues with hardware timer, and illustrate other timer types which are 'debug safe'
  • Add serial read callback to demonstrate UART2 function
  • Implement simple interactive debug console using GDB syscall support
  • Add error checking/reporting to serial receive callback

Basic_Serial sample

  • Add error checking/reporting to serial receive callback

Makefile changes:
* Remove esp-gdbstub as third-party module
* Remove all GDB stuff from `Makefile` - gdbstub is compiled into application, not framework
* gdbstub can be used with release or debug builds.
* Add support in all makefiles for assembly code (both .s and .S files)
* Remove all COM port settings from `Makefile` - application code does all this now
* Bring `Makefile-project.mk`inline with `Makefile-rboot.mk` (compiler warnings, etc.)

HardwareSerial
* Fix so that end() and systemDebugOutput() don't screw up initialisation of other serial port(s)
* Put callback handling code into static methods - saves IRAM and simplifies code

HardwareTimer
* Make HardwareTimer mode selectable (maskable or NMI)

user_main.cpp
* Don't change serial port baud rates - do that in gdb_init() instead which is compiled into application

system/crash_handler.c
* Remove stack dumping code and all serial port access - moved into gdb_hooks.cpp

system/SerialBuffer.h
* Move non-trivial code into .cpp module
* Add `getReadData()` and `skipRead()` methods for efficient block reading

esp_systemapi.h
* Remove uart prototypes - driver must be used instead
* Ensure wdt functions are available

uart.h, uart.cpp
* Add UART2 and provide generic notification callback handling
* Add UART_OPT_CALLBACK_RAW option
* Save some IRAM by moving uart_get_uart, uart_set_callback, uart_peek_last_char, uart_rx_available and moving uart_detach into flash
* Use inline functions in preference to macros
* Default debug port should be undefined, not UART0
* Use bit manipulation macros to simplify/clarify code
* Disable interrupts in `uart_tx_free()` and `uart_rx_available()` to ensure result is accurate

gdbstub, exception handling and stack dumping

* appspecific/gdb/ code is always compiled into application, contains system exception handler and stack dumper
* gdb/ code is only compiled when ENABLE_GDB=1
* gdbstub revised for readability and checking
* uart handling code in separate module (gdbuart.cpp)
* Add build option to support both patched and unpatched GDB versions
* Extend GDBSTUB_CTRLC_BREAK to prevent accidental breaking
* Add gdb_enable() function
* Include options for stack dumping, with defaults to provide consistent behaviour (i.e. OFF in release builds, ON in debug builds)
* Add debugging code to stub which can be optionally enabled to see realtime behaviour
* Implement register read/write (p & P) commands

LiveDebug sample

* Revise to highlight possible debugging issues with hardware timer, and illustrate other timer types which are 'debug safe'
* Add serial read callback to demonstrate UART2 function
@mikee47 mikee47 changed the title Initial commit GDB support improvements Mar 28, 2019
@mikee47 mikee47 changed the title GDB support improvements GDB and exception handling improvements Mar 28, 2019
@slaff slaff added this to the 3.8.0 milestone Mar 28, 2019
@slaff
Copy link
Contributor

slaff commented Mar 28, 2019

@riban-bw Mike is updating and improving the GDB debugging code. Can you test this PR and give us your feedback?

Sming/gdb/gdbstub.cpp Outdated Show resolved Hide resolved
@riban-bw
Copy link
Contributor

riban-bw commented Apr 2, 2019

I will try to review this week (whilst I am on a business trip to Inverness, Scotland - nice!). I have it running but need to remind myself how to use gdb and look at how Mike has implemented the sample. Hope to respond this week.

mikee47 added 11 commits April 2, 2019 12:38
Whilst testing I idly pasted a large file into the serial terminal. In normal build (without gdbstub) it went pop. Behaved a little better with gdbstub included. Investigation showed task queue getting flooded (there should only be 1 or 2 tasks on there at a time) and too many interrupts getting fired.

uart
* Make `uart_disable_interrupts()` and `uart_restore_interrupts()` public
* Add `uart_get_status()` function
* Don't invoke callback on receive FIFO full unless buffer is almost full, subject to minimum headroom
* Revise interrupt handler to store status flags (thus capturing a BREAK condition), and simplify loops/reduce code size.

gdbstub
* Use global structure to share state between code modules, simplifies code

gdbuart
* Use separate task callback function to avoid flooding task queue, so `sendUserDataQueued` isn't cleared when `gdbstub_send_user_data()` called directly.

HardwareSerial
* Add `getStatus()` method and `SerialStatus` enumeration for application use
* Rework interrupt callback handling to prevent a task being queued if the previous one hasn't been handled yet.
…t quite right.

Need to respond to overflow interrupt.
Update Basic_Serial sample to check and display status.

RX Overflow test: Paste a large block of data into the serial terminal
Serial BREAK test: On miniterm, hit Ctrl+T then Ctrl+B, and repeat.
@mikee47
Copy link
Contributor Author

mikee47 commented Apr 2, 2019

The last commits include additions to the LiveDebug sample so it interacts with GDB during a debug session. If you like the way it's going, I could take a look at revising the serial receive handler to process input in a similar way, so the program works the same whether using a regular terminal or GDB.

@mikee47 mikee47 closed this Apr 2, 2019
@mikee47 mikee47 reopened this Apr 2, 2019
@slaff
Copy link
Contributor

slaff commented Apr 2, 2019

If you like the way it's going, I could take a look at revising the serial receive handler to process input in a similar way, so the program works the same whether using a regular terminal or GDB.

Yep, that will be best.

@slaff
Copy link
Contributor

slaff commented Apr 3, 2019

I am trying to achieve something very simple but it still does not work for me. I am setting a software breakpoint for the init function

(gdb) br init
Breakpoint 1 at 0x40105da4: file app/application.cpp, line 399.

Then I am trying to run the code until the breakpoint

(gdb) c
Continuing.

But that ends up shortly after that with segfault

Program received signal SIGSEGV, Segmentation fault.
0x4000dfa9 in ?? ()

image

Any idea why is that happening?

@mikee47
Copy link
Contributor Author

mikee47 commented Apr 3, 2019

But that ends up shortly after that with segfault

@slaff I get the same! It's because of an optimisation to the writeMemoryBlock() function I made, which uses memcpy for RAM addresses. Clearly that doesn't work for IRAM; If you take that bit out it works again. I'll look at it in more detail later on, but will probably end up using memcpy for DRAM and slow writes for everything else.

mikee47 added 7 commits April 7, 2019 09:25
* Remove `ets_wdt_disable()` and `ets_wdt_enable()` calls from `dumpExceptionInfo()`, using normal watchdog reset instead. The `ets_wdt_enable()` call prevents the debugging breakpoint from working. Tested at 9600 baud shows reliable operation.
* When GDB is attached, temporarily override any previously output routine (for m_printf, etc.) and write directly to the GDB console. This ensures that a proper explanation of the exception (or system restart) is dispayed in the GDB console.
* Add `GDBSTUB_BREAK_ON_RESTART` to allow proper handling of unexpected system restarts ('crashes') with GDB. The cause of the restart is shown in the console with an appropriate signal.
* Do not show stack dumps when using GDB (the cause is always shown though)
* Add 'hang' command to LiveDebug sample to test behaviour (under GDB and using terminal in both GDB/non-GDB builds). Works as expected.
…ather than directly inside uart ISR. If application does not respond then repeating Ctrl+C twice will force a break.
… delete` commands

* Code in separate module
* Revise `fileDelete()` functions to return error code from SPIFFS
* Add `GDBSTUB_ENABLE_HOSTIO` option
* Add `Sming/System` and `Sming/gdb` directories
* Enable extraction of static members, so `static inline` code gets picked up
* Create GDB group
* Doxygen misbehaves with `__attribute__((packed))` so use `#pragma pack()` instead
@mikee47 mikee47 closed this Apr 7, 2019
@mikee47 mikee47 reopened this Apr 7, 2019
@mikee47
Copy link
Contributor Author

mikee47 commented Apr 7, 2019

@slaff @riban-bw Thank you both for your patience. I've tested the latest changes on Windows/Linux and reasonably happy with its behaviour. I've checked it using the LiveDebug sample in all three modes (GDB, Terminal and non-GDB) and behaves as expected.

For the Initial baud rate setting I've made the change at the user_init() stage as it's simplest, but early rBoot and ROM messages are still output at 74880 as at present.
For Ctrl+C handling and Host I/O I've implemented the propsed changes, but if you prefer to revert any of this that's fine.

I do have a further point about the behaviour of GDBSTUB_BREAK_ON_EXCEPTION, which may benefit from being redefined as with I did with the GDBSTUB_CTRLC_BREAK setting:
0: Disable exception trapping under GDB completely;
1: Enable exception reporting only when debugger is attached;
2: Enable exception reporting at any time (the existing behaviour, consistent with previous gdbstub implementations).
Ditto for GDBSTUB_BREAK_ON_RESTART. My reasoning here is that compiling with a setting of '1' will allow the application to run normally provided GDB isn't attached. So, if an exception occurs or the system crashes it will restart automatically. Contrast this with setting '2', where it will wait indefinitely for GDB. Perhaps setting '1' should be the default.

@mikee47
Copy link
Contributor Author

mikee47 commented Apr 7, 2019

resolving the most urgent / highest impact issues

I've been thinking about this one, and definitely the most critical part of this PR are that it doesn't break anything - and hopefully improves behaviour - when GDB is not enabled. The main culprit would be the uart driver here, of course, along with HardwareSerial, with the addition of error reporting and overflow checking should be more robust whilst keeping 100% backward compatibility. If you've any apps. which stress those in particular worth checking them for sure.

…e `break`, otherwise it's just a no-op.

* Appeared to work as debug prompt appears, but that's the initial startup breakpoint. Function gets called at elevated interrupt level so need to drop it below DEBUG to enable break instruction. Now get correct signal (SIGPWR) in GDB in response to a crash.
@slaff
Copy link
Contributor

slaff commented Apr 8, 2019

@mikee47 Is the PR ready for merging?

@mikee47
Copy link
Contributor Author

mikee47 commented Apr 8, 2019

@slaff Yes, all done.

@slaff
Copy link
Contributor

slaff commented Apr 8, 2019

@slaff Yes, all done.

Thanks a lot!

@slaff slaff removed the 3 - Review label Apr 8, 2019
@slaff slaff merged commit c618f85 into SmingHub:develop Apr 8, 2019
@mikee47 mikee47 deleted the feature/GDB_Integration branch April 8, 2019 06:42

static void IRAM_ATTR doCtrlBreak()
{
if(break_requests != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One side-effect that I am experiencing in Eclipse, and probably other debuggers where you cannot send twice Ctrl-C, is that it looks like the debugging session has paused in the block below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One side-effect that I am experiencing in Eclipse, and probably other debuggers where you cannot send twice Ctrl-C, is that it looks like the debugging session has paused in the block below.

@slaff Sorry, I don't understand - is this something you'd like me to look at before 3.8.0?

@slaff
Copy link
Contributor

slaff commented Apr 10, 2019

.. cannot send twice Ctrl-C ..

Sorry, I don't understand - is this something you'd like me to look at before 3.8.0?

Don't do anything related to this for now. Let me first try to get used to it myself and I will open a new issue if needed.

@mikee47
Copy link
Contributor Author

mikee47 commented Apr 10, 2019

Availability of GDB for Windows
Needs to be integrated into Choco and probably also manual build process.

That should be tackled in a separate PR in the choco-build scripts. Just give me a link from where I can get the xtensa debugger for Windows and I will try to do the rest.

@slaff You can find pre-built version of this at SysProgs. Download and run the executable installer, then copy the C:\SysGCC\esp8266\opt\xtensa-lx106-elf\bin\xtensa-lx106-elf-gdb.exe to a suitable location. This is the Espressif 'patched' version.

@mikee47
Copy link
Contributor Author

mikee47 commented Apr 11, 2019

@slaff All the Arduino toolchains are listed here. Their current toolchains use the unpatched version of GDB. Aside from the size of the register sets, I've noticed that they sometimes produce different stack trace results. Not sure why though, and probably never will without details of the Espressif GDB patch, which I believe is closed source.

@slaff slaff mentioned this pull request Apr 13, 2019
4 tasks
@slaff
Copy link
Contributor

slaff commented Apr 13, 2019

@mikee47 I am still experiencing issues with the debugger. For example a very simple session like the one below ends up in flames

1. New LiveDebugg application is flashed on the NodeMCU device.
2. make gdb is executed.
---
For help, type "help".
Type "apropos word" to search for commands related to "word".
0xffffffff in ?? ()
overlapping memory region
overlapping memory region
overlapping memory region
overlapping memory region
overlapping memory region
overlapping memory region
add symbol table from file "out/build/bootrom.elf" at
	.text_addr = 0x40000000

Welcome to SMING!
Type 'c' (continue) to run application

A program is being debugged already.  Kill it? (y or n) y
Remote debugging using /dev/ttyUSB0
gdbstub_init () at /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming//gdb/gdbstub.cpp:908
908			gdbstub_do_break();
(gdb) 
(gdb) c
Continuing.
LiveDebug sample
Explore some capabilities of the GDB debugger.

[OS] mode : sta(18:fe:34:f3:74:e7)
[OS] add if0
252348026 GdbAttach(1)
252391206 open log 0
(Attached) break
255528829 gdb_read_console() returned 6
Calling gdb_do_break()

Program received signal SIGTRAP, Trace/breakpoint trap.
gdb_do_break () at /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming//gdb/gdbstub.cpp:937
937		gdbstub_do_break();
(gdb) br blink
Breakpoint 1 at 0x40105d4c: file app/application.cpp, line 53.
(gdb) c
Continuing.

Breakpoint 1, blink () at app/application.cpp:53
53	{
(gdb) 
Continuing.

Breakpoint 1, blink () at app/application.cpp:53
53	{
(gdb) 
Continuing.
(Attached) 


***** Fatal exception 20 (INSTR_PROHIBITED)
pc=0xffffffff sp=0x3ffffec0 excvaddr=0x00000000
ps=0x00000030 sar=0x0000001e vpri=0x402072cc
r00: 0xffffffff=        -1 r01: 0x3ffffec0=1073741504 r02: 0x00000000=         0 
r03: 0x00000001=         1 r04: 0x00000000=         0 r05: 0x3ffeebf0=1073671152 
r06: 0x40224020=1075986464 r07: 0x3ffe8ac0=1073646272 r08: 0x00000008=         8 
r09: 0x00000000=         0 r10: 0x3ffe8f64=1073647460 r11: 0x3ffe8a9c=1073646236 
r12: 0x3fffc230=1073726000 r13: 0x401062fc=1074815740 r14: 0x3fffc200=1073725952 
r15: 0x00000022=        34 

Program received signal SIGSEGV, Segmentation fault.
0xffffffff in ?? ()

A shorter version of the commands above needed to reproduce the issue:

make gdb
(gdb) c
(Attached) break
(gdb) br blink
(gdb) c
(gdb) [ENTER]
(gdb) [ENTER]
(Attached) [ENTER]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants