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 support w/new toolchain and UART driver #5559

Merged
merged 21 commits into from
Jan 23, 2019
Merged

Conversation

earlephilhower
Copy link
Collaborator

@earlephilhower earlephilhower commented Dec 27, 2018

This extends #4386 by manually fixing the merge issues in @kylefleming 's original PR, fixing the linker literal assignment errors, and updating the PR to use the open source GDB protocol for Xtensa (as is included in the new toolchain).

It's still WIP because I've not tried everything out nor written up how to use it, but it's able to stop a program in mid-run, set a breakpoint, continue, show values, and change them.

#include <GDBStub.h>
extern "C" void gdbstub_init();
void setup() {
gdbstub_init();
Serial.begin(115200);
Serial.printf("Starting...\n");
}

void loop() {
static  uint32_t cnt = 0;
Serial.printf("%d\n", cnt++);
//*(int*)0=1;
delay(100);
}
earle@server:~/src/esp-quick-toolchain/repo/binutils-gdb/gdb$ ~/Arduino/hardware/esp8266com/esp8266/tools/xtensa-lx106-elf/bin/xtensa-lx106-elf-gdb
GNU gdb (GDB) 8.2.50.20180723-git
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "--host=x86_64-linux-gnu --target=xtensa-lx106-elf".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word".
(gdb) set remote hardware-breakpoint-limit 1
(gdb) set remote hardware-watchpoint-limit 1
(gdb) set remote interrupt-on-connect on
(gdb) set remote kill-packet off
(gdb) set remote symbol-lookup-packet off
(gdb) set remote verbose-resume-packet off
(gdb) file /tmp/arduino_build_257110/sketch_dec26a.ino.elf
Reading symbols from /tmp/arduino_build_257110/sketch_dec26a.ino.elf...done.
(gdb) mem 0x20000000 0x3fefffff ro cache
(gdb) mem 0x3ff00000 0x3fffffff rw
(gdb) mem 0x40000000 0x400fffff ro cache
(gdb) mem 0x40100000 0x4013ffff rw cache
(gdb) mem 0x40140000 0x5fffffff ro cache
(gdb) mem 0x60000000 0x60001fff rw
(gdb) set serial baud 115200
(gdb) target remote /dev/ttyUSB0
Remote debugging using /dev/ttyUSB0
0x40000f68 in ?? ()
(gdb) where
#0  0x40000f68 in ?? ()
#1  0x40000f58 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) cont
Continuing.
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
^C
Program received signal SIGINT, Interrupt.
0x40000f68 in ?? ()
(gdb) where
#0  0x40000f68 in ?? ()
#1  0x40000f58 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) lsit
Undefined command: "lsit".  Try "help".
(gdb) list
1	#include <Arduino.h>
2	#line 1 "/home/earle/Arduino/sketch_dec26a/sketch_dec26a.ino"
3	#line 1 "/home/earle/Arduino/sketch_dec26a/sketch_dec26a.ino"
4	#include <GDBStub.h>
5	extern "C" 
6	void gdbstub_init();
7	#line 4 "/home/earle/Arduino/sketch_dec26a/sketch_dec26a.ino"
8	void setup();
9	#line 11 "/home/earle/Arduino/sketch_dec26a/sketch_dec26a.ino"
10	void loop();
(gdb) cont
Continuing.
bcn_timout,ap_probe_send_start
312
313
314
315
^C
Program received signal SIGINT, Interrupt.
0x40000f68 in ?? ()
(gdb) break loop
Breakpoint 1 at 0x402024d7: file /home/earle/Arduino/sketch_dec26a/sketch_dec26a.ino, line 11.
(gdb) cont
Continuing.
Note: automatically using hardware breakpoints for read-only addresses.

Breakpoint 1, loop () at /home/earle/Arduino/sketch_dec26a/sketch_dec26a.ino:11
11	void loop() {
(gdb) where
#0  loop () at /home/earle/Arduino/sketch_dec26a/sketch_dec26a.ino:11
#1  0x40202a40 in loop_wrapper () at /home/earle/Arduino/hardware/esp8266com/esp8266/cores/esp8266/core_esp8266_main.cpp:124
#2  0x401013f9 in cont_wrapper () at /home/earle/Arduino/hardware/esp8266com/esp8266/cores/esp8266/cont.S:80
(gdb) info registers
pc             0x402024d7          0x402024d7 <loop()+3>
lbeg           <unavailable>
lend           <unavailable>
lcount         <unavailable>
sar            0x1e                30
litbase        0x0                 0
ps             0x20                32
threadptr      <unavailable>
scompare1      <unavailable>
a0             0x40202a40          1075849792
a1             0x3fffffb0          1073741744
a2             0x3ffee3b8          1073669048
a3             0x40202a20          1075849760
a4             0x0                 0
a5             0x0                 0
a6             0x0                 0
a7             0x0                 0
a8             0xfffffffe          -2
a9             0xffffffff          -1
a10            0x3fffc6fc          1073727228
a11            0x1                 1
a12            0x3ffee558          1073669464
a13            0x0                 0
a14            0x3fffdad0          1073732304
a15            0x3ffee58c          1073669516
(gdb) print cnt
No symbol "cnt" in current context.
(gdb) next
loop () at /home/earle/Arduino/sketch_dec26a/sketch_dec26a.ino:14
14	Serial.printf("%d\n", cnt++);
(gdb) print cnt
$1 = 316
(gdb) set cnt=2000
(gdb) print cnt
$2 = 2000
(gdb) cont
Continuing.
2000

Breakpoint 1, loop () at /home/earle/Arduino/sketch_dec26a/sketch_dec26a.ino:11
11	void loop() {
(gdb) cont
Continuing.
2001

kylefleming and others added 9 commits March 28, 2018 16:20
It appears that Espressif patched the open source xtensa GDB port in
order to build their old GDB executable and their old gdbstub (basically
removing any register in a generic xtensa and only leaving those
present in the chip they synthesized).  Their GDBStub also assumed this
behavior.

Unpatched upstream GNU GDB now expects all the registers in
xtensa-config.c to be sent/read on a 'g' command.  Change the GDB stub
to send "xxxxxxxx"s (legal per the spec) for unimplemented registers.
This makes the 'g' response much longer, but it's results are cached
and in an interactive debugger it isn't noticeable.
All functions which are not interrupt or exception called are now in
flash. A small IRAM wrapper enables flash when processing main GDB ops
by calling Cache_Read_Enable_New() and then jumping to the main flash
code.  This seems to work for catching exceptions, data and code breaks,
and Ctrl-C.

The UART ISR handler and exception handler register-saving bits of
code in ASM are still in IRAM.

GDB IRAM usage is now about 670 bytes.
Add some simple GDB documentation to the main tree showing a worked
example.

Adds the definition of `void gdbstub_init()` to <GDBStub.h>
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

I have several comments and questions, but I'm not sure if they should be addressed to @earlephilhower , @kylefleming , or someone else.

cores/esp8266/uart.c Outdated Show resolved Hide resolved
cores/esp8266/uart.c Outdated Show resolved Hide resolved
cores/esp8266/uart.c Show resolved Hide resolved
cores/esp8266/uart.c Show resolved Hide resolved
cores/esp8266/uart.c Outdated Show resolved Hide resolved
cores/esp8266/uart.c Outdated Show resolved Hide resolved
doc/gdb.rst Outdated Show resolved Hide resolved
libraries/GDBStub/src/internal/gdbstub.c Outdated Show resolved Hide resolved
libraries/GDBStub/src/internal/gdbstub.c Show resolved Hide resolved
libraries/GDBStub/src/GDBStub.h Show resolved Hide resolved
@devyte
Copy link
Collaborator

devyte commented Dec 28, 2018

@earlephilhower In #4386 there are some observations, e.g.:

  • by @igrr to have a switch to enable the smaller panic-only handler
  • about 3.7KB RAM usage

Does it make sense to pursue them in view of the current status of this implementation?

@earlephilhower
Copy link
Collaborator Author

earlephilhower commented Dec 30, 2018

@devyte wrote:

by @igrr to have a switch to enable the smaller panic-only handler

He was talking about the fatal exception handler (postmortem). It's already moved out in 2.5.0-b2

about 3.7KB RAM usage

That's IRAM usage AFAICT from the thread. The movement of things to flash with stubs that call os-enable-flash was already done. We're at ~700b of IRAM and ~1KB of heap (and ~3KB of flash). So it's not something we would want to enable by default, but it's not unreasonable. Might be heap/flash savings possible, but I've not yet looked into any optimizations other than minimizing the IRAM footprint..

Since the other review comments look like work, I'll leave them for tomorrow.

earlephilhower and others added 5 commits December 30, 2018 09:11
Replace GDBstub.h with the version in the internal/ directory, and
adjust stub code accordingly.  This way, only one copy of a file called
"GDBstub.h" will exist.

Update the gdbcommands and replace the obsolete ESPRESSIF readme with
@kylefleming's version since we're mainly doing serial, not TCP,
connected debugging.

Bump the library rev. number since this is a pretty big functionality
change.

Minor documentation tweak.
Remove the refactoring of pin control and other little things not directly
related to GDB processing.  Should greatly reduce the diff size in uart.c.
Should also remove any register value changes (intended or otherwise)
introduced in the original PR from @kylefleming.

Set the FIFO interrupt to 16 chars when in GDB mode, matching the latest
UART configuration for highest speed.
Comments added to UART.c trying to explain (as best as I understand it)
the changes done to support GDB and how they interact with standard
operation.

Fix the uart_uninit to stop the ISR and then free appropriately.

Fix uart_isr_handle_data (GDB's shim for sending chars to the 8266 app)
to do the exact same thing as the standard UART handler including set
the overflow properly and either discard or overwrite in that case.

Fix serial reception when GDB enabled by enabling the user recv ISR.

Remove commented attributes from gdbstub, leftover from the move to
flash.

General logic cleanup per comments in the PR.
Ensure we also check the UART flags and set the uart status
appropriately when in GDB mode.
Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

@earlephilhower earlephilhower changed the title WIP - GDB support w/new toolchain and UART driver GDB support w/new toolchain and UART driver Jan 3, 2019
@freeck
Copy link

freeck commented Jan 6, 2019

Hi there,
Great work!! Got it working using Eclipse-Sloeber-IDE, so no complicated userinterventions using gdb...
Also the exceptionhandler seems to work ok. IRAM-usage much better than previous versions.
Need more testing with a serious application.

@earlephilhower
Copy link
Collaborator Author

@freeck, thanks for the report! Is there anything needed to make it work with your IDE? I can update the docs easily enough (but run CLI for most things so haven't used Eclipse or an IDE in a long time).

@freeck
Copy link

freeck commented Jan 7, 2019

I will adapt my document (the document should reside on this site somewhere) based on the previous version of gdbstub. It is quit easy to configure Eclipse.
A question: are the mem settings as defined in the cmds-file fixed for all the esp8266 chips and all boards?

@earlephilhower
Copy link
Collaborator Author

Yes, the mem settings are applicable to all boards. While the 512M flash boards don't have the full 1MB program flash, the address space that doesn't exist should never be accessed by GDB (since your program can never reference it).

@freeck
Copy link

freeck commented Jan 8, 2019

Question perhaps a bit off topic: Is there any documentation on how to handle exceptions on application level (try catch) , with and without using this debugger? With the example provided I observe that the debugger halts at line 4....(-fexceptions). Adding a try catch statement around line 4 show the same result.
1:void loop() {
2:static uint32_t cnt = 0;
3:Serial.printf("%d\n", cnt++);
4:(int)0=1;
5: delay(100);
6:}

@earlephilhower
Copy link
Collaborator Author

They're 2 different things. Hardware exceptions vs C++ exceptions.

  1. *(int*)0=1; == HW exception (think segfault, bluescreen, etc.)
  2. char *toobigary = new char[9999999]; == C++ exception (it throws a std::exception out-of-mem exception).

You can't catch() the 1st type, they're below the language and hardware actually does a jump to a HW exception vector.

@earlephilhower
Copy link
Collaborator Author

I'm not sure I understand what you're trying to do, but if you don't include gdbtub.h and do the call then the GDB stub will never be installed and the default exception handler will be called.

The simplest thing you could do is use #ifdef RELEASE around the gdbstub_init() call if that's what you want. Or you can call gdbstub_init() somewhere else in your code, say if you get a special char sequence on the serial port. There may be some hiccup if you're already using the serial port when this happens, but it's programmatic at that point...

@freeck
Copy link

freeck commented Jan 23, 2019

The simplest thing you could do is use #ifdef RELEASE around the gdbstub_init()

That implies I have to build an new image and download to the target...

Or you can call gdbstub_init() somewhere else in your code, say if you get a special char sequence on the serial port. There may be some hiccup if you're already using the serial port when this happens, but it's programmatic at that point...

OK, that's an option! But now: when I disconnect the serial communication line, the gdbserver remains active, not? Ergo the GDB-server will be activated on an exception and the application halts. So how to detach programmatically in such cases?

@earlephilhower
Copy link
Collaborator Author

Actually, scratch that. By including a call to gdbstub_init() anywhere in the code, the linker will replace dummy no-op GDB hooks with real GDB hooks in the code, even if it doesn't get called. So gdbstub_init() later on won't affect things, that only hooks the serial port and (I think) the breakpoint exception.

With the way it's architected now, I don't believe there's a clean, sane way to do what you're trying.

@freeck
Copy link

freeck commented Jan 23, 2019

Ok no problem, I am happy as it is.

@earlephilhower earlephilhower added this to the 2.5.0 milestone Jan 23, 2019
@earlephilhower
Copy link
Collaborator Author

@devyte, what do you think of getting this in 2.5.0? It's self contained and introduces no changes in default operation (you need to mod your code to enable GDB). It's been tested by me and @freeck with different environments (Eclipse and CLI). Even in the worst case GDB would just not work (same as before), so it seems like only upside.

@freeck
Copy link

freeck commented Jan 23, 2019

Using Eclipse for debugging I observe that system messages and output from function ets_printf() are sent to the gdb-tracewindow on a single character basis packed in a debug-message. As functions like Serial.printf are buffered and are sent on a line basis. The first creates a lot of overhead in gdb client-server traffic and significantly delays the debugproces during entering a breakpoint or singel stepping.
Is this an gdbstub-issue? Or Eclipse-CDT related issue?

@earlephilhower
Copy link
Collaborator Author

Haven't seen any problem when running from CLI (printf's seem to come out at normal speed, but I haven't run it with a stopwatch or anything). Hitting a breakpoint and getting app as well as debug-command output seems instantaneous. Could be some weird JAVA thing or the way Eclipse is running things (maybe single-insn stepping through all code?).

@earlephilhower
Copy link
Collaborator Author

earlephilhower commented Jan 23, 2019

void ATTR_GDBEXTERNFN gdbstub_write_char(char c) {
if (gdb_attached) {
ETS_UART_INTR_DISABLE();
gdbSendOutputPacketChar(c);
ETS_UART_INTR_ENABLE();
} else {
gdbSendChar(c);
}
}

So only if the GDB debugger is connected will it encapsulate chars with the GDB packet. That's just the way it has to be when GDB is connected since it's parsing everything coming over the line and would interpret bytes as garbage and barf.

At 115200 and the CLI, it's as close as I can tell to normal speed. Maybe you're running at a low baud?

@devyte
Copy link
Collaborator

devyte commented Jan 23, 2019

My one comment is almost the same as @freeck 's: it would be nice to be able to de-init the gdbstub to allow normal operation. I know that calling gdbstub_init() replaces the hooks with real ones, but that shouldn't mean that the gdbstub functionality needs to be enabled all the time. In other words, it should be possible to have the gdbstub compiled into the app, then have something like a master switch that enables/disables the capability to attach gdb.
But that's a nice-to-have additional enhancement that needs to be investigated and should be handled in a different PR.

@freeck
Copy link

freeck commented Jan 23, 2019

@devyte It is indeed a nice to have. So far I am very happy with the debugger as it is.

@earlephilhower
Copy link
Collaborator Author

Please, feel free to open an issue for the next release. It's not something that's impossible, only tedious, but it will need new code and testing. You won't get back any IRAM, of course, which was the big reason for not including GDBStub by default.

@Makuna
Copy link
Collaborator

Makuna commented Jan 28, 2019

Sorry guys, but I wish I new about this while I had an active pull request of my own with uart.h/uart.c.

I want to discuss if this truly was the right thing to do. I have concerns over the architecture that moved the ownership of the uart ISR out of uart work into another place, versus having the gdbstub registering into the "uart" to take over complete ownership. What you have created is hard to manage in one spot or even enhance (which was what my pull was cleaning up and was active at the same time).

Thus making my changes and this pretty incompatible.

@freeck
Copy link

freeck commented Jan 31, 2019

Observation: my application relies on the softwareserial library (ver 18-12-2018). I receive 1000 bytes messages, baudrate is 115200, length message 1000 bytes, each message ends with a crc.
Using v2.4.2 the crc-error rate was aprox. 1%
Using v2.5.0 the crc-error rate was aprox. 50%
Using this tree with/without Gdbstub the crc error rate is >90%. As I assume softwareserial relies havily on handling interrupts each edge of a signal.
Any idea what might be the correlation between the upgrades of the later images (specially this image) , library softwareserial and the increasing error rate?

@earlephilhower
Copy link
Collaborator Author

Absolute no idea, sorry. SWSerial doesn't use interrupts at 115.2K, it busy-loops unless the baud is really low IIRC. I'd suggest pinging the SWSerial author if I were you (we did upgrade to the latest SWserial repo a month or so back)...

@freeck
Copy link

freeck commented Feb 1, 2019

Don't you think there is a strong relation with this issue #5513 …..knowing that SW-serial does not use interrupts....

@freeck
Copy link

freeck commented Feb 1, 2019

Concerning IRAM-usage for gdbstub: from version 2.5.0 on the I2C-library core_esp8266_si2c.c has been extended with slave-functionality. For a good reason there was decided to put a lot of functions in IRAM!
I don't use these functions but they are linked into the image nevertheless....but this costs 2596 bytes of our scarce IRAM.

@earlephilhower
Copy link
Collaborator Author

Don't you think there is a strong relation with this issue #5513 …..knowing that SW-serial does not use interrupts....

I don't think so. I was mistaken about SWserial. It transmits in a busy-loop, but Rx is handled right inside a single ISR handler.

https://github.com/plerup/espsoftwareserial/blob/23ae000cb2cf4d5823a2744f6b8ae831575ff135/SoftwareSerial.cpp#L224

It doesn't return from the ISR until a full byte is shifted in....not seeing how anything at all could be affected there no matter what's done in the core.

I don't use these functions but they are linked into the image nevertheless....but this costs 2596 bytes of our scarce IRAM.

Open up a new issue on IRAM w/the i2c blowing up, that way it can be followed. 2.5K is a big amount...

@freeck
Copy link

freeck commented Feb 1, 2019

Open up a new issue on IRAM w/the i2c blowing up, that way it can be followed. 2.5K is a big amount...

I will..

@freeck
Copy link

freeck commented Feb 3, 2019

I don't think so. I was mistaken about SWserial. It transmits in a busy-loop, but Rx is handled right inside a single ISR handler.

SWserial introduces a 10% CPU load in the ISR. In my case I sent 1000 chars/second, baud rate is 115200.
I observe three things:
1: Without GDBstub installed: the error rate of the messages is aprox 25%
2: With GDBstub installed (not started/attached): the error rate is aprox 95%. So it seems the interrupt-handlers are influencing each other...
3: Comparing v2.4.2 with v2.5.0: Using an idle-loopcounter in function loop(), v2.5.0 shows a decrease of performance of 60% (from 100000 to 40000 each second) compared to v2.4.2. (see also #5513)

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants