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

Code before:
    m_thinLock.StoreWithoutBarrier((threadId & SBLK_MASK_LOCK_THREADID) | (recursionLevel << SBLK_RECLEVEL_SHIFT));
}

OBJECTHANDLE SyncBlock::GetOrCreateLock(OBJECTREF lockObj)
{
    CONTRACTL
    {
        GC_TRIGGERS;
        THROWS;
        MODE_COOPERATIVE;
    }
    CONTRACTL_END;

    OBJECTHANDLE existingLock = VolatileLoad(&m_Lock);
    if (existingLock != (OBJECTHANDLE)NULL)
    {
        return existingLock;
    }

    SetPrecious();

    // We'll likely need to put this lock object into the sync block.
    // Create the handle here.
    OBJECTHANDLE lockHandle = GetAppDomain()->CreateHandle(lockObj);

    if (TryUpgradeThinLockToFullLock(lockHandle))
    {
        // Our lock instance is the one in the sync block now.
        return lockHandle;
    }

    // Another thread beat us to the upgrade.
    // Delete our GC handle.
    DestroyHandle(lockHandle);

    return VolatileLoad(&m_Lock);
}

bool SyncBlock::TryUpgradeThinLockToFullLock(OBJECTHANDLE lockHandle)
{
    CONTRACTL
    {
        GC_TRIGGERS;
        THROWS;
        MODE_COOPERATIVE;
    }
    CONTRACTL_END;

    // Switch to preemptive so we can grab the spin-lock.
    // Use the NO_DTOR version so we don't do a coop->preemptive->coop transition on return.
    GCX_PREEMP_NO_DTOR();

Review comment (suggestion):
We can skip the holder like this only if `TryUpgradeThinLockToFullLock` does not throw, but its contract says that it throws. Something does not add up here.

Code after:
    m_thinLock.StoreWithoutBarrier((threadId & SBLK_MASK_LOCK_THREADID) | (recursionLevel << SBLK_RECLEVEL_SHIFT));
}

OBJECTHANDLE SyncBlock::GetOrCreateLock(OBJECTREF lockObj)
{
    CONTRACTL
    {
        GC_TRIGGERS;
        THROWS;
        MODE_COOPERATIVE;
    }
    CONTRACTL_END;

    OBJECTHANDLE existingLock = VolatileLoad(&m_Lock);
    if (existingLock != (OBJECTHANDLE)NULL)
    {
        return existingLock;
    }

    SetPrecious();

    // We'll likely need to put this lock object into the sync block.
    // Create the handle here.
    OBJECTHANDLEHolder lockHandle = GetAppDomain()->CreateHandle(lockObj);

    if (TryUpgradeThinLockToFullLock(lockHandle.GetValue()))
    {
        // Our lock instance is the one in the sync block now.
        return lockHandle.Extract();
    }

    return VolatileLoad(&m_Lock);
}

bool SyncBlock::TryUpgradeThinLockToFullLock(OBJECTHANDLE lockHandle)
{
    CONTRACTL
    {
        GC_TRIGGERS;
        THROWS;
        MODE_COOPERATIVE;
    }
    CONTRACTL_END;

    // Switch to preemptive so we can grab the spin-lock.
    // Use the NO_DTOR version so we don't do a coop->preemptive->coop transition on return.
    GCX_PREEMP_NO_DTOR();

    HeaderSpinLockHolder lock(std::addressof(m_thinLock));

    // We don't need to be in preemptive any more here.
Repository: dotnet/runtime
File: src/mono/sample/wasm/browser-bench/wwwroot/main.js (JavaScript)

Code before:
            throw err;
        }
    }

    removeFrame() {
        if (this._frame.contentWindow.muteErrors !== undefined)
            this._frame.contentWindow.muteErrors();

        document.body.removeChild(this._frame);
    }
}

try {
    globalThis.mainApp = new MainApp();
    globalThis.mainApp.FrameBlazorFirstUI = globalThis.mainApp.frameBlazorFirstUI.bind(globalThis.mainApp);
    globalThis.mainApp.FrameReachedManaged = globalThis.mainApp.frameReachedManaged.bind(globalThis.mainApp);
    globalThis.mainApp.PageShow = globalThis.mainApp.pageShow.bind(globalThis.mainApp);
    globalThis.mainApp.Origin = globalThis.mainApp.origin.bind(globalThis.mainApp);
    globalThis.mainApp.SetFramePage = globalThis.mainApp.setFramePage.bind(globalThis.mainApp);

    const runtime = await dotnet
        // We enable jiterpreter stats so that in local runs you can open the devtools
        //  console to see statistics on how much code it generated and whether any new opcodes
        //  are causing traces to fail to compile
        .withRuntimeOptions(["--jiterpreter-stats-enabled"])
        .withConfig({ exitOnUnhandledError: true, logExitCode: true })
        .create();

    await mainApp.init(runtime);
}
catch (err) {
    exit(1, err);
}


Review comment (suggestion):
The old code included `.withElementOnExit()` which set `appendElementOnExit: true`, but this configuration flag is missing from the new config object. To maintain the same behavior, `appendElementOnExit: true` should be added to the config.
```suggestion
        .withConfig({ exitOnUnhandledError: true, logExitCode: true, appendElementOnExit: true })
```

Code after:
            throw err;
        }
    }

    removeFrame() {
        if (this._frame.contentWindow.muteErrors !== undefined)
            this._frame.contentWindow.muteErrors();

        document.body.removeChild(this._frame);
    }
}

try {
    globalThis.mainApp = new MainApp();
    globalThis.mainApp.FrameBlazorFirstUI = globalThis.mainApp.frameBlazorFirstUI.bind(globalThis.mainApp);
    globalThis.mainApp.FrameReachedManaged = globalThis.mainApp.frameReachedManaged.bind(globalThis.mainApp);
    globalThis.mainApp.PageShow = globalThis.mainApp.pageShow.bind(globalThis.mainApp);
    globalThis.mainApp.Origin = globalThis.mainApp.origin.bind(globalThis.mainApp);
    globalThis.mainApp.SetFramePage = globalThis.mainApp.setFramePage.bind(globalThis.mainApp);

    const runtime = await dotnet
        // We enable jiterpreter stats so that in local runs you can open the devtools
        //  console to see statistics on how much code it generated and whether any new opcodes
        //  are causing traces to fail to compile
        .withRuntimeOptions(["--jiterpreter-stats-enabled"])
        .withConfig({ exitOnUnhandledError: true, logExitCode: true, appendElementOnExit: true })
        .create();

    await mainApp.init(runtime);
}
catch (err) {
    exit(1, err);
}
Repository: dotnet/runtime
File: src/coreclr/vm/gcinfodecoder.cpp (C++)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#ifndef SOS_INCLUDE
#include "common.h"
#endif

#include "gcinfodecoder.h"
#include "interpexec.h"

#ifdef USE_GC_INFO_DECODER

#ifndef CHECK_APP_DOMAIN
#define CHECK_APP_DOMAIN    0
#endif

#ifndef GCINFODECODER_CONTRACT
#define GCINFODECODER_CONTRACT LIMITED_METHOD_CONTRACT
#endif // !GCINFODECODER_CONTRACT


#ifndef GET_CALLER_SP
inline size_t GET_CALLER_SP(PREGDISPLAY pREGDISPLAY)
{
    _ASSERTE(false);
    return 0;
}
#endif // !GET_CALLER_SP

#ifndef VALIDATE_OBJECTREF
#if defined(DACCESS_COMPILE)
#define VALIDATE_OBJECTREF(objref, fDeep)
#else // DACCESS_COMPILE
#define VALIDATE_OBJECTREF(objref, fDeep)                          \
    do {                                                           \

Review comment (suggestion):
The include directive for "interpexec.h" should be conditionally compiled with #ifdef FEATURE_INTERPRETER, similar to how the code that uses InterpMethodContextFrame is guarded. This prevents unnecessary includes when the interpreter feature is not enabled.
```suggestion
#include "gcinfodecoder.h"
#ifdef FEATURE_INTERPRETER
#include "interpexec.h"
#endif // FEATURE_INTERPRETER
```

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#ifndef SOS_INCLUDE
#include "common.h"
#endif

#include "gcinfodecoder.h"
#ifdef FEATURE_INTERPRETER
#include "interpexec.h"
#endif // FEATURE_INTERPRETER

#ifdef USE_GC_INFO_DECODER

#ifndef CHECK_APP_DOMAIN
#define CHECK_APP_DOMAIN    0
#endif

#ifndef GCINFODECODER_CONTRACT
#define GCINFODECODER_CONTRACT LIMITED_METHOD_CONTRACT
#endif // !GCINFODECODER_CONTRACT


#ifndef GET_CALLER_SP
inline size_t GET_CALLER_SP(PREGDISPLAY pREGDISPLAY)
{
    _ASSERTE(false);
    return 0;
}
#endif // !GET_CALLER_SP

#ifndef VALIDATE_OBJECTREF
#if defined(DACCESS_COMPILE)
#define VALIDATE_OBJECTREF(objref, fDeep)
#else // DACCESS_COMPILE
Repository: dotnet/runtime
File: src/coreclr/nativeaot/Runtime/amd64/GcProbe.asm (Assembly)

Code before:

LEAF_ENTRY RhpGcPoll, _TEXT
        cmp         [RhpTrapThreads], TrapThreadsFlags_None
        jne         @F                  ; forward branch - predicted not taken
        ret
@@:
        jmp         RhpGcPollRare
LEAF_END RhpGcPoll, _TEXT

NESTED_ENTRY RhpGcPollRare, _TEXT
        PUSH_COOP_PINVOKE_FRAME rcx
        call        RhpGcPoll2
        POP_COOP_PINVOKE_FRAME
        ret
NESTED_END RhpGcPollRare, _TEXT



ifdef FEATURE_GC_STRESS

;;
;; GC Stress Hijack targets
;;
LEAF_ENTRY RhpGcStressHijack, _TEXT
        FixupHijackedCallstack
        mov         r8d, DEFAULT_FRAME_SAVE_FLAGS + PTFF_SAVE_RAX + PTFF_SAVE_RCX
        jmp         RhpGcStressProbe
LEAF_END RhpGcStressHijack, _TEXT

;;
;; Worker for our GC stress probes.  Do not call directly!!
;; Instead, go through RhpGcStressHijack{Scalar|Object|Byref}.
;; This worker performs the GC Stress work and returns to the original return address.
;;
;; Register state on entry:
;;  RDX: thread pointer
;;  RCX: register bitmask
;;
;; Register state on exit:
;;  Scratch registers, except for RAX/RCX, have been trashed
;;  All other registers restored as they were when the hijack was first reached.
;;
NESTED_ENTRY RhpGcStressProbe, _TEXT
        PUSH_PROBE_FRAME rdx, rax, r8
        END_PROLOGUE

        call        RhpStressGc

        POP_PROBE_FRAME
        ret
NESTED_END RhpGcStressProbe, _TEXT

Review comment (suggestion):
Changed this to `mov` because the original `or` assumed FixupHijackedCallstack zeroed the register out which is __subtle__.

Code after:

LEAF_ENTRY RhpGcPoll, _TEXT
        cmp         [RhpTrapThreads], TrapThreadsFlags_None
        jne         @F                  ; forward branch - predicted not taken
        ret
@@:
        jmp         RhpGcPollRare
LEAF_END RhpGcPoll, _TEXT

NESTED_ENTRY RhpGcPollRare, _TEXT
        PUSH_COOP_PINVOKE_FRAME rcx
        call        RhpGcPoll2
        POP_COOP_PINVOKE_FRAME
        ret
NESTED_END RhpGcPollRare, _TEXT



ifdef FEATURE_GC_STRESS

;;
;; GC Stress Hijack targets
;;
LEAF_ENTRY RhpGcStressHijack, _TEXT
        FixupHijackedCallstack
        mov         r8d, DEFAULT_FRAME_SAVE_FLAGS + PTFF_SAVE_RAX + PTFF_SAVE_RCX
        jmp         RhpGcStressProbe
LEAF_END RhpGcStressHijack, _TEXT

;;
;; Worker for our GC stress probes.  Do not call directly!!
;; Instead, go through RhpGcStressHijack{Scalar|Object|Byref}.
;; This worker performs the GC Stress work and returns to the original return address.
;;
;; Register state on entry:
;;  RDX: thread pointer
;;  R8: register bitmask
;;
;; Register state on exit:
;;  Scratch registers, except for RAX/RCX, have been trashed
;;  All other registers restored as they were when the hijack was first reached.
;;
NESTED_ENTRY RhpGcStressProbe, _TEXT
        PUSH_PROBE_FRAME rdx, rax, r8
        END_PROLOGUE

        call        RhpStressGc

        POP_PROBE_FRAME
        ret
NESTED_END RhpGcStressProbe, _TEXT
Repository: dotnet/runtime
File: src/native/libs/System.Native/pal_mount.c (C)

Code before:
#define STRING_BUFFER_SIZE 8192

// Android does not define MNTOPT_RO
#ifndef MNTOPT_RO
#define MNTOPT_RO "r"
#endif
#endif

int32_t SystemNative_GetAllMountPoints(MountPointFound onFound, void* context)
{
#if HAVE_MNTINFO
    // Use getfsstat which is thread-safe (unlike getmntinfo which uses internal static buffers)
#if HAVE_STATFS
    struct statfs* mounts = NULL;
#else
    struct statvfs* mounts = NULL;
#endif
    // First call to get the number of mount points
    int count = getfsstat(NULL, 0, MNT_NOWAIT);
    if (count <= 0)
    {
        return count < 0 ? -1 : 0;
    }

    // Allocate buffer for mount points (with extra capacity in case new mounts appear)
    size_t bufferSize = (size_t)(count + 4) * sizeof(*mounts);
    mounts = malloc(bufferSize);
    if (mounts == NULL)
    {
        errno = ENOMEM;
        return -1;
    }

    // Second call to get actual mount point information
    count = getfsstat(mounts, (int)bufferSize, MNT_NOWAIT);
    if (count < 0)
    {
        free(mounts);
        return -1;
    }

    for (int32_t i = 0; i < count; i++)
    {
        onFound(context, mounts[i].f_mntonname);
    }

    free(mounts);
    return 0;
}

#elif HAVE_SYS_MNTENT_H

Review comment (bug):
The expression (count + 4) could theoretically overflow if count is close to INT_MAX, before it's cast to size_t. While getfsstat will never return such a large value in practice, for defensive programming consider casting count to size_t before the addition: size_t bufferSize = ((size_t)count + 4) * sizeof(*mounts);
```suggestion
    size_t bufferSize = ((size_t)count + 4) * sizeof(*mounts);
```

Code after:
#endif
#define STRING_BUFFER_SIZE 8192

// Android does not define MNTOPT_RO
#ifndef MNTOPT_RO
#define MNTOPT_RO "r"
#endif
#endif

int32_t SystemNative_GetAllMountPoints(MountPointFound onFound, void* context)
{
#if HAVE_MNTINFO
    // Use getfsstat which is thread-safe (unlike getmntinfo which uses internal static buffers)
#if HAVE_STATFS
    struct statfs* mounts = NULL;
#else
    struct statvfs* mounts = NULL;
#endif

    int count;
    int capacity = 0;
    size_t bufferSize = 0;

    // Loop to handle the case where mount points are added between calls
    while (1)
    {
        // Get the current number of mount points
        count = getfsstat(NULL, 0, MNT_NOWAIT);
        if (count < 0)
        {
            free(mounts);
            return -1;
        }

        // Reallocate buffer if needed - allocate one extra to detect if more mounts were added
        if (count >= capacity)
        {
            free(mounts);
            capacity = count + 1;
            if (!multiply_s((size_t)capacity, sizeof(*mounts), &bufferSize))
            {
                errno = ENOMEM;
                return -1;
            }
            if (bufferSize > INT_MAX)
            {
                errno = ENOMEM;
                return -1;
            }
#if HAVE_STATFS
            mounts = (struct statfs*)malloc(bufferSize);
Repository: dotnet/runtime
File: src/native/libs/System.Native/pal_mount.c (C)

Code before:
#if HAVE_MNTINFO
    // Use getfsstat which is thread-safe (unlike getmntinfo which uses internal static buffers)
#if HAVE_STATFS
    struct statfs* mounts = NULL;
#else
    struct statvfs* mounts = NULL;
#endif

    int count;
    int capacity = 0;

    // Loop to handle the case where mount points are added between calls
    while (1)
    {
        // Get the current number of mount points
        count = getfsstat(NULL, 0, MNT_NOWAIT);
        if (count < 0)
        {
            free(mounts);
            return -1;
        }
        if (count == 0)
        {
            free(mounts);
            return 0;
        }

        // Reallocate buffer if needed
        if (count > capacity)
        {
            free(mounts);
            capacity = count;
            mounts = malloc((size_t)capacity * sizeof(*mounts));
            if (mounts == NULL)
            {
                errno = ENOMEM;
                return -1;
            }
        }

        // Get actual mount point information
        count = getfsstat(mounts, capacity * (int)sizeof(*mounts), MNT_NOWAIT);
        if (count < 0)
        {
            free(mounts);
            return -1;
        }

        // If count fits in capacity, we got all mount points
        if (count <= capacity)
        {

Review comment (suggestion):
For count  as 0 you can just break out of the loop and let the post loop code handle the empty case, which it already does

Code after:
{
#if HAVE_MNTINFO
    // Use getfsstat which is thread-safe (unlike getmntinfo which uses internal static buffers)
#if HAVE_STATFS
    struct statfs* mounts = NULL;
#else
    struct statvfs* mounts = NULL;
#endif

    int count;
    int capacity = 0;
    size_t bufferSize = 0;

    // Loop to handle the case where mount points are added between calls
    while (1)
    {
        // Get the current number of mount points
        count = getfsstat(NULL, 0, MNT_NOWAIT);
        if (count < 0)
        {
            free(mounts);
            return -1;
        }

        // Reallocate buffer if needed - allocate one extra to detect if more mounts were added
        if (count >= capacity)
        {
            free(mounts);
            capacity = count + 1;
            if (!multiply_s((size_t)capacity, sizeof(*mounts), &bufferSize))
            {
                errno = ENOMEM;
                return -1;
            }
            if (bufferSize > INT_MAX)
            {
                errno = ENOMEM;
                return -1;
            }
#if HAVE_STATFS
            mounts = (struct statfs*)malloc(bufferSize);
#else
            mounts = (struct statvfs*)malloc(bufferSize);
#endif
            if (mounts == NULL)
            {
                errno = ENOMEM;
                return -1;
            }
        }
Repository: dotnet/runtime
File: src/native/libs/System.Native/pal_mount.c (C)

Code before:
    struct statfs* mounts = NULL;
#else
    struct statvfs* mounts = NULL;
#endif

    int count;
    int capacity = 0;

    // Loop to handle the case where mount points are added between calls
    while (1)
    {
        // Get the current number of mount points
        count = getfsstat(NULL, 0, MNT_NOWAIT);
        if (count < 0)
        {
            free(mounts);
            return -1;
        }

        // Reallocate buffer if needed - allocate one extra to detect if more mounts were added
        if (count >= capacity)
        {
            free(mounts);
            capacity = count + 1;
#if HAVE_STATFS
            mounts = (struct statfs*)malloc((size_t)capacity * sizeof(*mounts));
#else
            mounts = (struct statvfs*)malloc((size_t)capacity * sizeof(*mounts));
#endif
            if (mounts == NULL)
            {
                errno = ENOMEM;
                return -1;
            }
        }

        // If count is 0, break - post-loop code handles empty case
        if (count == 0)
        {
            break;
        }

        // Get actual mount point information
        count = getfsstat(mounts, (size_t)capacity * sizeof(*mounts), MNT_NOWAIT);
        if (count < 0)
        {
            free(mounts);
            return -1;
        }

        // If count is less than capacity, we got all mount points

Review comment (bug):
@copilot Use `multiply_s` method for integer-overflow safe multiplication. Return error on integer overflow

Code after:
#if HAVE_STATFS
    struct statfs* mounts = NULL;
#else
    struct statvfs* mounts = NULL;
#endif

    int count;
    int capacity = 0;
    size_t bufferSize = 0;

    // Loop to handle the case where mount points are added between calls
    while (1)
    {
        // Get the current number of mount points
        count = getfsstat(NULL, 0, MNT_NOWAIT);
        if (count < 0)
        {
            free(mounts);
            return -1;
        }

        // Reallocate buffer if needed - allocate one extra to detect if more mounts were added
        if (count >= capacity)
        {
            free(mounts);
            capacity = count + 1;
            if (!multiply_s((size_t)capacity, sizeof(*mounts), &bufferSize))
            {
                errno = ENOMEM;
                return -1;
            }
            if (bufferSize > INT_MAX)
            {
                errno = ENOMEM;
                return -1;
            }
#if HAVE_STATFS
            mounts = (struct statfs*)malloc(bufferSize);
#else
            mounts = (struct statvfs*)malloc(bufferSize);
#endif
            if (mounts == NULL)
            {
                errno = ENOMEM;
                return -1;
            }
        }

        // If count is 0, break - post-loop code handles empty case
        if (count == 0)
        {
Repository: dotnet/runtime
File: src/native/libs/System.Native/pal_mount.c (C)

Code before:

        // Reallocate buffer if needed - allocate one extra to detect if more mounts were added
        if (count >= capacity)
        {
            free(mounts);
            capacity = count + 1;
#if HAVE_STATFS
            mounts = (struct statfs*)malloc((size_t)capacity * sizeof(*mounts));
#else
            mounts = (struct statvfs*)malloc((size_t)capacity * sizeof(*mounts));
#endif
            if (mounts == NULL)
            {
                errno = ENOMEM;
                return -1;
            }
        }

        // If count is 0, break - post-loop code handles empty case
        if (count == 0)
        {
            break;
        }

        // Get actual mount point information
        count = getfsstat(mounts, (size_t)capacity * sizeof(*mounts), MNT_NOWAIT);
        if (count < 0)
        {
            free(mounts);
            return -1;
        }

        // If count is less than capacity, we got all mount points
        if (count < capacity)
        {
            break;
        }
        // Otherwise, more mounts were added - loop again with larger buffer
    }

    for (int32_t i = 0; i < count; i++)
    {
        onFound(context, mounts[i].f_mntonname);
    }

    free(mounts);
    return 0;
}

#elif HAVE_SYS_MNTENT_H
    int result = -1;

Review comment (suggestion):
@copilot Cast the buffer size to int to fix build break. Return error if the buffer size does not fit into int.

Code after:
            return -1;
        }

        // Reallocate buffer if needed - allocate one extra to detect if more mounts were added
        if (count >= capacity)
        {
            free(mounts);
            capacity = count + 1;
            if (!multiply_s((size_t)capacity, sizeof(*mounts), &bufferSize))
            {
                errno = ENOMEM;
                return -1;
            }
            if (bufferSize > INT_MAX)
            {
                errno = ENOMEM;
                return -1;
            }
#if HAVE_STATFS
            mounts = (struct statfs*)malloc(bufferSize);
#else
            mounts = (struct statvfs*)malloc(bufferSize);
#endif
            if (mounts == NULL)
            {
                errno = ENOMEM;
                return -1;
            }
        }

        // If count is 0, break - post-loop code handles empty case
        if (count == 0)
        {
            break;
        }

        // Get actual mount point information
        count = getfsstat(mounts, (int)bufferSize, MNT_NOWAIT);
        if (count < 0)
        {
            free(mounts);
            return -1;
        }

        // If count is less than capacity, we got all mount points
        if (count < capacity)
        {
            break;
        }
        // Otherwise, more mounts were added - loop again with larger buffer
    }