Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISC-V] Set the HAVE_FUNCTIONAL_PTHREAD_ROBUST_MUTEXES_EXITCOD value to 0 #92324

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

JongHeonChoi
Copy link
Contributor

@JongHeonChoi JongHeonChoi commented Sep 20, 2023

The HAVE_FUNCTIONAL_PTHREAD_ROBUST_MUTEXES_EXITCODE value is also set to 0 in the VisionFive2 target of RISCV64.

cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @alpencolt @gbalykov @clamp03 @sirntar @yurai007

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 20, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 20, 2023
@@ -170,8 +170,6 @@ else()
message(FATAL_ERROR "Unsupported platform. OS: ${CMAKE_SYSTEM_NAME}, arch: ${TARGET_ARCH_NAME}")
endif()

if(TARGET_ARCH_NAME MATCHES "^(x86|x64|s390x|armv6|loongarch64|ppc64le)$")
if(TARGET_ARCH_NAME MATCHES "^(x86|x64|s390x|armv6|loongarch64|riscv64|ppc64le)$")
set_cache_value(HAVE_FUNCTIONAL_PTHREAD_ROBUST_MUTEXES_EXITCODE 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kouvel Do we still need HAVE_FUNCTIONAL_PTHREAD_ROBUST_MUTEXES? It does not look right that it is architecture specific.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was introduced due to a couple of issues, one in Alpine Linux and one in glibc on ARM/ARM64, but I believe that both have since been fixed. I'm not sure why the cache value is being set based on architecture, it should probably be checked based on the target OS, which is currently done in mutex.hpp in the PAL.

It was noted in #82380 (comment) that a PAL test for named mutexes fails when pthread robust mutexes are used on RISC-V, so I wonder if it actually works. @JongHeonChoi, have you checked if the corresponding test case in

if(NOT CLR_CMAKE_HOST_ARCH_ARM AND NOT CLR_CMAKE_HOST_ARCH_ARM64)
set(CMAKE_REQUIRED_LIBRARIES pthread)
check_cxx_source_runs("
// This test case verifies the pthread process-shared robust mutex's cross-process abandon detection. The parent process starts
// a child process that locks the mutex, the process process then waits to acquire the lock, and the child process abandons the
// mutex by exiting the process while holding the lock. The parent process should then be released from its wait, be assigned
// ownership of the lock, and be notified that the mutex was abandoned.
#include <sys/mman.h>
#include <sys/time.h>
#include <errno.h>
#include <pthread.h>
#include <stdio.h>
#include <unistd.h>
#include <new>
using namespace std;
struct Shm
{
pthread_mutex_t syncMutex;
pthread_cond_t syncCondition;
pthread_mutex_t robustMutex;
int conditionValue;
Shm() : conditionValue(0)
{
}
} *shm;
int GetFailTimeoutTime(struct timespec *timeoutTimeRef)
{
int getTimeResult = clock_gettime(CLOCK_REALTIME, timeoutTimeRef);
if (getTimeResult != 0)
{
struct timeval tv;
getTimeResult = gettimeofday(&tv, NULL);
if (getTimeResult != 0)
return 1;
timeoutTimeRef->tv_sec = tv.tv_sec;
timeoutTimeRef->tv_nsec = tv.tv_usec * 1000;
}
timeoutTimeRef->tv_sec += 30;
return 0;
}
int WaitForConditionValue(int desiredConditionValue)
{
struct timespec timeoutTime;
if (GetFailTimeoutTime(&timeoutTime) != 0)
return 1;
if (pthread_mutex_timedlock(&shm->syncMutex, &timeoutTime) != 0)
return 1;
if (shm->conditionValue != desiredConditionValue)
{
if (GetFailTimeoutTime(&timeoutTime) != 0)
return 1;
if (pthread_cond_timedwait(&shm->syncCondition, &shm->syncMutex, &timeoutTime) != 0)
return 1;
if (shm->conditionValue != desiredConditionValue)
return 1;
}
if (pthread_mutex_unlock(&shm->syncMutex) != 0)
return 1;
return 0;
}
int SetConditionValue(int newConditionValue)
{
struct timespec timeoutTime;
if (GetFailTimeoutTime(&timeoutTime) != 0)
return 1;
if (pthread_mutex_timedlock(&shm->syncMutex, &timeoutTime) != 0)
return 1;
shm->conditionValue = newConditionValue;
if (pthread_cond_signal(&shm->syncCondition) != 0)
return 1;
if (pthread_mutex_unlock(&shm->syncMutex) != 0)
return 1;
return 0;
}
void DoTest_Child();
int DoTest()
{
// Map some shared memory
void *shmBuffer = mmap(NULL, 4096, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED, -1, 0);
if (shmBuffer == MAP_FAILED)
return 1;
shm = new(shmBuffer) Shm;
// Create sync mutex
pthread_mutexattr_t syncMutexAttributes;
if (pthread_mutexattr_init(&syncMutexAttributes) != 0)
return 1;
if (pthread_mutexattr_setpshared(&syncMutexAttributes, PTHREAD_PROCESS_SHARED) != 0)
return 1;
if (pthread_mutex_init(&shm->syncMutex, &syncMutexAttributes) != 0)
return 1;
if (pthread_mutexattr_destroy(&syncMutexAttributes) != 0)
return 1;
// Create sync condition
pthread_condattr_t syncConditionAttributes;
if (pthread_condattr_init(&syncConditionAttributes) != 0)
return 1;
if (pthread_condattr_setpshared(&syncConditionAttributes, PTHREAD_PROCESS_SHARED) != 0)
return 1;
if (pthread_cond_init(&shm->syncCondition, &syncConditionAttributes) != 0)
return 1;
if (pthread_condattr_destroy(&syncConditionAttributes) != 0)
return 1;
// Create the robust mutex that will be tested
pthread_mutexattr_t robustMutexAttributes;
if (pthread_mutexattr_init(&robustMutexAttributes) != 0)
return 1;
if (pthread_mutexattr_setpshared(&robustMutexAttributes, PTHREAD_PROCESS_SHARED) != 0)
return 1;
if (pthread_mutexattr_setrobust(&robustMutexAttributes, PTHREAD_MUTEX_ROBUST) != 0)
return 1;
if (pthread_mutex_init(&shm->robustMutex, &robustMutexAttributes) != 0)
return 1;
if (pthread_mutexattr_destroy(&robustMutexAttributes) != 0)
return 1;
// Start child test process
int error = fork();
if (error == -1)
return 1;
if (error == 0)
{
DoTest_Child();
return -1;
}
// Wait for child to take a lock
WaitForConditionValue(1);
// Wait to try to take a lock. Meanwhile, child abandons the robust mutex.
struct timespec timeoutTime;
if (GetFailTimeoutTime(&timeoutTime) != 0)
return 1;
error = pthread_mutex_timedlock(&shm->robustMutex, &timeoutTime);
if (error != EOWNERDEAD) // expect to be notified that the robust mutex was abandoned
return 1;
if (pthread_mutex_consistent(&shm->robustMutex) != 0)
return 1;
if (pthread_mutex_unlock(&shm->robustMutex) != 0)
return 1;
if (pthread_mutex_destroy(&shm->robustMutex) != 0)
return 1;
return 0;
}
void DoTest_Child()
{
// Lock the robust mutex
struct timespec timeoutTime;
if (GetFailTimeoutTime(&timeoutTime) != 0)
return;
if (pthread_mutex_timedlock(&shm->robustMutex, &timeoutTime) != 0)
return;
// Notify parent that robust mutex is locked
if (SetConditionValue(1) != 0)
return;
// Wait a short period to let the parent block on waiting for a lock
sleep(1);
// Abandon the mutex by exiting the process while holding the lock. Parent's wait should be released by EOWNERDEAD.
}
int main()
{
int result = DoTest();
return result >= 0 ? result : 0;
}" HAVE_FUNCTIONAL_PTHREAD_ROBUST_MUTEXES)
set(CMAKE_REQUIRED_LIBRARIES)
endif()
compiles and runs successfully on RISC-V? Probably would also need to check the PAL named mutex tests to make sure that it works.

Copy link
Contributor Author

@JongHeonChoi JongHeonChoi Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kouvel Yes. I checked the return value(result >= 0 ? result : 0;) of the test case in VisionFive2 target.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks! I'll look into whether this check can be removed, as I'm not aware of any current cases where it would fail.

@jkotas jkotas requested a review from kouvel September 20, 2023 13:03
@clamp03 clamp03 added the arch-riscv Related to the RISC-V architecture label Sep 21, 2023
Copy link
Member

@gbalykov gbalykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I've added pal tests results with this change too in #75749 (comment).

@kouvel kouvel merged commit 05730a3 into dotnet:main Sep 21, 2023
mikem8361 pushed a commit to dotnet/diagnostics that referenced this pull request Sep 21, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-riscv Related to the RISC-V architecture area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants