Convert funceval to UCO pattern#126809
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
|
I am not sure which one will go first. If these changes are independent, it would make more sense to submit them as independent PRs. |
ffa8a51 to
695f29e
Compare
jkotas
left a comment
There was a problem hiding this comment.
One of the Microsoft maintainers will have to run internal VS debugger tests on this before it gets merged.
|
Failures are known, the new one is unrelated deadlettered leg. Results still match before/after #126809 (comment) |
|
Let me run the private diagnostic tests with this change. |
|
There is about 15 failed tests with this change (compared to the commit right before your change). Moreover, I've found that we also have some additional failures in these tests that one of your previous changes - #126222 has introduced. Since I have run those tests at one point during your PR, it seems that some changes made after that has introduced the problem. |
|
So, regarding the issues introduced by the old PR, the DacDbiInterfaceImpl::UnwindStackWalkFrame needs to be updated to skip frames of "System.Environment.CallEntryPoint". I've tried a quick hack (just comparing method name string) and found that fixes two of the three EH related failures. The remaining issue from the old PR is strange. The debugger can see a stack where the System.Environment.CallEntryPoint is at the top of the stack, with Main at the bottom. That doesn't make sense, I am looking at the test to see if I can isolate a standalone repro. |
|
@dotnet/dotnet-diag-contrib Any concerns with this change? It is switching funceval to use reflection invoke. It makes the code a lot simpler and fully portable that is going to help with the wasm port. |
noahfalk
left a comment
There was a problem hiding this comment.
This seems like a great simplification. Looking over it it appears to be doing the right things. There are lots of subtleties I'm unlikely to catch just by reading the code so I'm also relying heavily on the testing you did @janvorli (thanks!)
@tommcdon - is there any additional testing we could do on this, perhaps VS, that would help flush out lingering issues?
|
Thanks, will keep an eye out post-merge for any fallouts after daily SDK is released. (I think folks will be able to test conveniently after the signed binaries are out) |
|
@janvorli, @tommcdon, a note: this branch should be built from clean state (i.e. after deleting artifacts dir). I noticed that cmake or ninja doesn't reinstall some DAC related stuff if we built main before rebuilding this branch and we run into mismatch error when using debugger in VS with |
|
@janvorli, would it be possible to share the failing test scenarios so I can investigate? If not, I can close this PR and |
Hi @aml11! I apologize for the delay! Here's a summary of the issues that we found:
Nested break state shows a Repro case: // Simpler repro via Visual Studio:
// 1. Open this project in VS 2022/2026 (with your custom .NET 11 runtime)
// 2. Set a breakpoint on the first line after the curly brace in Return42 -- ("int i = 0; i++;")
// 3. Run; pause at first Debugger.Break() (the one in Main)
// 4. In VS Immediate Window, evaluate: Return42()
// (VS uses "all threads evaluate" mode for Immediate, which allows nested breakpoints)
// 5. The breakpoint inside Return42 fires; open the Call Stack window
// Expected call stack entry: " Evaluation of: Return42()"
// Actual call stack entry: " [Lightweight Function]"
Debugger.Break(); // Pause here, then evaluate Return42() in the Immediate window
// with a breakpoint set inside Return42
Console.WriteLine(Return42());
static int Return42()
{
int i = 0; i++; // <-- set breakpoint here; check call stack during func-eval
return 42;
}
Evaluating // Repro steps (Visual Studio):
// 1. Attach VS 2022/2026 with your custom runtime
// 2. Run the program — it pauses at Debugger.Break()
// 3. Open Immediate Window (Debug > Windows > Immediate) or Watch window
// 4. Evaluate: test.AsSpan().Slice(0, 2)
//
// Expected result:
// System.Span<int>[2] (a span containing elements 0 and 1)
//
// Actual result (with UCO commit):
// 'test.AsSpan().Slice(0, 2)' threw an exception of type 'System.NotSupportedException'
// Message: "Specified method is not supported."
//
// Root cause hypothesis:
// InvokeFuncEval (the UCO trampoline) uses MethodBase.Invoke() to call the target method.
// Span<T> is a ref struct and cannot be boxed into object[]. When funceval.cpp attempts to
// box the Span<T> arguments or return value for the object[] array, it fails.
// The old PackArgumentArray / ARG_SLOT-based approach handled ref structs directly.
using System.Diagnostics;
int[] test = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
// Pause here. Evaluate: test.AsSpan().Slice(0, 2)
// Expected: System.Span<int>[2]
// Actual: throws System.NotSupportedException
Debugger.Break();
Console.WriteLine("Done");
Evaluating Similarly, // Repro steps (Visual Studio):
// 1. Attach VS 2022/2026 with your custom runtime
// 2. Run the program — it pauses at Debugger.Break()
// 3. Open Immediate Window (Debug > Windows > Immediate) or Watch window
// 4. Evaluate each expression below and check the result
//
// Test 1 - DateTime constructor:
// Evaluate: date = new DateTime(2077, 5, 24, 3, 20, 52, DateTimeKind.Utc)
// Expected: {5/24/2077 3:20:52 AM}
// Actual: {12/18/2019 3:20:52 AM} <-- stale pre-existing value
//
// Test 2 - decimal constructor:
// Evaluate: d = new decimal(5.0)
// Expected: 5
// Actual: 2 <-- stale pre-existing value
//
// Root cause hypothesis:
// InvokeFuncEval calls ConstructorInfo.Invoke(thisObj, args) where thisObj is the
// pre-allocated boxed value. For value type constructors, MethodBase.Invoke may not
// mutate the pre-allocated box in-place (it may box/unbox internally, losing changes),
// so pDE->m_result still holds the original object's data.
//
// NOTE: The returned EvalResult shows the OLD value (before construction), proving
// that the constructor ran on a different copy or the box was not updated.
using System.Diagnostics;
DateTime date = new DateTime(2019, 12, 18, 3, 20, 52, DateTimeKind.Utc);
decimal d = new decimal(2.0);
// Pause here. Evaluate:
// date = new DateTime(2077, 5, 24, 3, 20, 52, DateTimeKind.Utc) -> expected {5/24/2077 3:20:52 AM}
// d = new decimal(5.0) -> expected 5
Debugger.Break();
Console.WriteLine($"date = {date}, d = {d}"); |
|
Thanks for sharing clear repro @tommcdon. I was able to repro it. I have pushed a commit with fixes. Now Span.Slice, stale assignment and extra frames issues are fixed. Please let me know if you find some other fallouts. |
| pBufferForArgsArray | ||
| DEBUG_ARG(pDataLocationArray) | ||
| ); | ||
| // Byref-like args (e.g. Span<T>) need special handling: we cannot route them through |
There was a problem hiding this comment.
@noahfalk How does CordbValue works for byref-like types (e.g. Span)? I do not see anything that protects the byref fields in these types when they are wrapped by CorDbValue. Is that a bug or am I missing some invariant that makes it unnecessary?



Built on top of #126542.Last part of #123864 before the final cleanups.