[cDAC] Draft CodeVersionsTest refactor by max-charlamb · Pull Request #2 · max-charlamb/runtime

max-charlamb · 2024-12-13T22:27:24Z

No description provided.

max-charlamb · 2024-12-13T22:27:50Z

-        private IReadOnlyCollection<MockCodeBlockStart> _codeBlocks;
+    private Mock<IRuntimeTypeSystem> mockRuntimeTypeSystem = new(MockBehavior.Strict);
+    private Mock<IExecutionManager> mockExecutionManager = new(MockBehavior.Strict);
+    private Mock<ILoader> mockLoader = new(MockBehavior.Strict);


MockBehavior.Strict isn't required, but I wanted to make sure I was capturing all the calls.

max-charlamb · 2024-12-13T22:28:54Z

-            ulong addressLowBits = (ulong)address & ((ulong)_target.PointerSize - 1);
-
+            // this is not quite accurate on 32 bit architectures, but it's good enough for testing
+            ulong addressLowBits = (ulong)address & 7;


Should be (ulong)address & ((ulong)_target.PointerSize - 1);, however when we set this up the target isn't created. I guess we could create the target first and pass it in.

You should be able to get the pointer size off of the TargetTestHelpers for MockDescriptor.CodeVersions, which we create before doing this.

max-charlamb · 2024-12-13T22:29:41Z

    [ClassData(typeof(MockTarget.StdArch))]
    public void GetNativeCodeVersion_Null(MockTarget.Architecture arch)
    {
+        mockExecutionManager.Setup(e => e.GetCodeBlockHandle(TargetCodePointer.Null)).Returns(() => null);


Only required with MockBehavior.Strict. Otherwise this test relies on the mock returning null when there is no setup. I find this a bit confusing.

elinor-fung · 2024-12-14T00:04:02Z

-
-        TargetCodePointer IExecutionManager.GetStartAddress(CodeBlockHandle codeInfoHandle) => FindCodeBlock(codeInfoHandle.Address)?.StartAddress ?? TargetCodePointer.Null;
-        TargetPointer IExecutionManager.GetMethodDesc(CodeBlockHandle codeInfoHandle) => FindCodeBlock(codeInfoHandle.Address)?.MethodDescAddress ?? TargetPointer.Null;
+    private void AddCodeBlock(MockCodeBlockStart block)


Do you think MockCodeBlockStart and friends are still helpful? Versus having these method just take in the address, length, method desc, etc. as separate parameters.

I think they are still helpful to encapsulate state related to a mock. I think if we removed them, it would make tests with multiple MethodDescs confusing due to keeping track of multiple similar fields.

elinor-fung · 2024-12-14T00:42:23Z

-    internal class MockExecutionManager : IExecutionManager
-    {
-        private IReadOnlyCollection<MockCodeBlockStart> _codeBlocks;
+    private Mock<IRuntimeTypeSystem> mockRuntimeTypeSystem = new(MockBehavior.Strict);


I tend to find it easier to reason about tests when everything is contained in the test method (explicitly sets up what it needs / no state outside the method, don't have to know that each test method actually gets a new test class instance).

Maybe try something like having the test cases explicitly create the mock(s) they need and turn the Add* methods into extension methods for Mock<...>?

Happy to make this change. I settled on having a MockExtensions class which adds the helpers as extensions on the correct generic implementation of Mock<...>.

…otnet#124642) ## Summary Fixes dotnet#123621 When a constant-folded operand appears **after** a non-constant operand in a short-circuit `&&` expression (e.g., `v == 2 && Environment.NewLine != "\r\n"`), callee inlining can leave dead local stores in the return block. The `isReturnBool` lambda in `fgFoldCondToReturnBlock` required `hasSingleStmt()`, which caused the optimization to bail out when these dead stores were present, resulting in suboptimal branching codegen. ### Changes - **`src/coreclr/jit/optimizebools.cpp`**: Relax the `hasSingleStmt()` constraint in `isReturnBool` to allow preceding statements as long as they have no globally visible side effects (`GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS`). This enables `fgFoldCondToReturnBlock` to fold the conditional into a branchless return even when dead local stores from inlining remain in the block. ### Before (ARM64, `Inline_After`) ```asm cmp w0, #2 bne G_M4495_IG04 mov w0, #1 ret G_M4495_IG04: mov w0, #0 ret ``` ### After (ARM64, `Inline_After`) ```asm cmp w0, #2 cset x0, eq ret ``` ## Test plan - [x] Added regression test `Runtime_123621` covering the original issue pattern - [x] Verified `Hoisted`, `Inline_Before`, and `Inline_After` all produce identical branchless codegen (`cset` on ARM64) - [x] Verified existing `DevDiv_168744` regression test still passes - [x] Verified side-effect-ful blocks are correctly excluded from the optimization

- Fix README test filter syntax: use FullyQualifiedName~BasicAlloc (#1) - Remove goto statements from GCInfoDecoder.EnumerateLiveSlots: extract ReportUntrackedAndSucceed local function (#2) - Move CheckForSkippedFrames from Next() to UpdateState (#6) - Add XUnitConsoleRunner package reference for Helix payload (#9) - Support TypeSpec (tag=2) in DecodeTypeDefOrRefOrSpec matching native CorSigUncompressToken behavior (#10) - Fix IsAppleArm64ABI: set to false until Apple platform detection is available (filed dotnet#127282) (#11) - Fix Unix x64 float register stride: use FloatRegisterSize instead of hardcoded 8 (#12) - Replace FrameIterator.OffsetFromGCRefMapPos with CallingConventionInfo version that handles x86 reversed register layout (dotnet#13) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…128163) > [!NOTE] > This PR was authored with assistance from GitHub Copilot. Fixes dotnet#128044. ## Problem createdump SIGSEGVs on Linux when generating a Heap-type minidump for a process running interpreted code. The crash reproduces locally with the `InterpreterStack` DumpTests debuggee and matches the CI failure that prompted `<DumpTypes>Full</DumpTypes>` to be added as a temporary workaround. The faulting backtrace is: ``` #0 Thread::IsAddressInStack threads.cpp:6741 #1 Thread::EnumMemoryRegionsWorker threads.cpp:6909 (calls IsAddressInStack(currentSP)) #2 Thread::EnumMemoryRegions threads.cpp #3 ThreadStore::EnumMemoryRegions #4 ClrDataAccess::EnumMemDumpAllThreadsStack #5 ClrDataAccess::EnumMemoryRegionsWorkerHeap (HEAP2-only path) ``` ## Root cause `Thread::m_pInterpThreadContext` was declared as a raw `InterpThreadContext *`. In non-DAC code that's a normal host pointer, but in DAC mode the field's value is a target-process address. When `IsAddressInStack` (a DAC-callable helper) dereferenced `m_pInterpThreadContext->pStackStart` it read from a target-process address as if it were a host address, which faults inside createdump. ## Fix Change the field type to `PTR_InterpThreadContext` (DPTR), matching the treatment of other Thread fields like `m_pFrame`. In non-DAC builds `DPTR(T)` is just `T*`, so there is no overhead or behavior change. In DAC builds the read goes through `__DPtr<T>` and marshals correctly from the target. Also remove the `<DumpTypes>Full</DumpTypes>` workaround on the `InterpreterStack` DumpTests debuggee so the Heap path that originally failed is exercised again. ## Validation Locally reproduced the original SIGSEGV on Linux x64 with the auto-dump mechanism (`DOTNET_DbgMiniDumpType=2` + `DOTNET_Interpreter=MethodA`) running the `InterpreterStack` debuggee. With this fix applied, createdump produces a complete Heap dump (~74 MB) instead of crashing. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

An Android production app reported a native abort while building an X.509 chain on arm64. The available tombstone snippet showed the process aborting in `AndroidCryptoNative_X509ChainBuild` from `pal_x509chain.c`, with the native guard reporting that parameter `ctx` was not a valid pointer. The report did not include a repro or full tombstone, but the observed failure mode means managed code reached the native build entry point with a null `X509ChainContext*`. ``` Thread /__w/1/s/src/native/libs/System.Security.Cryptography.Native.Android/pal_x509chain.c:113 (AndroidCryptoNative_X509ChainBuild): Parameter 'ctx' must be a valid pointer *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** pid: 0, tid: 31609 >>> com.app.name <<< backtrace: #00 pc 0x000000000002232c /system/lib64/libc.so (abort+116) #1 pc 0x0000000000021fe8 [removed]-KwPZdoEumri00C7kBm3pQw==/lib/arm64/libSystem.Security.Cryptography.Native.Android.so #2 pc 0x00000000000220b0 [removed]-KwPZdoEumri00C7kBm3pQw==/lib/arm64/libSystem.Security.Cryptography.Native.Android.so (AndroidCryptoNative_X509ChainBuild+88) #3 pc 0x000000000000cfcc ``` `X509ChainContext` is created by `AndroidCryptoNative_X509ChainCreateContext`. That initialization can fail if Android certificate store setup or PKIX parameter construction throws, or if required JNI global references cannot be created. Previously, the managed Android chain path stored the returned `SafeHandle` without checking whether context creation failed, so a later build could pass a null native context to `AndroidCryptoNative_X509ChainBuild` and terminate the app process. This change makes context creation fail gracefully: - The native create path checks Java exceptions around object creation and method calls more consistently. - Partial native contexts are destroyed if global-reference creation fails. - The Android interop wrapper checks the returned chain context immediately, including a null safe-handle return, and throws `CryptographicException` if initialization failed. No regression test is included because the reliable failure modes depend on Android platform/provider state or artificial fault injection, and a test hook would be fragile and not representative. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Kevin Jones <kevin@vcsjones.com>

Three issues prevented x86 stress from passing in CI (build 1473187): 1. Leftover Console.WriteLine debug output in GCArgTable.cs flooded the Helix log with thousands of "CallArgCount/CallPndTabCnt/lastSkip/stkOffs" lines and made test triage impossible. Removed all four diagnostic writes plus their now-dead locals (stkOffs/lowBit, lastSkip+param, callPndTabSize, callPndTab read-without-use). 2. CalculatePushedArgSizeAt threw "Unsupported gc transition type" (InvalidOperationException, HRESULT 0x80131509) whenever an ESP-frame walk hit a CalleeSavedRegister transition (emitted by the partial- interrupt no-EBP decoder, case 7 / 0x04). Native scanArgRegTable treats this tag as informational (no effect on outgoing-arg depth); the cDAC switch was missing the case so every ESP-frame stack with a callee-saved-reg tag aborted GetStackReferences with hr=0x80131509. This was the source of 18-70 [FAIL] entries per debuggee in CI. 3. AddressFromGCRefMapPos used the same forward-from-FirstGCRefMapSlot formula on every architecture, but x86's TransitionBlock lays out its two argument registers (EDX/ECX) backward via ENUM_ARGUMENT_REGISTERS_BACKWARD. Native OffsetFromGCRefMapPos (frames.cpp) reverses pos 0/1 within the arg-regs area on x86. Without this fix, ExternalMethodFrame GCRefMap-driven scans reported the wrong arg-reg slot for the first two positions. Stack-arg positions (pos >= 2) remain forward, matching native. Local PInvoke debuggee under DOTNET_CdacStress=0x001: Before fix #1+#2: 70 fail / 64926 matched / 0 mismatched (every [FAIL] was an InvalidOperationException, not a real mismatch). After #1+#2: 2 fail / 66250 matched / 2 mismatched. After all three: 2 fail (remaining is a separate x86 GCRefMap decode bug in pos >= 2 stack args, tracked as follow-up). 5 of 9 debuggees now report 0 failures and 0 mismatches.

max-charlamb commented Dec 13, 2024

View reviewed changes

elinor-fung reviewed Dec 14, 2024

View reviewed changes

max-charlamb force-pushed the cdac-update-cv-tests branch from c317e85 to 658ee65 Compare December 16, 2024 17:33

update CodeVersions unit tests

372952c

max-charlamb force-pushed the cdac-update-cv-tests branch from 658ee65 to 372952c Compare December 16, 2024 17:47

max-charlamb changed the base branch from md-gccover to main December 16, 2024 17:48

max-charlamb changed the base branch from main to md-dynamic December 16, 2024 17:49

max-charlamb changed the base branch from md-dynamic to main December 16, 2024 17:49

max-charlamb mentioned this pull request Dec 16, 2024

[cDAC] Use Moq in CodeVersionsTest dotnet/runtime#110750

Merged

max-charlamb closed this Mar 21, 2025

max-charlamb deleted the cdac-update-cv-tests branch June 10, 2025 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cDAC] Draft CodeVersionsTest refactor#2

[cDAC] Draft CodeVersionsTest refactor#2
max-charlamb wants to merge 1 commit into
mainfrom
cdac-update-cv-tests

max-charlamb commented Dec 13, 2024

Uh oh!

max-charlamb Dec 13, 2024

Uh oh!

max-charlamb Dec 13, 2024

Uh oh!

elinor-fung Dec 13, 2024

Uh oh!

max-charlamb Dec 13, 2024

Uh oh!

elinor-fung Dec 14, 2024

Uh oh!

max-charlamb Dec 16, 2024

Uh oh!

elinor-fung Dec 14, 2024

Uh oh!

max-charlamb Dec 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

max-charlamb commented Dec 13, 2024

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants