Skip to content

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Sep 9, 2022

This reverts commit 77b696c.

fixes #19713

Turning off tlsEmulation causes an implicit dependency on libwinpthreads.dll, which is a surprising regression for users using mingw on Windows. Because all working programs need to ship libwinthreads.dll on end-users's computers. By default, it shouldn't require libwinpthreads.dll, otherwise it might break the simplest programs written in Nim implicitly.

tlsEmulation doesn't work with the Nim compiler since it has a huge rope cache which is a threadvar variable. I turn off tlsEmulation for the Nim compiler because mingw always ship libwinpthreads.dll and mingw is a necessity for compiling a Nim program, it is fair to rely on that fact.

Summary

  • turn on the tlsEmulation option for the Nim application which is consistent with former versions
  • turn off the tlsEmulation option for the Nim compiler

@ringabout ringabout changed the title Revert "Remove tlsEmulation enabled from Windows + GCC config (#19119) [backport:1.6] fixes https://github.com/nim-lang/Nim/issues/19713; Revert "Remove tlsEmulation enabled from Windows + GCC config (#19119) [backport:1.6] Sep 9, 2022
@ringabout ringabout changed the title fixes https://github.com/nim-lang/Nim/issues/19713; Revert "Remove tlsEmulation enabled from Windows + GCC config (#19119) [backport:1.6] fixes #19713; Revert "Remove tlsEmulation enabled from Windows + GCC config (#19119) [backport:1.6] Sep 9, 2022
@ringabout ringabout changed the title fixes #19713; Revert "Remove tlsEmulation enabled from Windows + GCC config (#19119) [backport:1.6] fixes #19713; Revert "Remove tlsEmulation enabled from Windows + GCC config" (#19119) [backport:1.6] Sep 9, 2022
@ringabout
Copy link
Member Author

It seems to exceed thread local storage size and seems to need more thoughts.

too large thread local storage size requested,
use -d:"nimTlsSize=X" to setup even more or stop using unittest.nimFAILURE

@ringabout
Copy link
Member Author

ringabout commented Sep 12, 2022

It is not surprising to see that the compilation needs more tls storage.

Two solutions:

  • increase nimTlsSize to 60000
  • don't make the rope cache a threadvar variable
typedef struct {tyProc__9axCnCRMUx32AHzFgBrzSMg localRaiseHook__system_2585;
tySequence__aNpJjaoUowCQc7fBn3wtDQ* threadDestructionHandlers__system_2834;
NI threadId__system_3016;
TFrame* framePtr__system_3070;
TSafePoint* excHandler__system_3071;
Exception* currException__system_3072;
GcFrameHeader* gcFramePtr__system_3073;
tyObject_GcHeap__1TRH1TZMaVZTnLNcIHuNFQ gch__system_5794;
tyObject_TimezonecolonObjectType___F8OvqlxXyGXRSiK9c1fCDVw* utcInstance__pureZtimes_1555;
tyObject_TimezonecolonObjectType___F8OvqlxXyGXRSiK9c1fCDVw* localInstance__pureZtimes_1556;
tySequence__sM4lkSb7zS6F7OVMvW9cffQ* ownArgv__pureZos_1617;
NIM_BOOL ownParsedArgv__pureZos_1618;
tyObject_PTerminalcolonObjectType___EcU8GhMNGo9bGDXbfqZ82og* gTerm__pureZterminal_17;
tyArray__b9c59bIypTigCSkwUynjAeZg cache__ropes_44;
tyObject_Gconfig__06LLZM9btHCEc6WftruBy5g gconfig__ast_987;
} NimThreadVars;

@ringabout
Copy link
Member Author

ringabout commented Sep 12, 2022

It seems reasonable for the Nim compiler to rely on libwinpthreads.dll when using Mingw. Not with the application written in Nim.

@ringabout
Copy link
Member Author

I have to remove threadvar for tlsEmulation because libraries using compiler apis will use up the thread local storage and fail.

@ringabout ringabout changed the title fixes #19713; Revert "Remove tlsEmulation enabled from Windows + GCC config" (#19119) [backport:1.6] fixes #19713; Revert "Remove tlsEmulation enabled from Windows + GCC config" (#19119) Sep 12, 2022
@ringabout ringabout added the Requires Araq To Merge PR should only be merged by Araq label Sep 15, 2022
@ringabout ringabout requested a review from Varriount September 15, 2022 15:22
@Varriount Varriount requested a review from Araq September 15, 2022 20:59
@Araq Araq merged commit 97259a5 into devel Sep 19, 2022
@Araq Araq deleted the pr_tdm branch September 19, 2022 07:16
@github-actions
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 97259a5

Hint: mm: orc; threads: on; opt: speed; options: -d:release
164228 lines; 12.071s; 842.504MiB peakmem

narimiran pushed a commit that referenced this pull request Sep 19, 2022
…config" (#19119) (#20327)

* Revert "Remove tlsEmulation enabled from Windows + GCC config (#19119) [backport:1.6]"

This reverts commit 77b696c.

* increase nimTlsSize to 48000

* enable for windows

* fixes tests

* fixes tlsEmulation:on

(cherry picked from commit 97259a5)
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
…s + GCC config" (nim-lang#19119) (nim-lang#20327)

* Revert "Remove tlsEmulation enabled from Windows + GCC config (nim-lang#19119) [backport:1.6]"

This reverts commit 77b696c.

* increase nimTlsSize to 48000

* enable for windows

* fixes tests

* fixes tlsEmulation:on
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Requires Araq To Merge PR should only be merged by Araq
Projects
None yet
2 participants