Explicitly set thread ID when getting stack depth

[jreineckearm]

Found by @jonahgraham during code review:

We have a couple of calls to mi.sendStackInfoDepth which do not receive a threadId parameter although we have the knowledge in the surrounding code.

This should in theory be fine because the affected functionality is always executed on the currently selected thread. Which should be correctly selected in GDB. But we should make use of exact knowledge when we have it, like for most of these calls. Especially if there is a lot going on in the GUI which could change the thread selection while operations are in flight.

[jonahgraham]

This should in theory be fine because the affected functionality is always executed on the currently selected thread.

Actually I don't think it is ok ever, we just generally get away with it because we (and users) mostly only look at frame 0 (top of the stack) so the fact we get the wrong depth doesn't matter. Generally we never change the selected thread with the adpater.

To demonstrate, modify the multithread test like this:

diff --git a/src/integration-tests/multithread.spec.ts b/src/integration-tests/multithread.spec.ts
index 19d018a..c0a4185 100644
--- a/src/integration-tests/multithread.spec.ts
+++ b/src/integration-tests/multithread.spec.ts
@@ -52,7 +52,7 @@ describe('multithread', async function () {
         await dc.stop();
     });
 
-    it('sees all threads', async function () {
+    it.only('sees all threads', async function () {
         if (!gdbNonStop && os.platform() === 'win32' && isRemoteTest) {
             // The way thread names are set in remote tests on windows is unsupported
             this.skip();
@@ -163,6 +163,9 @@ describe('multithread', async function () {
                     frameId: stack.body.stackFrames[0].id,
                 });
                 const vr = scopes.body.scopes[0].variablesReference;
+                dc.send('cdt-gdb-tests/executeCommand', {
+                    command: 'info thread',
+                });
 
                 const vars = await dc.variablesRequest({
                     variablesReference: vr,

This will show us the selected thread (i.e. what -stack-info-depth will run on if --thread is not provided)

Then run the test, and look at test-logs/multithread/defaults/sees all threads.log - in it you will see we are requesting info on thread 2, but getting depth of thread 1:

[14:14:50.675 UTC] From client: variables({"variablesReference":1011})
[14:14:50.675 UTC] GDB command: 31 -stack-info-depth 100
[14:14:50.675 UTC] GDB write command: 31 -stack-info-depth 100
[14:14:50.676 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"log","output":"info thread\n"}}
[14:14:50.676 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"  Id   Target Id                                        Frame \n"}}
[14:14:50.676 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"* 1    Thread 0x7ffff7f9f740 (LWP 267972) \"MultiThread\" main (argc=1, argv=0x7fffffffd5b8) at MultiThread.cc:92\n"}}
[14:14:50.676 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"  2    Thread 0x7ffff7bff6c0 (LWP 267975) \"monday\"      futex_wait (futex_word=0x7fffffffd324, expected=0, private=0) at ../sysdeps/nptl/futex-internal.h:146\n"}}
[14:14:50.676 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"  3    Thread 0x7ffff73fe6c0 (LWP 267976) \"tuesday\"     futex_wait (futex_word=0x7fffffffd324, expected=0, private=0) at ../sysdeps/nptl/futex-internal.h:146\n"}}
[14:14:50.676 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"  4    Thread 0x7ffff6bfd6c0 (LWP 267977) \"wednesday\"   futex_wait (futex_word=0x7fffffffd324, expected=0, private=0) at ../sysdeps/nptl/futex-internal.h:146\n"}}
[14:14:50.676 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"  5    Thread 0x7ffff63fc6c0 (LWP 267978) \"thursday\"    futex_wait (futex_word=0x7fffffffd324, expected=0, private=0) at ../sysdeps/nptl/futex-internal.h:146\n"}}
[14:14:50.676 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"  6    Thread 0x7ffff5bfb6c0 (LWP 267979) \"friday\"      futex_wait (futex_word=0x7fffffffd324, expected=0, private=0) at ../sysdeps/nptl/futex-internal.h:146\n"}}
[14:14:50.676 UTC] GDB result: 30 done
[14:14:50.676 UTC] GDB command: 30 info thread completed with data
[14:14:50.676 UTC] GDB result: 31 done,depth="1"
[14:14:50.676 UTC] GDB command: 31 -stack-info-depth 100 completed with data
[14:14:50.676 UTC] To client: {"seq":0,"type":"response","request_seq":14,"command":"cdt-gdb-tests/executeCommand","success":true,"body":{"status":"Ok","result":{"cdt-token":"30","cdt-command":"info thread"},"console":["info thread\n","  Id   Target Id                                        Frame \n","* 1    Thread 0x7ffff7f9f740 (LWP 267972) \"MultiThread\" main (argc=1, argv=0x7fffffffd5b8) at MultiThread.cc:92\n","  2    Thread 0x7ffff7bff6c0 (LWP 267975) \"monday\"      futex_wait (futex_word=0x7fffffffd324, expected=0, private=0) at ../sysdeps/nptl/futex-internal.h:146\n","  3    Thread 0x7ffff73fe6c0 (LWP 267976) \"tuesday\"     futex_wait (futex_word=0x7fffffffd324, expected=0, private=0) at ../sysdeps/nptl/futex-internal.h:146\n","  4    Thread 0x7ffff6bfd6c0 (LWP 267977) \"wednesday\"   futex_wait (futex_word=0x7fffffffd324, expected=0, private=0) at ../sysdeps/nptl/futex-internal.h:146\n","  5    Thread 0x7ffff63fc6c0 (LWP 267978) \"thursday\"    futex_wait (futex_word=0x7fffffffd324, expected=0, private=0) at ../sysdeps/nptl/futex-internal.h:146\n","  6    Thread 0x7ffff5bfb6c0 (LWP 267979) \"friday\"      futex_wait (futex_word=0x7fffffffd324, expected=0, private=0) at ../sysdeps/nptl/futex-internal.h:146\n"]}}

But... That said, I don't understand the use of the depth the times it is obtained because it is just used to key into the vars with this comment:

stack depth necessary for differentiating between similarly named variables at different stack depths

So, I don't know what is going on exactly. But I still think something is wrong here. I have a new test that attempts to cover variables in a multi-threaded system at different depths. The existing tests are a little oversimplified, so I hope my new test is better in that regard. The test passes, but shows that the depth being contributed as part of the key isn't really doing anything? More experimentation coming.

[jonahgraham]

My new test is more complicated, but still doesn't cover the original intention of why depth is used in the var manager. I suspect there are times when we are creating new var object when we don't need to be, but because we don't really take full advantage of the var mechanism it probably doesn't matter too much.

[jreineckearm]

Thanks, @jonahgraham . I agree something is wrong. And also start having a feeling depth is redundant because of carrying a frame ID in the variable key....

[jreineckearm]

@jonahgraham , I was looking again into #391 . And I am starting to get a feeling that something is seriously broken the depth usage. Not sure at this point if passing in the thread ID with #449 helped or if it more regularly uncovers trouble now. I'd strongly recommend to wait with the next release until we got this one sorted.

[jreineckearm]

One thing to note and that need to be fixed is that stack depth calls became asymmetric. setVariableRequest and handleVariableRequestFrame have calls which also should pass the thread ID.

[jonahgraham]

Thanks @jreineckearm - my reading of the above messages show me you went down a similar rabbit hole that I did.

One important thing to note about our implementation is that I don't think we use gdb var objects well. We do have a var manager, but we don't take advantage of the functionality of var objects (in particular we don't ask gdb to notify us when vars change*, we do everything by polling every single variable). This makes everything much more complicated than it probably needs to be, and we never delete vars, which means we probably have a memory leak/reduced performance on very long runs with lots of different variables. My open question is, do we need to keep var objects around at all, or can we do everything with either temporary var objects and/or direct methods. If we don't keep vars around, then we don't need those particular depth calls at all.

* We do var-update on each variable rather than doing -var-update *

[jreineckearm]

Thanks, @jonahgraham .

Yes, I realized the same. Especially that "var-spaces" are only cleaned when reused. This can indeed become memory-consuming when lots of different stack depths + stack frame selections are found/happen during the lifetime of a debug session.
If I understand correctly from earlier comments, this is to reduce the number of var-create/var-delete calls. Which I can understand for slow MI connections.

But even if we keep the var-buffering, there is some room for simplifying code around parents and children and arrays and pointers and how to assemble evaluate names.

I am currently in the process of better understanding the whole area as we are seeing issues with more complex combinations of structs, pointers, arrays which we need to get under control for a more reliable debug experience. Especially in combination with evaluate requests from other view extensions.

I'll get back to this next week as I am about to head out. And we have public holiday in Germany tomorrow.

I'll also have a brief look if things really regressed with recent changes. Or if I am just seeing problems that were there before. I'd really like to get the aux GDB stuff into early users' hands.

[jreineckearm]

Quick update on the oddities I saw: these were caused by an optimized out pointer-to-struct object where I successfully ignored the <optimized out> value of the variable itself.
We should disallow to expand such variables. This otherwise ends up in some decent confusion as var object children don't show <optimized out> due to empty values. They show nothing or types.
Entering an expression in the Watch window which includes struct members/arrays again gives you the <optimized out> value as expected.

[jreineckearm]

Made the sendStackInfoDepth calls consistent. But the varobject caching where this is used needs improvement. At the the moment, its strategy is based on thread, frame, and stack depth keys and doesn't seem to have a proper clear-strategy. There is some risk for long running real-world use cases where this may unnecessarily consume memory.
Keeping this ticket open until creating a follow-up for that which references the above history.