fix: don't cache asyncMethodSteppingInfo

[noam-sol]

Fixes #129

I have found that sometimes asyncMethodSteppingInfo.lastIlOffset is 0, and always initializing it gives back a correct value.

for example here - https://github.com/Samsung/netcoredbg/blob/3fcd3e8bc3f0b1cc520e60a987a7a4176c3ddee3/src/debugger/stepper_async.cpp#L276 it compares the offset to 0 and then steps out and not steps-over.

This also fixes #129

[viewizard]

I have found that sometimes asyncMethodSteppingInfo.lastIlOffset is 0, and always initializing it gives back a correct value.

This case should be investigated first and real point of issue should be found before provide any fixes.

The main reason of asyncMethodSteppingInfo present is don't search data for same method all the time on each step and speed up stepping in this way. You just remove cache re-usage instead of real fix.

[viewizard]

Here is fix:

From b544891b72849fca9dae96d690e9a8fd7090824c Mon Sep 17 00:00:00 2001
From: Mikhail Kurinnoi <m.kurinnoi@samsung.com>
Date: Thu, 6 Jul 2023 00:17:11 +0300
Subject: [PATCH] Fix async stepping.

---
 src/metadata/async_info.cpp | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/metadata/async_info.cpp b/src/metadata/async_info.cpp
index 8aab0ec..badca7d 100644
--- a/src/metadata/async_info.cpp
+++ b/src/metadata/async_info.cpp
@@ -28,7 +28,13 @@ HRESULT AsyncInfo::GetAsyncMethodSteppingInfo(CORDB_ADDRESS modAddress, mdMethod

         HRESULT Status;
         std::vector<Interop::AsyncAwaitInfoBlock> AsyncAwaitInfo;
-        IfFailRet(Interop::GetAsyncMethodSteppingInfo(mdInfo.m_symbolReaderHandles[methodVersion - 1], methodToken, AsyncAwaitInfo, &asyncMethodSteppingInfo.lastIlOffset));
+        if (FAILED(Status = Interop::GetAsyncMethodSteppingInfo(mdInfo.m_symbolReaderHandles[methodVersion - 1], methodToken, AsyncAwaitInfo, &asyncMethodSteppingInfo.lastIlOffset)))
+        {
+            asyncMethodSteppingInfo.modAddress = 0;
+            asyncMethodSteppingInfo.methodToken = 0;
+            asyncMethodSteppingInfo.methodVersion = 0;
+            return Status;
+        }

         for (const auto &entry : AsyncAwaitInfo)
         {
-- 
2.25.1

[viewizard]

or better version:

From 89b7c682ab1a3c3e1e169040d42cb765f95dad73 Mon Sep 17 00:00:00 2001
From: Mikhail Kurinnoi <m.kurinnoi@samsung.com>
Date: Thu, 6 Jul 2023 00:27:14 +0300
Subject: [PATCH] Fix async stepping.

---
 src/metadata/async_info.cpp | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/metadata/async_info.cpp b/src/metadata/async_info.cpp
index 8aab0ec..c4d3414 100644
--- a/src/metadata/async_info.cpp
+++ b/src/metadata/async_info.cpp
@@ -21,6 +21,10 @@ HRESULT AsyncInfo::GetAsyncMethodSteppingInfo(CORDB_ADDRESS modAddress, mdMethod
     if (!asyncMethodSteppingInfo.awaits.empty())
         asyncMethodSteppingInfo.awaits.clear();

+    asyncMethodSteppingInfo.modAddress = 0;
+    asyncMethodSteppingInfo.methodToken = 0;
+    asyncMethodSteppingInfo.methodVersion = 0;
+
     return m_sharedModules->GetModuleInfo(modAddress, [&](ModuleInfo &mdInfo) -> HRESULT
     {
         if (mdInfo.m_symbolReaderHandles.empty() || mdInfo.m_symbolReaderHandles.size() < methodVersion)
-- 
2.25.1

[viewizard]

Hmm... looks like cache don't work for normal methods at all, will fix this too.

[noam-sol]

Hmm... looks like cache don't work for normal methods at all, will fix this too.

Thanks!

[viewizard]

Here is the fix, that also add cache logic for normal methods:

From 98fefe97496ddb7f4fba9743cdfeab366acc677c Mon Sep 17 00:00:00 2001
From: Mikhail Kurinnoi <m.kurinnoi@samsung.com>
Date: Thu, 6 Jul 2023 00:37:30 +0300
Subject: [PATCH] Fix async stepping.

---
 src/metadata/async_info.cpp | 16 ++++++++++------
 src/metadata/async_info.h   |  3 ++-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/src/metadata/async_info.cpp b/src/metadata/async_info.cpp
index 8aab0ec5e..8ca75c5aa 100644
--- a/src/metadata/async_info.cpp
+++ b/src/metadata/async_info.cpp
@@ -13,15 +13,21 @@ namespace netcoredbg
 // Caller must care about m_asyncMethodSteppingInfoMutex.
 HRESULT AsyncInfo::GetAsyncMethodSteppingInfo(CORDB_ADDRESS modAddress, mdMethodDef methodToken, ULONG32 methodVersion)
 {
+    // Note, for normal methods, `Interop::GetAsyncMethodSteppingInfo()` will return error code and set `lastIlOffset` to 0.
+    // Error during async info search (debug info not available or method token belong to normal method) is proper behaviour and debugger logic also count on this.
+
     if (asyncMethodSteppingInfo.modAddress == modAddress &&
         asyncMethodSteppingInfo.methodToken == methodToken &&
         asyncMethodSteppingInfo.methodVersion == methodVersion)
-        return S_OK;
+        return asyncMethodSteppingInfo.retCode;

     if (!asyncMethodSteppingInfo.awaits.empty())
         asyncMethodSteppingInfo.awaits.clear();

-    return m_sharedModules->GetModuleInfo(modAddress, [&](ModuleInfo &mdInfo) -> HRESULT
+    asyncMethodSteppingInfo.modAddress = modAddress;
+    asyncMethodSteppingInfo.methodToken = methodToken;
+    asyncMethodSteppingInfo.methodVersion = methodVersion;
+    asyncMethodSteppingInfo.retCode = m_sharedModules->GetModuleInfo(modAddress, [&](ModuleInfo &mdInfo) -> HRESULT
     {
         if (mdInfo.m_symbolReaderHandles.empty() || mdInfo.m_symbolReaderHandles.size() < methodVersion)
             return E_FAIL;
@@ -35,12 +41,10 @@ HRESULT AsyncInfo::GetAsyncMethodSteppingInfo(CORDB_ADDRESS modAddress, mdMethod
             asyncMethodSteppingInfo.awaits.emplace_back(entry.yield_offset, entry.resume_offset);
         }

-        asyncMethodSteppingInfo.modAddress = modAddress;
-        asyncMethodSteppingInfo.methodToken = methodToken;
-        asyncMethodSteppingInfo.methodVersion = methodVersion;
-
         return S_OK;
     });
+
+    return asyncMethodSteppingInfo.retCode;
 }

 // Check if method have await block. In this way we detect async method with awaits.
diff --git a/src/metadata/async_info.h b/src/metadata/async_info.h
index 16f952516..a4ced8963 100644
--- a/src/metadata/async_info.h
+++ b/src/metadata/async_info.h
@@ -47,13 +47,14 @@ private:
         CORDB_ADDRESS modAddress;
         mdMethodDef methodToken;
         ULONG32 methodVersion;
+        HRESULT retCode;

         std::vector<AwaitInfo> awaits;
         // Part of NotifyDebuggerOfWaitCompletion magic, see ManagedDebugger::SetupAsyncStep().
         ULONG32 lastIlOffset;

         AsyncMethodInfo() :
-            modAddress(0), methodToken(mdMethodDefNil), methodVersion(0), awaits(), lastIlOffset(0)
+            modAddress(0), methodToken(mdMethodDefNil), methodVersion(0), retCode(S_OK), awaits(), lastIlOffset(0)
         {};
     };

[gbalykov]

Fix is part of latest release