ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/runtime
File: src/coreclr/debug/daccess/inspect.cpp (C++)

Code before:
            EX_RETHROW;
        }
    }
    EX_END_CATCH

    DAC_LEAVE();
    return status;
}

HRESULT STDMETHODCALLTYPE
ClrDataValue::StartEnumFields(
    /* [in] */ ULONG32 flags,
    /* [in] */ IXCLRDataTypeInstance *fromType,
    /* [out] */ CLRDATA_ENUM *handle)
{
    HRESULT status;

    DAC_ENTER_SUB(m_dac);

    EX_TRY
    {
        if (m_typeHandle.IsNull())
        {
            *handle = 0;
            status = E_NOTIMPL;
        }
        else
        {
            status = SplitName::
                CdStartField(NULL,
                             0,
                             flags,
                             fromType,
                             m_typeHandle,
                             NULL,
                             mdTypeDefNil,
                             m_baseAddr,
                             m_thread,
                             NULL,
                             m_appDomain,
                             NULL,
                             NULL,
                             handle);
        }
    }
    EX_CATCH
    {
        if (!DacExceptionFilter(GET_EXCEPTION(), m_dac, &status))
        {
            EX_RETHROW;
        }

Review comment (suggestion):
These checks are not necessary nor consistent. All the member functions are directly using `m_typeHandle` assuming it's valid. The `EX_CATCH` macro should also provide guard for this.

Code after:
            EX_RETHROW;
        }
    }
    EX_END_CATCH

    DAC_LEAVE();
    return status;
}

HRESULT STDMETHODCALLTYPE
ClrDataValue::StartEnumFields(
    /* [in] */ ULONG32 flags,
    /* [in] */ IXCLRDataTypeInstance *fromType,
    /* [out] */ CLRDATA_ENUM *handle)
{
    HRESULT status;

    DAC_ENTER_SUB(m_dac);

    EX_TRY
    {
        status = SplitName::
            CdStartField(NULL,
                         0,
                         flags,
                         fromType,
                         m_typeHandle,
                         NULL,
                         mdTypeDefNil,
                         m_baseAddr,
                         m_thread,
                         NULL,
                         m_appDomain,
                         NULL,
                         NULL,
                         handle);
    }
    EX_CATCH
    {
        if (!DacExceptionFilter(GET_EXCEPTION(), m_dac, &status))
        {
            EX_RETHROW;
        }
    }
    EX_END_CATCH

    DAC_LEAVE();
    return status;
}

HRESULT STDMETHODCALLTYPE
Repository: dotnet/runtime
File: src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Wasm/WasmTypeNode.cs (C#)

Code before:
            _types = types;
        }

        public override bool IsShareable => true;

        public override int ClassCode => -45678931;

        public override bool StaticDependenciesAreComputed => true;

        public override ObjectNodeSection GetSection(NodeFactory factory) => ObjectNodeSection.WasmTypeSection;

        protected override string GetName(NodeFactory factory)
            => $"Wasm Type ({string.Join(",", _types.Skip(1))}) -> {_types.FirstOrDefault()}";

        public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
            => new ObjectData(
                Array.Empty<byte>(),
                Array.Empty<Relocation>(),
                1,
                Array.Empty<ISymbolDefinitionNode>());

        public override int CompareToImpl(ISortableNode other, CompilerComparer comparer)
        {
            var wtm = (WasmTypeNode)other;
            ReadOnlySpan<CorInfoWasmType> lhs = _types, rhs = wtm._types;
            return MemoryExtensions.SequenceCompareTo(lhs, rhs);
        }
    }
}


Review comment (suggestion):
There is a preferred sort order for these nodes, provided the object writer follows it (which it 'should'). Everything else being equal, we should prefer shorter signatures to be first, since they're more likely to be used. Pure lexicographic sort doesn't quite do that.

(So this could use an additional `lhs.Length < rhs.Length` or similar I suppose)

Code after:
        private readonly CorInfoWasmType[] _types;

        public WasmTypeNode(CorInfoWasmType[] types)
        {
            _types = types;
        }

        public override bool IsShareable => true;

        public override int ClassCode => -45678931;

        public override bool StaticDependenciesAreComputed => true;

        public override ObjectNodeSection GetSection(NodeFactory factory) => ObjectNodeSection.WasmTypeSection;

        protected override string GetName(NodeFactory factory)
            => $"Wasm Type ({string.Join(",", _types.Skip(1))}) -> {_types.FirstOrDefault()}";

        public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
            => new ObjectData(
                Array.Empty<byte>(),
                Array.Empty<Relocation>(),
                1,
                Array.Empty<ISymbolDefinitionNode>());

        public override int CompareToImpl(ISortableNode other, CompilerComparer comparer)
        {
            var wtm = (WasmTypeNode)other;
            ReadOnlySpan<CorInfoWasmType> lhs = _types, rhs = wtm._types;
            // Put shorter signatures earlier in the sort order, on the assumption that they are more likely to be used.
            int result = lhs.Length.CompareTo(rhs.Length);
            if (result == 0)
                result = MemoryExtensions.SequenceCompareTo(lhs, rhs);
            return result;
        }
    }
}
Repository: dotnet/runtime
File: src/coreclr/vm/jitinterface.cpp (C++)

Code before:
    {
        pLookup->accessType = IAT_PVALUE;
        pLookup->addr = (LPVOID)&(pNMD->m_pPInvokeTarget);
    }

    EE_TO_JIT_TRANSITION();
}

CORINFO_WASM_TYPE_SYMBOL_HANDLE CEEInfo::getWasmTypeSymbol(
    CorInfoWasmType* types, size_t typesSize)
{
    CONTRACTL {
        THROWS;
        GC_TRIGGERS;
        MODE_PREEMPTIVE;
    } CONTRACTL_END;

    CORINFO_WASM_TYPE_SYMBOL_HANDLE result = NULL;

    JIT_TO_EE_TRANSITION();

    UNREACHABLE(); // To be implemented

    EE_TO_JIT_TRANSITION();

    return result;
}

CORINFO_METHOD_HANDLE CEEInfo::getSpecialCopyHelper(CORINFO_CLASS_HANDLE type)
{
    CONTRACTL {
        THROWS;
        GC_TRIGGERS;
        MODE_PREEMPTIVE;
    } CONTRACTL_END;

    CORINFO_METHOD_HANDLE result = NULL;

    JIT_TO_EE_TRANSITION();

#ifdef FEATURE_IJW
    TypeHandle th = TypeHandle(type);

    _ASSERTE(th.IsValueType());

    MethodDesc* pHelperMD = CoreLibBinder::GetMethod(METHOD__RUNTIME_HELPERS__COPY_CONSTRUCT);

    pHelperMD = MethodDesc::FindOrCreateAssociatedMethodDesc(
        pHelperMD,
        pHelperMD->GetMethodTable(),
        FALSE,

Review comment (suggestion):
```suggestion
    LIMITED_METHOD_CONTRACT;
    UNREACHABLE_RET();
```
Nit: This does not need JIT_TO_EE_TRANSITION macros (similar to other UNREACHABLE methods)

Code after:
    {
        pLookup->accessType = IAT_PVALUE;
        pLookup->addr = (LPVOID)&(pNMD->m_pPInvokeTarget);
    }

    EE_TO_JIT_TRANSITION();
}

CORINFO_WASM_TYPE_SYMBOL_HANDLE CEEInfo::getWasmTypeSymbol(
    CorInfoWasmType* types, size_t typesSize)
{
    LIMITED_METHOD_CONTRACT;
    UNREACHABLE_RET();
}

CORINFO_METHOD_HANDLE CEEInfo::getSpecialCopyHelper(CORINFO_CLASS_HANDLE type)
{
    CONTRACTL {
        THROWS;
        GC_TRIGGERS;
        MODE_PREEMPTIVE;
    } CONTRACTL_END;

    CORINFO_METHOD_HANDLE result = NULL;

    JIT_TO_EE_TRANSITION();

#ifdef FEATURE_IJW
    TypeHandle th = TypeHandle(type);

    _ASSERTE(th.IsValueType());

    MethodDesc* pHelperMD = CoreLibBinder::GetMethod(METHOD__RUNTIME_HELPERS__COPY_CONSTRUCT);

    pHelperMD = MethodDesc::FindOrCreateAssociatedMethodDesc(
        pHelperMD,
        pHelperMD->GetMethodTable(),
        FALSE,
        Instantiation(&th, 1),
        FALSE,
        FALSE);

    pHelperMD->CheckRestore();
    result = (CORINFO_METHOD_HANDLE)pHelperMD;
#else
    _ASSERTE(!"Requires IJW feature");
#endif // FEATURE_IJW

    EE_TO_JIT_TRANSITION();

    return result;
Repository: dotnet/runtime
File: src/coreclr/pal/src/thread/thread.cpp (C++)

Code before:

    retValue = (*pfnStartRoutine)(pvPar);

    TRACE("Thread exited (%u)\n", retValue);
    pThread->SetExitCode(retValue);

    return NULL;

fail:

    //
    // Notify InternalCreateThread that a failure occurred
    //

    if (NULL != pThread)
    {
        pThread->synchronizationInfo.SetThreadState(TS_FAILED);
        pThread->SetStartStatus(FALSE);
    }

    /* do not call ExitThread : we don't want to call DllMain(), and the thread
       isn't in a clean state (e.g. lpThread isn't in TLS). the cleanup work
       above should release all resources */
    return NULL;
#endif // FEATURE_SINGLE_THREADED
}

/*++
Function:
    CreateThreadData

Abstract:
    Create the CPalThread for the startup thread
    or another external thread entering the PAL
    for the first time

Parameters:
    ppThread - on success, receives the CPalThread

Return:
   PAL_ERROR
--*/

PAL_ERROR
CorUnix::CreateThreadData(
    CPalThread **ppThread
    )
{
    PAL_ERROR palError = NO_ERROR;
    CPalThread *pThread = NULL;


Review comment (suggestion):
```suggestion
}
#endif // FEATURE_SINGLE_THREADED
```

Code after:
    TRACE("Thread exited (%u)\n", retValue);
    pThread->SetExitCode(retValue);

    return NULL;

fail:

    //
    // Notify InternalCreateThread that a failure occurred
    //

    if (NULL != pThread)
    {
        pThread->synchronizationInfo.SetThreadState(TS_FAILED);
        pThread->SetStartStatus(FALSE);
    }

    /* do not call ExitThread : we don't want to call DllMain(), and the thread
       isn't in a clean state (e.g. lpThread isn't in TLS). the cleanup work
       above should release all resources */
    return NULL;
}
#endif // !FEATURE_SINGLE_THREADED

/*++
Function:
    CreateThreadData

Abstract:
    Create the CPalThread for the startup thread
    or another external thread entering the PAL
    for the first time

Parameters:
    ppThread - on success, receives the CPalThread

Return:
   PAL_ERROR
--*/

PAL_ERROR
CorUnix::CreateThreadData(
    CPalThread **ppThread
    )
{
    PAL_ERROR palError = NO_ERROR;
    CPalThread *pThread = NULL;

    /* Create the thread object */
    pThread = AllocTHREAD();
Repository: dotnet/runtime
File: src/mono/wasm/Wasm.Build.Tests/Blazor/EventPipeDiagnosticsTests.cs (C#)

Code before:
            // concatenate the buffers into a single Uint8Array
            const concatenated = new Uint8Array(traces.reduce((acc, curr) => acc + curr.byteLength, 0));
            let offset = 0;
            for (const trace of traces) {
                concatenated.set(new Uint8Array(trace), offset);
                offset += trace.byteLength;
            }

            console.log(`File ${filename} size to upload: ${concatenated.byteLength} bytes ${new Date().toISOString()}`);
            await fetch(`/upload/${filename}`, {
                headers: {
                    'Content-Type': 'application/octet-stream'
                },
                method: 'POST',
                body: concatenated
            });
            console.log(`File uploaded successfully`);
        };
        console.log(`globalThis.uploadTrace method created ${new Date().toISOString()}`);
        """;
        await page.EvaluateAsync(up);
    }

    private async Task ClickAndCollect(IPage page)
    {
        await page.EvaluateAsync(@"globalThis.upload;");

        // Click the button a few times
        for (int i = 0; i < 5; i++)
        {
            await page.Locator("text=\"Click me\"").ClickAsync();
            await Task.Delay(10);
        }
        await Task.Delay(1000);

        var txt2 = await page.Locator("p[role='status']").InnerHTMLAsync();
        Assert.NotEqual("Current count: 0", txt2);
    }
}


Review comment (suggestion):
The evaluation of `globalThis.upload;` doesn't call the function. Since the earlier changes wrap the diagnostic collection in a function (`globalThis.upload = () => ...`), this line should be `globalThis.upload();` to actually invoke the function and start the diagnostic collection before clicking the button.
```suggestion
        await page.EvaluateAsync(@"globalThis.upload();");
```

Code after:
        await page.EvaluateAsync(up);
    }

    private async Task ClickAndCollect(IPage page)
    {
        // Use void to prevent Playwright from awaiting the returned Promise,
        // so tracing runs in parallel with button clicks below.
        await page.EvaluateAsync(@"void (globalThis.donePromise = globalThis.collectAndUpload())");
        _testOutput.WriteLine($"Installed script: {DateTime.Now.ToString("O")}");

        // Click the button a few times while tracing is running
        for (int i = 0; i < 5; i++)
        {
            await page.Locator("text=\"Click me\"").ClickAsync();
            await Task.Delay(10);
        }
        _testOutput.WriteLine($"Done clicking: {DateTime.Now.ToString("O")}");

        var txt2 = await page.Locator("p[role='status']").InnerHTMLAsync();
        Assert.NotEqual("Current count: 0", txt2);

        // Wait for trace collection and upload to complete
        await page.EvaluateAsync(@"globalThis.donePromise");
    }
}
Repository: dotnet/runtime
File: src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs (C#)

Code before:

            for (digitEnd = poundPos + 1; digitEnd < helpFile.Length; digitEnd++)
            {
                if (char.IsWhiteSpace(helpFile[digitEnd]))
                    break;
            }

            if (uint.TryParse(helpFile.AsSpan(poundPos + 1, digitEnd - poundPos - 1), out helpContext))
            {
                helpFile = helpFile.Substring(0, poundPos);
            }

            return helpFile;
        }

        // used by vm
        [UnmanagedCallersOnly]
        internal static unsafe void GetHelpContextBstr(Exception* obj, IntPtr* bstr, uint* helpContext, Exception* pException)
        {
            try
            {
                string? helpFile = obj->GetHelpContext(out *helpContext);

                *bstr = helpFile != null
                    ? Interop.OleAut32.SysAllocStringLen(helpFile, (uint)helpFile.Length)
                    : IntPtr.Zero;
            }
            catch (Exception ex)
            {
                *pException = ex;
            }
        }
#endif
    }
}


Review comment (suggestion):
```suggestion
                *bstr = Marshal.StringToBSTR(helpFile);
```

Code after:
            {
                if (char.IsWhiteSpace(helpFile[digitEnd]))
                    break;
            }

            if (uint.TryParse(helpFile.AsSpan(poundPos + 1, digitEnd - poundPos - 1), out helpContext))
            {
                helpFile = helpFile.Substring(0, poundPos);
            }

            return helpFile;
        }

        // used by vm
        [UnmanagedCallersOnly]
        internal static unsafe void GetHelpContextBstr(Exception* obj, IntPtr* bstr, uint* helpContext, Exception* pException)
        {
            *bstr = IntPtr.Zero;
            try
            {
                string? helpFile = obj->GetHelpContext(out *helpContext);

                *bstr = Marshal.StringToBSTR(helpFile);
            }
            catch (Exception ex)
            {
                *pException = ex;
            }
        }
#endif
    }
}
Repository: dotnet/runtime
File: src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs (C#)

Code before:

            int poundPos, digitEnd;

            if (helpFile is null || (poundPos = helpFile.LastIndexOf('#')) == -1)
            {
                return helpFile;
            }

            for (digitEnd = poundPos + 1; digitEnd < helpFile.Length; digitEnd++)
            {
                if (char.IsWhiteSpace(helpFile[digitEnd]))
                    break;
            }

            if (uint.TryParse(helpFile.AsSpan(poundPos + 1, digitEnd - poundPos - 1), out helpContext))
            {
                helpFile = helpFile.Substring(0, poundPos);
            }

            return helpFile;
        }

        // used by vm
        [UnmanagedCallersOnly]
        internal static unsafe void GetHelpContextBstr(Exception* obj, IntPtr* bstr, uint* helpContext, Exception* pException)
        {
            try
            {
                string? helpFile = obj->GetHelpContext(out *helpContext);

                *bstr = Marshal.StringToBSTR(helpFile);
            }
            catch (Exception ex)
            {
                *pException = ex;
            }
        }
#endif
    }
}


Review comment (suggestion):
```suggestion
        {
            *bstr = IntPtr.Zero;
```

Code after:

            int poundPos, digitEnd;

            if (helpFile is null || (poundPos = helpFile.LastIndexOf('#')) == -1)
            {
                return helpFile;
            }

            for (digitEnd = poundPos + 1; digitEnd < helpFile.Length; digitEnd++)
            {
                if (char.IsWhiteSpace(helpFile[digitEnd]))
                    break;
            }

            if (uint.TryParse(helpFile.AsSpan(poundPos + 1, digitEnd - poundPos - 1), out helpContext))
            {
                helpFile = helpFile.Substring(0, poundPos);
            }

            return helpFile;
        }

        // used by vm
        [UnmanagedCallersOnly]
        internal static unsafe void GetHelpContextBstr(Exception* obj, IntPtr* bstr, uint* helpContext, Exception* pException)
        {
            *bstr = IntPtr.Zero;
            try
            {
                string? helpFile = obj->GetHelpContext(out *helpContext);

                *bstr = Marshal.StringToBSTR(helpFile);
            }
            catch (Exception ex)
            {
                *pException = ex;
            }
        }
#endif
    }
}
Repository: dotnet/runtime
File: src/coreclr/jit/codegenwasm.cpp (C++)

Code before:
        case GT_NOT:
            genCodeForNegNot(treeNode->AsOp());
            break;

        case GT_IND:
            genCodeForIndir(treeNode->AsIndir());
            break;

        case GT_STOREIND:
            genCodeForStoreInd(treeNode->AsStoreInd());
            break;

        case GT_CALL:
            genCall(treeNode->AsCall());
            break;

        case GT_NULLCHECK:
            genCodeForNullCheck(treeNode->AsIndir());
            break;

        case GT_BOUNDS_CHECK:
            genRangeCheck(treeNode);
            break;

        case GT_KEEPALIVE:
            // TODO-WASM-CQ: avoid generating anything here?
            genConsumeRegs(treeNode->AsOp()->gtOp1);
            GetEmitter()->emitIns(INS_drop);
            break;

        default:
#ifdef DEBUG
            NYIRAW(GenTree::OpName(treeNode->OperGet()));
#else
            NYI_WASM("Opcode not implemented");
#endif
            break;
    }
}

//------------------------------------------------------------------------
// genCodeForJTrue: emit Wasm br_if
//
// Arguments:
//    treeNode - predicate value
//
void CodeGen::genCodeForJTrue(GenTreeOp* jtrue)
{
    BasicBlock* const block = m_compiler->compCurBB;
    assert(block->KindIs(BBJ_COND));


Review comment (suggestion):
```suggestion
            // TODO-WASM-RA: remove KEEPALIVE after we've produced the GC info.
```

Code after:
        case GT_NOT:
            genCodeForNegNot(treeNode->AsOp());
            break;

        case GT_IND:
            genCodeForIndir(treeNode->AsIndir());
            break;

        case GT_STOREIND:
            genCodeForStoreInd(treeNode->AsStoreInd());
            break;

        case GT_CALL:
            genCall(treeNode->AsCall());
            break;

        case GT_NULLCHECK:
            genCodeForNullCheck(treeNode->AsIndir());
            break;

        case GT_BOUNDS_CHECK:
            genRangeCheck(treeNode);
            break;

        case GT_KEEPALIVE:
            // TODO-WASM-RA: remove KEEPALIVE after we've produced the GC info.
            genConsumeRegs(treeNode->AsOp()->gtOp1);
            GetEmitter()->emitIns(INS_drop);
            break;

        default:
#ifdef DEBUG
            NYIRAW(GenTree::OpName(treeNode->OperGet()));
#else
            NYI_WASM("Opcode not implemented");
#endif
            break;
    }
}

//------------------------------------------------------------------------
// genCodeForJTrue: emit Wasm br_if
//
// Arguments:
//    treeNode - predicate value
//
void CodeGen::genCodeForJTrue(GenTreeOp* jtrue)
{
    BasicBlock* const block = m_compiler->compCurBB;
    assert(block->KindIs(BBJ_COND));
Repository: dotnet/runtime
File: src/coreclr/vm/debugdebugger.cpp (C++)

Code before:
    // Allocate memory for the initial 'n' frames
    pData->pElements = new DebugStackTrace::Element[pData->cElementsAllocated];
    GetThread()->StackWalkFrames(GetStackFramesCallback, pData, FUNCTIONSONLY | QUICKUNWIND, NULL);

    // Do a 2nd pass outside of any locks.
    // This will compute IL offsets.
    for (INT32 i = 0; i < pData->cElements; i++)
    {
        pData->pElements[i].InitPass2();
    }
}

extern "C" void QCALLTYPE AsyncHelpers_AddContinuationToExInternal(
    void* diagnosticIP,
    QCall::ObjectHandleOnStack exception)
{
    QCALL_CONTRACT;

    BEGIN_QCALL;

    GCX_COOP();

    OBJECTREF pException = (OBJECTREF)exception.Get();
    _ASSERTE(pException != NULL);

    GCPROTECT_BEGIN(pException);

    // populate exception with information from the continuation object
    EECodeInfo codeInfo((PCODE)diagnosticIP);
    _ASSERTE(codeInfo.IsValid());
    MethodDesc* methodDesc = codeInfo.GetMethodDesc();
    StackTraceInfo::AppendElement(
        pException,
        (UINT_PTR)diagnosticIP,
        0,
        methodDesc,
        NULL);

    GCPROTECT_END();

    END_QCALL;
}

extern "C" void QCALLTYPE StackTrace_GetStackFramesInternal(
    QCall::ObjectHandleOnStack stackFrameHelper,
    BOOL fNeedFileInfo,
    QCall::ObjectHandleOnStack exception)
{
    QCALL_CONTRACT;

    BEGIN_QCALL;

Review comment (suggestion):
None of this code that follows between here and the argument being passed to `StackTraceInfo::AppendElement` triggers GC (the code has NOTRIGGERS contract https://github.com/dotnet/runtime/blob/f6b4b6ce93a8dce234a93e461060c648865dae3f/src/coreclr/vm/jitinterface.cpp#L15134-L15135 . This GCPROTECT is unnecessary - it is not the right fix.

Code after:
    // Allocate memory for the initial 'n' frames
    pData->pElements = new DebugStackTrace::Element[pData->cElementsAllocated];
    GetThread()->StackWalkFrames(GetStackFramesCallback, pData, FUNCTIONSONLY | QUICKUNWIND, NULL);

    // Do a 2nd pass outside of any locks.
    // This will compute IL offsets.
    for (INT32 i = 0; i < pData->cElements; i++)
    {
        pData->pElements[i].InitPass2();
    }
}

extern "C" void QCALLTYPE AsyncHelpers_AddContinuationToExInternal(
    void* diagnosticIP,
    QCall::ObjectHandleOnStack exception)
{
    QCALL_CONTRACT;

    BEGIN_QCALL;

    GCX_COOP();

    OBJECTREF pException = (OBJECTREF)exception.Get();
    _ASSERTE(pException != NULL);

    // populate exception with information from the continuation object
    EECodeInfo codeInfo((PCODE)diagnosticIP);
    _ASSERTE(codeInfo.IsValid());
    MethodDesc* methodDesc = codeInfo.GetMethodDesc();
    StackTraceInfo::AppendElement(
        pException,
        (UINT_PTR)diagnosticIP,
        0,
        methodDesc,
        NULL);

    END_QCALL;
}

extern "C" void QCALLTYPE StackTrace_GetStackFramesInternal(
    QCall::ObjectHandleOnStack stackFrameHelper,
    BOOL fNeedFileInfo,
    QCall::ObjectHandleOnStack exception)
{
    QCALL_CONTRACT;

    BEGIN_QCALL;

    GCX_COOP();

    struct