Fix windows binaries exiting with code 24#449
Open
isanych wants to merge 2 commits intoSRI-CSL:per-thread-statefrom
Open
Fix windows binaries exiting with code 24#449isanych wants to merge 2 commits intoSRI-CSL:per-thread-statefrom
isanych wants to merge 2 commits intoSRI-CSL:per-thread-statefrom
Conversation
…compatible with windows
Contributor
|
Is |
|
The Win32 code is wrong in that the TLS should be initialized once (in yices_init), not per thread (as is the case now in yices_per_thread_init), so consumes a slot with every call to yices_per_thread_init, which we call a lot. |
wojbas
reviewed
Oct 18, 2023
| * usage of the API. | ||
| */ | ||
| if (!yices_tls_initialized) { | ||
| yices_tls_error_index = TlsAlloc(); |
There was a problem hiding this comment.
despite the comment above this is called via yices_per_thread_init and not yices_init
Contributor
|
I’m guessing, from the reference to yices_per_thread_init, that this is on the per-thread-state branch, then?
—
Mark Mitchell
… On Oct 17, 2023, at 11:53 PM, Wojtek Basalaj ***@***.***> wrote:
@wojbas commented on this pull request.
In src/api/yices_error_report_win.c <#449 (comment)>:
> -static void thread_local_free_yices_error_report(void){
- error_report_t* tl_yices_error = TlsGetValue(yices_tls_error_index);
- safe_free(tl_yices_error);
- TlsSetValue(yices_tls_error_index, NULL);
-}
-
-
-void init_yices_error(void){
- /*
- * This is called explicitly in yices_init, so if there is a race
- * condition here, then there is a race condition b/w yices_init and
- * a call to the API. Which is not OUR problem. Just incorrect
- * usage of the API.
- */
- if (!yices_tls_initialized) {
- yices_tls_error_index = TlsAlloc();
despite the comment above this is called via yices_per_thread_init and not yices_init
—
Reply to this email directly, view it on GitHub <#449 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKVS3FPBK7TLPTF3QR4RSLX754IHAVCNFSM6AAAAAAZ6VM6SWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMOBUGIZDQNJUGY>.
You are receiving this because you commented.
|
Contributor
Author
|
yep, it is PR for per-thread-state branch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unfortunately I cannot provide reasonable reproducible example - but we had regular cases when binary sporadically exit with code 24 on big multithreaded projects on windows only, when linux binary working fine on the same project on similar hardware.
Looks like windows code intended to be initialized differently vs posix, but as posix code is working and compatible with windows we have not looked into details too much. With this change our windows binaries are working as expected, without issues seen before.