-
Notifications
You must be signed in to change notification settings - Fork 31
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
pthread library intrinsics #582
Conversation
* Ignore inline assembly, but generate nondeterministic return values * Add intrinsics for void* calloc(size_t,size_t), int pthread_mutex_trylock(void*) * Add intrinsics for pthread_cond* * Add empty intrinsics for pthread_mutexattr* * Add intrinsics for pthread_rwlock* * Add intrinsics for strcpy * Intrinsics error message now contains all missing intrinsics * Add intrinsics pthread_getspecific * Add pthread_setspecific * Add pthread_key_create * Add pthread_key_destroy --------- Co-authored-by: René Pascal Maseli <[email protected]>
dartagnan/src/main/java/com/dat3m/dartagnan/program/event/EventFactory.java
Show resolved
Hide resolved
P_THREAD_MUTEXATTR_SET(List.of( | ||
"pthread_mutexattr_setprioceiling", | ||
"pthread_mutexattr_setprotocol", | ||
"pthread_mutexattr_settype", | ||
"pthread_mutexattr_setpolicy_np"), | ||
true, false, true, true, Intrinsics::inlineAsZero), | ||
P_THREAD_MUTEXATTR_GET(List.of( | ||
"pthread_mutexattr_getprioceiling", | ||
"pthread_mutexattr_getprotocol", | ||
"pthread_mutexattr_gettype", | ||
"pthread_mutexattr_getpolicy_np"), | ||
false, true, true, true, Intrinsics::inlineAsZero), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how faithful are these implementation?
P_THREAD_RWLOCKATTR_SET("pthread_rwlockattr_setpshared", true, false, true, true, Intrinsics::inlineAsZero), | ||
P_THREAD_RWLOCKATTR_GET("pthread_rwlockattr_getpshared", true, false, true, true, Intrinsics::inlineAsZero), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how faithful are these implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementations fail e.g. this simple test, which should pass on full support:
void test() {
int pshared;
pthread_rwlockattr_t attr;
pthread_rwlockattr_init(&attr);
pthread_rwlockattr_setpshared(&attr, PTHREAD_PROCESS_PRIVATE);
pthread_rwlockattr_getpshared(&attr, &pshared);
assert(pshared == PTHREAD_PROCESS_PRIVATE);
}
I don't expect these structures to be used like that in errors we are interested in right now.
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/Intrinsics.java
Outdated
Show resolved
Hide resolved
final Register result = checkValueAndArguments(2, call); | ||
final Expression address = call.getArguments().get(0); | ||
//final Expression attributes = call.getArguments().get(1); | ||
final Expression initializedState = expressions.makeZero(types.getArchType()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the state of a conditional variable always initialize to zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value was intended to correspond with the number of threads that can be awakened. It does not take spurious wake-ups into account, though. Also, it cannot recognize if the pthread_cond_init
is missing. It should rather be 1, pthread_cond_destroy
should store 0, and the remaining functions should check this.
return List.of( | ||
EventFactory.newLocal(result, expressions.makeGeneralZero(result.getType()))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand these local storage methods, but since the create access global memory, I looks suspicious the delete does not clean/free/... those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation could treat zero as values of uninitialized objects, instead. Then, destructors can just check non-zero, then store zero. This could be done for the entire pthread library.
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/Intrinsics.java
Outdated
Show resolved
Hide resolved
private List<Event> inlinePthreadMutexTryLock(FunctionCall call) { | ||
final Register register = checkValueAndArguments(1, call); | ||
checkArgument(register.getType() instanceof IntegerType, "Wrong return type for \"%s\"", call); | ||
final var error = new INonDet(constantId++, (IntegerType) register.getType(), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean this method can fail even if the mutex is free?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a bug. I did not want to insert a CAS here, since I did not know if it would have interfered with Lock
and Unlock
. Instead I could add a new TryLock
class or replace the existing lock events with atomics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw. I think if we have like 80% of the pthread library implemented via C11 atomics, we can also go all the way and do so for the standard lock, unlock, and init, that are currently handled by special events.
We are unlikely to do verification w.r.t. to the full C11 model in the near future and that is the only model that talks about these locks as special events. It is also unlikely that the standard implementation via acquire/release deviates much from the ordering implied by the C11 model (so we are unlikely to miss bugs with a concrete implementation).
Lastly, even if we decide to do full C11, we have to introduce a lot of new events anyhow, so adding back in the three we kicked out is not going to be a big deal.
final Register dummy = call.getFunction().newRegister(types.getArchType()); | ||
final Expression zero = expressions.makeZero(types.getArchType()); | ||
final Expression one = expressions.makeOne(types.getArchType()); | ||
final var replacement = new INonDet(constantId++, types.getArchType(), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean the method can spuriously fail, or together with the assume, this gives correct semantics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have given correct semantics. I have replaced it with a CAS now, as suggested by @ThomasHaas.
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/Intrinsics.java
Outdated
Show resolved
Hide resolved
These errors suggests problems with some of the intrinsics
|
Fix pthread_cond_wait and pthread_cond_timedwait aborting if a broadcast guessed the right amount of threads to awake
The two benchmarks from above now fail due to this problem
|
The exceptions are gone now, but we return the wrong result for The task is supposed to be unsafe, but I don't understand the comment explaining why this is the case. |
The task is unsafe, because the |
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/Intrinsics.java
Outdated
Show resolved
Hide resolved
private List<Event> inlinePthreadCondSignal(FunctionCall call) { | ||
//see https://linux.die.net/man/3/pthread_cond_signal | ||
// Because of spurious wake-ups, there is no need to do anything here. | ||
final Register errorRegister = getResultRegisterAndCheckArguments(1, call); | ||
//final Expression condAddress = call.getArguments().get(0); | ||
return List.of( | ||
assignSuccess(errorRegister) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could still check that condAddress
is initialized and return EINVAL
if not. I think we should have a TODO/FIXME for that. or we put a more general FIXME somewhere that says that we assume most/all pthread intrinsics to return success.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is platform-independence: The values of error codes are platform-dependent and not exposed as-is in the LLVM IR. We would need something like int __VERIFIER_EINVAL() {return EINVAL;}
in the input program. I will add a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. That makes error code handling annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about assuming by default they success and having an option to allow failure. If the option is set, we just return a non-det value (we might need to restrict the range)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not that much a matter of allowing failure, but that of returning correct values. For cond_signal
we can precisely determine when it should succeed and when it should fail, it's just that we do not know what error code to return (it should be EINVAL
, but we don't know its value).
We could opt to just always return a non-deterministic, non-zero value as error. No need to have an option for that.
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/Intrinsics.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/Intrinsics.java
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/Intrinsics.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/Intrinsics.java
Outdated
Show resolved
Hide resolved
// Try to lock | ||
EventFactory.Atomic.newExchange(dummy, lockAddress, replacement, Tag.C11.MO_ACQUIRE), | ||
// Store one (write-locked) only if successful, else leave unchanged | ||
EventFactory.newAssume(expressions.makeEQ(replacement, properReplacement)), | ||
// Deadlock if violation occurs in another thread | ||
EventFactory.newAbortIf(locked), | ||
assignSuccess(errorRegister) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't a CAS-based solution work without any magic? CAS(&lock, 0, 1)
will succeed only if the lock is free. In a loop, it will do the correct thing (and deadlock detection will also work). Without the loop, you get the TryWrlock
implementation without spurious failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should work.
// Increment shared counter only if not locked by writer. | ||
EventFactory.Atomic.newFADD(dummy, lockAddress, increment, Tag.C11.MO_ACQUIRE), | ||
// Deadlock if a violation occurred in another thread. In this case, lock value was not changed | ||
EventFactory.newAbortIf(expressions.makeEQ(increment, unlocked)), | ||
// On success, lock cannot have been write-locked. | ||
EventFactory.newAssume(expressions.makeNEQ(dummy, wrLocked)), | ||
// On success, incremented by two, if first reader, else one. | ||
EventFactory.newAssume(expressions.makeEQ(increment, properIncrement)), | ||
assignSuccess(errorRegister) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if a CAS-based solution works here as well and circumvents the spurious store that happens if the locking fails.
expected = non_det();
desired = expected + 2;
(oldValue, success) = CAS(&lock, expected, desired)
if (oldValue != WrLocked) {
assume (success); // CAS succeeds if oldValue was fine. Implies that the guessed expected value matched oldValue.
} else {
assume (!success); // CAS must fail.
}
// Could be simplified to assume(ITE(oldValue != WrLocked, success, !success));
Putting this into a loop will give you proper deadlock handling as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. You can even simplify it to assume(expected != WrLocked);
.
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/Intrinsics.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/Intrinsics.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/Intrinsics.java
Outdated
Show resolved
Hide resolved
Fix type mismatch in pthread_mutex_trylock. Switch implementations of pthread_cond_signal and pthread_cond_broadcast.
Outlined several rwlock-related code.
The exception that is triggered in the CI is because we do not support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add handling of the nasty "\01" prefix for many pthread calls rather than removing the prefix by hand from the .ll code.
We should be able to run on unmodified .ll code.
[16.12.2023] 11:34:59 [ERROR] Dartagnan.main - Unknown intrinsics
"\01_pthread_cond_init", "\01_pthread_cond_timedwait", "\01_pthread_cond_wait",
"\01_pthread_mutexattr_destroy", "\01_pthread_rwlock_destroy",
"\01_pthread_rwlock_init", "\01_pthread_rwlock_rdlock",
"\01_pthread_rwlock_tryrdlock", "\01_pthread_rwlock_trywrlock",
"\01_pthread_rwlock_unlock", "\01_pthread_rwlock_wrlock"
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/Intrinsics.java
Outdated
Show resolved
Hide resolved
#591 alone does not fix the CI. As I said in my last post, there is a missing cast and the loop bounds of the test are insufficient (IIRC there are also unannotated loops that need annotatation). |
Ignore test for return values of pthread_join Ignore test for destructors of pthread_key_create
We still have this problem |
This is not a trivial fix because it involves handling of function pointers and instrumenting the exit code of threads. |
I am ok with keeping this as it is (I will open an issue so we don't forget about this one). @ThomasHaas do you have any further comments about the code or can I merge? @xeren can you please rebase? |
Signed-off-by: Hernan Ponce de Leon <[email protected]>
I think there are some minor things to fix, but since it is working now, we can merge. |
Adds intrinsics for several library functions required for SVCOMP 2024. This includes condition variables, rwlocks and specifics (thread-local storage). Also alters the behavior of pthread mutex intrinsics.