[Needs testing] Add full gdb support with uart/Serial integration

[kylefleming]

Summary

This adds full gdb debugging support, including integration with HardwareSerial and Esp8266Arduino's uart library.

Note: This will need some testing to make sure it can be added to any sketch without any side effects until the user wants to debug with gdb.

Dependencies

This PR requires #4356 to work properly.

Features

• Only requires #include <GDBStub.h> in order to enable
• Operates transparently until a gdb packet is recognized, indicating the user wants to debug with GDB. When the user disconnects GDB, it goes back to operating like normal
• Allows connecting to a running sketch or one that has hit an exception
• Allows continuing the sketch with gdb connected and using ctl-c to break at any point
• Allows detaching gdb with the sketch resuming normal operation
• Allows hardware and software breakpoints
• Compatible with PlatformIO Unified Debugger

Code changes

• Converted the uart isr handler code to be asynchronous
• Added flags so the uart lib knows when it should avoid touching uart registers, since gdbstub will handle that
• Added callbacks so the uart lib can register with gdbstub and receive printf and incoming uart data whenever a gdb session is not attached
• Responds to the attached command to signal that gdb connected to a running program and should detach instead of kill when disconnecting
• Turned on GDBSTUB_REDIRECT_CONSOLE_OUTPUT and GDBSTUB_CTRLC_BREAK flags by default, since those should now be compatible with the rest of the Esp8266Arduino library.
• Moved assembly functions to ram
• Added response for detach and kill commands
• Fixed packet size reporting
• Added enable pin hook so frameworks (Esp8266Arduino in this case) can run custom code to enable the uart tx/rx pins

Running gdb

Create a file containing the following gdb initial commands (adjust your target and baudrate if needed):

set remote hardware-breakpoint-limit 1
set remote hardware-watchpoint-limit 1
set remotebaud 115200
file </Path/to/firmware.elf>
target remote <Your port, e.g. /dev/cu.SLAB_USBtoUART>

Run gdb:

$ /path/to/xtensa-lx106-elf-gdb -x </path/to/gdbcmdsfile>

PlatformIO support

• A custom debug tool is currently required for PlatformIO Unified Debugger support. This is an example of the config I used for the d1 mini (added to platform-espressif8266/boards/d1_mini.json)

"debug": {
  "tools": {
    "custom": {
      "server": {
        "package": "toolchain-xtensa",
        "executable": "bin/xtensa-lx106-elf-gdb"
      },
      "init_cmds": [
        "set remote hardware-breakpoint-limit 1",
        "set remote hardware-watchpoint-limit 1",
        "file \"$PROG_PATH\"",
        "set remotebaud 115200",
        "target remote $DEBUG_PORT"
      ],
      "load_mode": "manual",
      "default": true
    }
  }
}

and then set debug_port in your platformio.ini. For example:

debug_port = /dev/cu.SLAB_USBtoUART

[devyte]

@kylefleming are you aware of PR #4336 ?
At a glance, I can't tell which support for gdbstub is better. PR #4336 includes extensive docs and examples, and I have the impression that the underlying implementation is more comprehensive, while this one seems to have hardware serial support.
I suspect that we can come to a "best of both worlds" solution. Please discuss with @freeck to see if we can unite both approaches.

[igrr]

@devyte #4336 adds another copy of gdbstub configured with different settings, while this PR integrates Arduino-specific UART handling into gdbstub so that they can coexist. I like this approach a lot.

However sometimes the full gdb stub (which would include run control debugging support) is not needed, and there is not enough IRAM to include it. So it would be nice if there was still an option to include the smaller panic-handler-only gbd stub.

[kylefleming]

(Edit: @igrr beat me to it)

@devyte Yep, I'm aware of it. The code that's being added in #4336 is actually just a near copy of the GDBStub code already in Esp8266Arduino. #4336 enables the rest of the GDBStub flags, which were previously deemed incompatible with HardwareSerial and Esp8266Arduino's uart library (as noted in this comment). Unfortunately, the GDBStub added with #4336 still maintains that incompatibility. This PR fixes that by rewriting GDBStub in an attempt to be 100% compatible with Esp8266Arduino.

Documentation and tutorials still seem like a great contribution though, which is what I believe #4336 is really contributing. The tutorial will likely need to be revised though since this PR simplifies the process quite a bit.

[kylefleming]

@igrr Good point about having the option to only include the exception handler. How do you suggest users integrate each version? I'm not familiar with how technical users of this library are. Is adding a build flag to enable the full version something that we can reasonably expect of them?

Also, is there any sort of DEBUG flag that would be included if Arduino, Sloeber, PlatformIO, etc were compiling in order to run in debug mode with the debugger attached?

[igrr]

I would try to implement this choice (full or panic-only gdb stub) using two header files, for example GDBStub.h and GDBStubDebug.h. In GDBStubDebug.h, there would be a non-static function declared, for example:

extern "C" void gdbstub_all_features_hook() { 
  extern "C" void gdbstub_all_features();
  gdbstub_all_features();
}

In gdbstub source, there is a weak definition of gdbstub_all_features_hook, which does not pull gdbstub_all_features. gdbstub_all_features_hook is called somewhere from gdb stub initialization.

gdbstub_all_features function is defined in a separate source file, along with a bunch of functions which implement gdbstub functionality only needed in "full" mode. These functions are called from the main gdbstub source file. Main gdbstub source file also includes weak no-op definitions of these extra functions.

Putting it all together, if GDBStub.h is included into the sketch, gdbstub_all_features_hook is not defined by the sketch, so gdbstub_all_features does not get referenced, and extra gdb stub functions are not pulled in (weak no-op functions are used instead).
If GDBStubDebug.h is included, gdbstub_all_features_hook from the sketch gets linked in, which pulls gdbstub_all_features, which in turn pulls in real definitions of extra gdb stub functions required for full debugging experience.

That's quite a hack, but on the other hand it's very easy to use and does not depend on IDE specific features. Does this make sense to you?

[kylefleming]

Yep, makes sense. Does the build script skip compiling and linking the "full debug source" compilation unit if it's not being referenced from the sketch? My initial assumption was that it linked everything in the library.

[igrr]

Hmm, you're right — this is only going to work if the GDBStub library is first packaged into an archive file. Then the object file will not be included into the output unless it resolves some of the undefined symbols. However Arduino IDE does not build an archive file for each library, so that's not going to work. Let me think of some other option.

[devyte]

@igrr I understand that it's different settings, which is why I mentioned at some point that the two PRs don't seem mutually exclusive to me (in answer to @freeck's concerns about his efforts).
My point is that there is overlap between the two (three?) implementations, and it seems to me we should avoid having multiple very-similar-yet-not-the-same libs.
Instead, I would like to know if we can have a single gdbstub lib, with certain options exposed to the user.
Maybe something like how we have lwip integrated, where you can choose from some prebuilt versions with specific options, or build your own from sources with a custom config.

[kylefleming]

@devyte I appreciate the concern to not have wasted efforts. There aren't multiple competing implementations at this point, don't worry. There're only 2 versions: the original GDBStub that Espressif wrote (both the version currently in Esp8266Arduino and the one included in PR #4336 are basically the same as this original one) and the new reworked one that is the content of this PR.

What @freeck is contributing in #4336 is documentation on how to use the one written by Espressif. Some of that documentation will still be valid and some of it will be obsolete, which is what @freeck's comment was pointing out. This PR is an improvement on the original implementation, so it's a good thing that the process is simplified and we no longer need quite so extensive documentation.

[kylefleming]

@igrr it looks like this gdbstub takes up around 3.7k of ram. Theoretically, there wouldn't be much of a difference between the panic-only version and the full version since most of the space is being taken up by packet reading/building and gdb command response logic, which both versions would require.

The main 2 differences between the 2 versions are that the full version requires uart hooking logic and the fact that monitoring uart at all times requires the packet deconstruction code to be able to operate asynchronously. Reading asynchronously takes up more code, but doesn't necessarily need to take up that much more than a synchronous version.

If I optimized the library, is there a size that would be acceptable to include the full version by default?

[freeck]

@kylefleming I totally agree with @igrr that your version is the ultimate one, I dont see any reason to keep "my version" on github. Some of the documentation is perhaps reuseable.
BUT the ram-usage you mention of 3.7k is quit large. With my version I use less than 500 bytes. As the espressif version took too much iram (I run out of iram very quickly!!!) I modified the gdbstub-code by moving code to flash. That might have consequences for debugging code but until now it works fine.

[devyte]

Has there been progress in reducing that 3.7K?

[freeck]

@devyte @kylefleming I got it working using the Sloeber IDE using arduino-esp V2.4.0-rc2 (btw, the latest version takes 500 bytes IRAM extra!).
From the gdbstub-library I bluntly changed allmost all IRAM directives to FLASH directives, and copied the uart.c and uart.h to V2.4.0-rc2. Now gdbstub takes less than 500 bytes IRAM. Using the commandline debugger I can now dynamically attach into a running program and do some debug-stuff like single stepping, set new breakponts etc....I cant oversee the consequences of my modifications concerning timing issues and other internal issues, but it works great so far!

One issue: when you change GDBSTUB_REDIRECT_OUTPUT 1 to 0 the compilation fails.

[weswitt]

@freeck will you be able to share the new work on this? PR? What are the plans to get this into the main branch?

[freeck]

@weswitt No problem, but lets first wait for the comments of @kylefleming concerning my proposed modifications... or use the files published on #4336

[earlephilhower]

@kylefleming This is a very interesting PR! But, it looks like the build is failing due to warnings (== error) in the sources.

Can you try rebuilding locally, but using the "File->Preferences->Compiler Warnings->All" option? That'll show the same errors the build is reporting (as the CI requires -Wall to be clean).

[freeck]

@earlephilhower I had the same problem compiling with Sloeber/Eclipse-IDE. The sections seem to interfere, I got "dangerous relocation errors".....
Being a dirty programmer I removed most if not all IRAM directives because GDBSTUB takes too much IRAM.
Now Sloeber compiles without errors, and I am now able to start a debugging session on a running target, and able to set breakpoints on the fly, GREAT!

[earlephilhower]

@freeck it's not even getting to the link stage here. The compiler is showing many unused parameters and variables, which makes the core -Wall -Werror compile fail:

/home/travis/arduino_ide/hardware/esp8266com/esp8266/cores/esp8266/uart.c: In function 'gdbstub_set_putc1_callback':
/home/travis/arduino_ide/hardware/esp8266com/esp8266/cores/esp8266/uart.c:57:62: error: unused parameter 'func' [-Werror=unused-parameter]
 void __attribute__((weak)) gdbstub_set_putc1_callback(void (*func)(char)) {}
                                                              ^
/home/travis/arduino_ide/hardware/esp8266com/esp8266/cores/esp8266/uart.c: In function 'gdbstub_set_uart_isr_callback':
/home/travis/arduino_ide/hardware/esp8266com/esp8266/cores/esp8266/uart.c:60:65: error: unused parameter 'func' [-Werror=unused-parameter]
 void __attribute__((weak)) gdbstub_set_uart_isr_callback(void (*func)(void*, uint8_t), void* arg) {}
                                                                 ^
/home/travis/arduino_ide/hardware/esp8266com/esp8266/cores/esp8266/uart.c:60:94: error: unused parameter 'arg' [-Werror=unused-parameter]
 void __attribute__((weak)) gdbstub_set_uart_isr_callback(void (*func)(void*, uint8_t), void* arg) {}
                                                                                              ^
/home/travis/arduino_ide/hardware/esp8266com/esp8266/cores/esp8266/uart.c: In function 'gdbstub_write_char':
/home/travis/arduino_ide/hardware/esp8266com/esp8266/cores/esp8266/uart.c:62:52: error: unused parameter 'c' [-Werror=unused-parameter]
 void __attribute__((weak)) gdbstub_write_char(char c) {}
                                                    ^
/home/travis/arduino_ide/hardware/esp8266com/esp8266/cores/esp8266/uart.c: In function 'gdbstub_write':
/home/travis/arduino_ide/hardware/esp8266com/esp8266/cores/esp8266/uart.c:63:54: error: unused parameter 'buf' [-Werror=unused-parameter]
 void __attribute__((weak)) gdbstub_write(const char* buf, size_t size) {}
                                                      ^
/home/travis/arduino_ide/hardware/esp8266com/esp8266/cores/esp8266/uart.c:63:66: error: unused parameter 'size' [-Werror=unused-parameter]
 void __attribute__((weak)) gdbstub_write(const char* buf, size_t size) {}
                                                                  ^
cc1: all warnings being treated as errors
Using library ESP8266WiFi at version 1.0 in folder: /home/travis/arduino_ide/hardware/esp8266com/esp8266/libraries/ESP8266WiFi 
Using library ESP8266mDNS in folder: /home/travis/arduino_ide/hardware/esp8266com/esp8266/libraries/ESP8266mDNS (legacy)
Using library ArduinoOTA at version 1.0 in folder: /home/travis/arduino_ide/hardware/esp8266com/esp8266/libraries/ArduinoOTA 
exit status 1

Simple code edits, assuming these variables are consciously intended not to be used, can fix this (i.e. adding "(void) xxx;" for unused params, deleting locals which aren't used, etc.).

[freeck]

@earlephilhower I do not use the Arduino-IDE for application development, but the Sloeber-IDE. I am afraid you have to wait for @kylefleming to respond....sorry.

@devyte @kylefleming I got it working using the Sloeber IDE using arduino-esp V2.4.0-rc2 (btw, the latest version takes 500 bytes IRAM extra!).
From the gdbstub-library I bluntly changed allmost all IRAM directives to FLASH directives, and copied the uart.c and uart.h to V2.4.0-rc2.

[earlephilhower]

Sure, I got that @freeck. But for the Arduino core, PRs need to compile cleanly w/-Wall -Werror, which is the (very minor!) problem here. @kylefleming needs to do some minor tweaks and then we can worry about moving between IRAM and IROM on this setup.

[freeck]

@earlephilhower I understand, But for me reducing IRAM was a major issue as debugging medium to large applications is almost impossible with this version of gdbstub......

[romansavrulin]

Hello, guys! Please take a look at #4480. We've found an unexpected behaviour of assert(0) without GDB stub. I think you should definitely consider testing this case here

[kylefleming]

Thanks all for the feedback. I've taken another pass at this.

Changes:

• Works properly when instruction cache is disabled (ensured all system functions are the versions that in ram, plus moved array initialization out of method since it uses memcpy)
• Added option for disabling break on exception (for when you'd rather it restarts)
• Shrunk code to 3.2kB (when using the default -Os)
• Fixed -Wextra warnings and other issues
• Integrated with @igrr's gdb_hook changes

Note: Requires #4356 to work properly

I think the main remaining issue is the code size. Obviously, we could move everything to flash, as a few people have suggested, but then we lose the ability to use gdb at all during periods when the icache is disabled (whether an exception was thrown, a breakpoint was placed, or ctrl-c interrupt was sent). @igrr Do you have any further thoughts on this?

[kylefleming]

More improvements:

• Fixed a linker issue
• Trimmed iram size to 2.8k

I also created an example repo. It uses platformio for setup, but the gdbcmds file can be adapted for other setups. The gdb.sh script runs gdb from the command line.

The example can be found here:
https://github.com/kylefleming/platformio-gdbstub-example

[kylefleming]

@igrr I had another thought. Could we check for a disabled cache and re-enable it? When execution continues, it could be re-disabled.

It looks like Cache_Read_Enable_2 and Cache_Read_Disable_2 serve this function. These are the methods the spi functions use. I'm not sure what would need to be flushed though. Possibly a few extw or isync instructions would be enough.

If we went this route, only the wrapper function would need to be in iram, alleviating the size concern.

[igrr]

Good idea, i think it should work, and might also be used in the non-GDB panic handler.

…

On Fri, Mar 16, 2018 at 1:33 PM, Kyle Fleming ***@***.***> wrote: @igrr <https://github.com/igrr> I had another thought. Could we check for a disabled cache and re-enable it? When execution continues, it could be re-disabled. It looks like Cache_Read_Enable_2 and Cache_Read_Disable_2 serve this function. These are the methods the spi functions use. I'm not sure what would need to be flushed though. Possibly a few extw or isync instructions would be enough. If we went this route, only the wrapper function would need to be in iram, alleviating the size concern. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4386 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEJcenv_3VVHyU2SxAxe1qw6tI6M8WB8ks5te060gaJpZM4SKHl_> .

[devyte]

@kylefleming there are conflicts in this pr now. Could you please resolve?

[devyte]

Closing in favor of #5559 .