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

fix mingw timeout error #461

Merged
merged 2 commits into from
Oct 4, 2023
Merged

fix mingw timeout error #461

merged 2 commits into from
Oct 4, 2023

Conversation

ahmed-irfan
Copy link
Member

  • fix pthread issue for ming

  • the_timeout variable is used by the mingw implementation, which is not declared if THREAD_SAFE is not defined. However, it is needed for mingw. (this was changed in the recent PR about thread safety)

@ahmed-irfan ahmed-irfan requested a review from disteph October 4, 2023 04:58
@coveralls
Copy link

coveralls commented Oct 4, 2023

Coverage Status

coverage: 65.098%. remained the same when pulling a72f3a4 on fix-timeout-mingw into ef6ccf7 on master.

@markpmitchell
Copy link
Contributor

I missed the MinGW case in this code. Sorry!

I don't think using the_timeout makes sense on MinGW if thread-safe. When not thread-safe, the_timeout works -- but it's inherently wrong to use it when threaded. (We could put it in thread-local storage, but there's no reason that creation of a timeout and use of a timeout have to happen in the same thread.) IIUC, your patch would define the_timeout even when THREAD_SAFE is defined, which I think is a mistake.

So, I think the first hunk of the patch is correct (avoid the pthread stuff on MinGW), but the second hunk shouldn't be necessary. Right now, we don't have a thread-safe version of timeouts for MinGW, so we should add:

#if defined(THREAD_SAFE) && defined(MINGW)
#error "No thread-safe implementation of timeouts"
#endif

Does that make sense?

@ahmed-irfan
Copy link
Member Author

So, I think the first hunk of the patch is correct (avoid the pthread stuff on MinGW), but the second hunk shouldn't be necessary. Right now, we don't have a thread-safe version of timeouts for MinGW, so we should add:

#if defined(THREAD_SAFE) && defined(MINGW)
#error "No thread-safe implementation of timeouts"
#endif

This will make the compilation of yices fail under MinGW when using THREAD_SAFE. However, we have external users who use THREAD_SAFE Yices under windows. I don't know if that should have been allowed at the first place.
Do you see another way to fix the issue?

@markpmitchell
Copy link
Contributor

The answer to "should it have been allowed?" depends on how dogmatic we want to be. No platform was actually thread-safe when using timeouts; the timeouts weren't per-thread. But, since we have Windows users relying on THREAD_SAFE, then, IMO, we should fix thread-safe timeouts to work on Windows. I'm happy to do that, if you like -- though I need to figure out how to get access to a Windows box.

In the meantime, we could define the_timeout if !defined(THREAD_SAFE) || defined(MINGW). We shouldn't define it when THREAD_SAFE when POSIX threads because the variable is never used -- and should not be used -- in that case.

@ahmed-irfan
Copy link
Member Author

The answer to "should it have been allowed?" depends on how dogmatic we want to be. No platform was actually thread-safe when using timeouts; the timeouts weren't per-thread. But, since we have Windows users relying on THREAD_SAFE, then, IMO, we should fix thread-safe timeouts to work on Windows. I'm happy to do that, if you like -- though I need to figure out how to get access to a Windows box.

In the meantime, we could define the_timeout if !defined(THREAD_SAFE) || defined(MINGW). We shouldn't define it when THREAD_SAFE when POSIX threads because the variable is never used -- and should not be used -- in that case.

I updated the code with your suggestion. Thanks!

I will email you about the access to the windows machine that I am using.

@ahmed-irfan ahmed-irfan requested review from markpmitchell and removed request for disteph October 4, 2023 17:19
@markpmitchell
Copy link
Contributor

Looks good!

@ahmed-irfan ahmed-irfan merged commit 2cd77a6 into master Oct 4, 2023
@ahmed-irfan ahmed-irfan deleted the fix-timeout-mingw branch October 4, 2023 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants