GDB: Extended functionality by mrajwa · Pull Request #965 · thesofproject/sof

mrajwa · 2019-02-05T16:28:09Z

Added support for following packets:

breakpoint insert
breakpoint remove
single step in code
read/write registers
read/write memory

lgirdwood

In general only the generic GDB code should exist in src/gdb. Anything that is xtensa specific (either C or assembler) must be in arch/xtensa/gdb and use a arch_gdb_ call prefix. This then makes the GDB code easily portable to other DSP architectures e.g ARM M4, Ceva Teaklite since only the arch_gdb_ APIs need to be implemented.

lgirdwood · 2019-02-06T10:06:45Z

indentation looks wrong.

Yes, I moved whole while loop max to the left on purpose - we have many switches which are sitting inside another switches and these switches are inside while loop, there are of cource ifs all around... so to meet the 80 characters MAX limit I would have to slice each line (it for sure won't improve code readability). Also keep in mind that after the move of xtensa specific stuff into arch/xtensa/gdb our write_sr function has become arch_gdb_write_sr(arg1, arg2); - it alone took 30 characters!
So the bottom line is, I would sacrifice this while(1){ indentation in this case.

@lgirdwood @mrajwa
Move loop body to another function (may be static inline if you care about call performance penalty), then you'll keep indent of that part

Imho still unresolved. Please change it.

dbaluta · 2019-02-06T11:42:02Z

@mrajwa few general comments:

https://chris.beams.io/posts/git-commit/#end
https://chris.beams.io/posts/git-commit/#imperative
no need to write all caps for the commit message tag.

So, for example:

GDB: Added support for insert breakpoint.

should be

gdb: Add support for insert breakpoint

mrajwa · 2019-02-13T13:29:12Z

@lgirdwood changes have been applied. Once this PR is merged people may use GDB.

lgirdwood

Only some minor issues but can you squash the review comments into the earlier patches in this PR. Look forward to using GDB !

lgirdwood · 2019-02-13T19:00:07Z

intentional or can it be removed ?

Can be removed and actually was in "cleanup" commit.

lgirdwood · 2019-02-13T19:02:18Z

"out of scope" is a more meaningful message if its correct ?

Yes it is, however the error message must contain only HEX characters 0..F otherwise it may cause GDB to close the session.

lgirdwood · 2019-02-13T19:11:01Z

some magic numbers here that I dont know what they mean. Can we use macros

plbossart · 2019-03-28T15:32:52Z

@lgirdwood should this be in 1.3?

lgirdwood · 2019-03-29T12:00:37Z

@mrajwa @michalgrodzicki we could do with this in 1.3, this would let the kernel guys add driver support. @cujomalainey already has some example kernel code but we need this to complete integration.

lgirdwood · 2019-04-01T14:08:03Z

@mrajwa @michalgrodzicki ping, this is very close to merge now. Can we get it to the finish line for 1.3 this week ?

cujomalainey · 2019-04-01T20:12:01Z

Here is the archived repo https://github.com/cujomalainey/cros-kernel
This is pre-github days for SOF and this is running a 4.4 kernel (i think?)
It is also an experiment so just be aware, but basically I modified a virtual tty loopback to use the audio driver and then that connected to the firmware.

mrajwa · 2019-04-01T20:59:12Z

@lgirdwood, sure I will update soon.

lgirdwood · 2019-04-02T08:56:22Z

@cujomalainey thanks. @plbossart @ranj063 see comment above for kernel GDB tunnel code. This will need integration into sof-dev kernel branch.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa · 2019-04-02T22:08:48Z

@lgirdwood, update have been pushed, is there anything that you would still like to change/correct before merging it to master?

PS: I do work with @ranj063 on preparing driver daemon for it.

lgirdwood

This code is good but is still not portable. All xtensa specific code and macros need to be out of src/gdb/.

lgirdwood

@mrajwa I think I was looking at old/stale commits about 2 hours ? Which is a little confusing. The new update had less patches and were portable.

mrajwa · 2019-04-03T22:32:43Z

@lgirdwood I don't know. I made the update yesterday once and from that time haven't touched it. Also, the number of commits is exactly the same as it was before my yesterday update.
Anyway, if it is fine for you now lets merge it :)

lgirdwood · 2019-04-04T09:57:36Z

@mrajwa CI reporting an issue plus we need 2 approvers now before merge.@jajanusz @tlauda can you guys review.

jajanusz · 2019-04-04T11:02:33Z

Imho still unresolved. Please change it.

mrajwa · 2019-04-04T11:06:47Z

@lgirdwood, its passing now, looks like temporary CI problem.
@jajanusz, ok.

mrajwa · 2019-04-04T18:37:37Z

@jajanusz @lgirdwood updated.

jajanusz · 2019-04-04T19:38:31Z

ret is always 1 here, no need to introduce new variable

mrajwa · 2019-04-05T09:26:52Z

@jajanusz updated.

jajanusz · 2019-04-05T10:26:02Z

Weird that compiler didn't complain that there are exectution path without return value

jajanusz · 2019-04-05T10:27:25Z

I just realized that you can do anything with GDB and it will pass CI because we don't test configs with CONFIG_GDB ...

mrajwa · 2019-04-05T12:03:59Z

Seems to be OK now, @tlauda can you put your comment to?

lgirdwood · 2019-04-05T13:12:57Z

@mrajwa looks like we are waiting on CI completion too.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

lgirdwood · 2019-04-08T12:26:14Z

@zrombel CI not running ?

jajanusz · 2019-04-08T12:42:47Z

@zrombel CI not running ?

It's running but queue is long, as always on mondays

mrajwa requested review from lbetlej, lgirdwood, mmaka1 and plbossart as code owners February 5, 2019 16:28

lgirdwood requested changes Feb 6, 2019

View reviewed changes

dbaluta reviewed Feb 6, 2019

View reviewed changes

Comment thread src/gdb/gdb.c Outdated

mrajwa force-pushed the gdb_support branch from 830c5fe to cce3ae6 Compare February 13, 2019 12:24

mrajwa requested a review from jajanusz as a code owner February 13, 2019 12:24

lgirdwood requested changes Feb 13, 2019

View reviewed changes

lgirdwood added this to the 1.3 milestone Mar 29, 2019

mrajwa added 10 commits April 2, 2019 23:09

GDB: Added support for insert breakpoint.

0e64ede

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

GDB: Added the possibility to remove breakpoints.

948ad84

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

GDB: Added support for single step.

5e9b3b8

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

GDB: Added support for register read.

bc90831

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

GDB: Added support for register write.

1c8879e

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

GDB: Added support for memory read.

3020325

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

GDB: Added support for memory write (binary mode).

8b06b35

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

GDB: Added support for memory write.

460e139

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

GDB: fix "step" followed by "continue" issue

2cae346

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

GDB: move Xtensa specific stuff into arch/xtensa/gdb/*

0d04549

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa force-pushed the gdb_support branch from cce3ae6 to 32be251 Compare April 2, 2019 22:02

lgirdwood requested changes Apr 3, 2019

View reviewed changes

Comment thread src/gdb/gdb.c Outdated

lgirdwood approved these changes Apr 3, 2019

View reviewed changes

jajanusz suggested changes Apr 4, 2019

View reviewed changes

Comment thread src/gdb/gdb.c Outdated

jajanusz Apr 4, 2019

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Imho still unresolved. Please change it.

mrajwa force-pushed the gdb_support branch from 32be251 to e224227 Compare April 4, 2019 18:16

jajanusz suggested changes Apr 4, 2019

View reviewed changes

Comment thread src/gdb/gdb.c Outdated

jajanusz Apr 4, 2019

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

ret is always 1 here, no need to introduce new variable

mrajwa force-pushed the gdb_support branch from e224227 to ee3636a Compare April 5, 2019 09:24

jajanusz approved these changes Apr 5, 2019

View reviewed changes

mrajwa force-pushed the gdb_support branch from ee3636a to 3113ace Compare April 5, 2019 10:27

jajanusz self-requested a review April 5, 2019 10:27

jajanusz approved these changes Apr 5, 2019

View reviewed changes

jajanusz reviewed Apr 5, 2019

View reviewed changes

Comment thread src/arch/xtensa/gdb/CMakeLists.txt Outdated

GDB: cleanup and comments

edc3fc9

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa force-pushed the gdb_support branch from 3113ace to edc3fc9 Compare April 8, 2019 11:56

jajanusz merged commit 18d116b into thesofproject:master Apr 8, 2019

Uh oh!

Conversation

mrajwa commented Feb 5, 2019

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbaluta commented Feb 6, 2019

Uh oh!

Uh oh!

mrajwa commented Feb 13, 2019

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plbossart commented Mar 28, 2019

Uh oh!

lgirdwood commented Mar 29, 2019

Uh oh!

lgirdwood commented Apr 1, 2019

Uh oh!

cujomalainey commented Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrajwa commented Apr 1, 2019

Uh oh!

lgirdwood commented Apr 2, 2019

Uh oh!

mrajwa commented Apr 2, 2019

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgirdwood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrajwa commented Apr 3, 2019

Uh oh!

lgirdwood commented Apr 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrajwa commented Apr 4, 2019

Uh oh!

mrajwa commented Apr 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrajwa commented Apr 5, 2019

Uh oh!

jajanusz commented Apr 5, 2019

Uh oh!

cujomalainey commented Apr 1, 2019 •

edited

Loading

lgirdwood left a comment •

edited

Loading