search

eclipse-cdt-cloud / cdt-gdb-adapter

CDT GDB Debug Adapter
Eclipse Public License 2.0
28 stars 40 forks source link

The incorrect state of multicore in Call Stack View #267

Closed trongle0504 closed 1 year ago

trongle0504 commented 1 year ago

When starting multicore with debugger type "amalgamator", the state of multicore was displayed in Call Stack View is incorrect.

Due to difference about the information of thread got from threadsRequest, the order of threads to store information for corresponding core at initialization and when got from GDB server is different.

So, the state of multicore was displayed in Call Stack View are incorrectly updated.

trongle0504 commented 1 year ago

Hi @jonahgraham ,

Could you please help me take a look at this PR? I think is the cause of Call Stack View showing incorrect status which was mentioned in PR https://github.com/eclipse-cdt-cloud/cdt-amalgamator/pull/19

Thank you so much!

trongle0504 commented 1 year ago

It sounds like you have a case where GDB is returning threads in a random(ish)/changing order?

We have a case where GDB is returning threads in a changing order compared to the original order. MicrosoftTeams-image (5)

If we really need to keep a consistent order here them we should simply sort the return value (result.threads) by thread id. But I need an explanation why we should be overriding GDB's ordering.

The thread order after initializing when starting multicore is set with the corresponding frameID, and when GDB is returning value with the new thread order, It causes confusion about frameID for the respective threadID in the new order of the return value by GDB.

jonahgraham commented 1 year ago

It sounds like you have a case where GDB is returning threads in a random(ish)/changing order?

We have a case where GDB is returning threads in a changing order compared to the original order. MicrosoftTeams-image (5)

Do you know why GDB is returning in the different order? Normally I would assume the order of threads as returned by GDB is the correct order to display to users as that will match what is displayed by info threads.

If we really need to keep a consistent order here them we should simply sort the return value (result.threads) by thread id. But I need an explanation why we should be overriding GDB's ordering.

The thread order after initializing when starting multicore is set with the corresponding frameID, and when GDB is returning value with the new thread order, It causes confusion about frameID for the respective threadID in the new order of the return value by GDB.

This is the part I don't understand - what is the confusion caused? Who is confused here? If it is amalgamator it sounds like bug in amalgamator. Is VSCode fine with the changing order?

jonahgraham commented 1 year ago

Please answers questions in https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/pull/267#issuecomment-1572533406

But when I commit the code, I get the build fails error, apparently due to a server problem.

I have restarted the tests.

jonahgraham commented 1 year ago

In addition to restoring one of the if (!this.isRunning) {:

Please answers questions in https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/pull/267#issuecomment-1572533406

trongle0504 commented 1 year ago

This is the part I don't understand - what is the confusion caused? Who is confused here? If it is amalgamator it sounds like bug in amalgamator. Is VSCode fine with the changing order?

I was a bit confused. The cause of confusion here is that the FrameID has not changed the order according to the threadID in the new order of the value returned by GDB. It's true that this bug should be fixed in amalgamator, but I still couldn't find a way to change it in amalgamator, so I used cdt-gdb-adapter's threadsRequest() to change the order.

I checked with this change that there will be a change in the display order in VSCode's Call Stack View, however it's fine.

jonahgraham commented 1 year ago

Where in the amalgamator is it using the array index of the threads instead of the threadId of the thread. I'm guessing in collectChildTheads?

I don't think the simple sorting by thread id in the adapter is a big problem, I still disagree with adding it as it leaves open holes related to any adapter that dynamically creates and deletes threads. However most of the time (all the time?) standard GDB returns threads in thread id order. Therefore I am going to merge this change as, compared to the first iteration of fixing this issue it seems straightforward enough.

The tests are all passing now that you have restored the if (!this.isRunning) {

jonahgraham commented 1 year ago

Latest versions now published: