code review 4952058: windows/amd64: use dynamic thread local storage.

windows/amd64: use dynamic thread local storage.

[vcc, 2011-09-07 15:00:24]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[jp, 2011-09-07 15:26:02]

On 2011/09/07 15:00:24, vcc wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

What the problem does it solve ?

[rsc, 2011-09-07 18:32:08]

This adds an instruction and a memory reference
(to runtime.tlsindex) to the beginning of every function
body in the whole program.

Why did the current code work?
Why isn't it enough anymore?

On other systems that insist on handing out
TLS references (for example, OS X), we cut
the code back down to one instruction by
assuming we can get a specific index since
we are running so early in the program's lifetime.

Russ

[jp, 2011-09-07 18:49:13]

On 2011/09/07 18:32:08, rsc wrote:
> This adds an instruction and a memory reference
> (to runtime.tlsindex) to the beginning of every function
> body in the whole program.
> 
> Why did the current code work?
> Why isn't it enough anymore?
> 
> On other systems that insist on handing out
> TLS references (for example, OS X), we cut
> the code back down to one instruction by
> assuming we can get a specific index since
> we are running so early in the program's lifetime.
> 
> Russ

Windows has 2 tls: static and dynamic.

Static TLS variables look in C source like "__declspec(thread) int x;"
Indices of them are allocated by linker.
It is possible to have up to 64 static TLS variables.

Dynamic TLS values are allocated in runtime by calling TlsAlloc() API.
There are up to 1024 of them.

Go uses static TLS.

[rsc, 2011-09-07 18:54:40]

On Wed, Sep 7, 2011 at 14:49,  <jp@webmaster.ms> wrote:
> Go uses static TLS.

Great, thanks for the explanation.

Wei, why should we change to dynamic TLS?

Russ

[jp, 2011-09-07 19:09:19]

On 2011/09/07 18:54:40, rsc wrote:

Oops, actually it uses dynamic tls, seizing the first 2 cells without calling
TlsAlloc(). 
Exactly as you described OS X.

A bug can appear if Go code will call a DLL which uses TlsAlloc(), then the DLL
can overwrite Go's values.
So, it has sense to call TlsAlloc() and reserve the indices.
Or to switch to the static TLS.

> On Wed, Sep 7, 2011 at 14:49,  <mailto:jp@webmaster.ms> wrote:
> > Go uses static TLS.
> 
> Great, thanks for the explanation.
> 
> Wei, why should we change to dynamic TLS?
> 
> Russ

[vcc, 2011-09-08 10:41:27]

On 2011/09/07 19:09:19, jp wrote:
> On 2011/09/07 18:54:40, rsc wrote:
> 
> Oops, actually it uses dynamic tls, seizing the first 2 cells without calling
> TlsAlloc(). 
> Exactly as you described OS X.
> 
> A bug can appear if Go code will call a DLL which uses TlsAlloc(), then the
DLL
> can overwrite Go's values.
> So, it has sense to call TlsAlloc() and reserve the indices.

Yes. windows/386 also need do this to fixes this bug.

> Or to switch to the static TLS.
> 
> > On Wed, Sep 7, 2011 at 14:49,  <mailto:jp@webmaster.ms> wrote:
> > > Go uses static TLS.
> > 
> > Great, thanks for the explanation.
> > 
> > Wei, why should we change to dynamic TLS?
> > 
> > Russ

[vcc, 2011-09-08 10:42:45]

PTAL

[jp, 2011-09-08 11:25:44]

On 2011/09/08 10:42:45, vcc wrote:
> PTAL

Wouldn't something like "if(TlsAlloc()!=1 && TlsAlloc()!=2) panic()" in the
beginning of execution be a simpler solution ?
Or switching to the static TLS.
Remembering the TLS index seems superfluous, it supposes to be always the same
value.

[vcc, 2011-09-08 14:52:36]

2011/9/8  <jp@webmaster.ms>:
> On 2011/09/08 10:42:45, vcc wrote:
> Wouldn't something like "if(TlsAlloc()!=1 && TlsAlloc()!=2) panic()" in
> the beginning of execution be a simpler solution ?

Not work, I got TlsAlloc() return 6 for one test program, can't assume
TlsAlloc() return values.

> Or switching to the static TLS.

We are already using static TLS, 0x58(GS) is thread's
ThreadLocalStoragePointer for static TLS, but we just set it to one
buffer and don't setup and manage .tls, so if other dlls has static
TLS will  reset ThreadLocalStoragePointer, will fail.

I think dynamic TLS is simple and reliable, should use it instead of static TLS.

>
> http://codereview.appspot.com/4952058/
>

[jp, 2011-09-08 15:40:38]

On 2011/09/08 14:52:36, vcc wrote:
> 2011/9/8  <jp@webmaster.ms>:
> > On 2011/09/08 10:42:45, vcc wrote:
> > Wouldn't something like "if(TlsAlloc()!=1 && TlsAlloc()!=2) panic()" in
> > the beginning of execution be a simpler solution ?
> 
> Not work, I got TlsAlloc() return 6 for one test program, can't assume
> TlsAlloc() return values.
> 
> > Or switching to the static TLS.
> 
> We are already using static TLS, 0x58(GS) is thread's
> ThreadLocalStoragePointer for static TLS, but we just set it to one
> buffer and don't setup and manage .tls, so if other dlls has static
> TLS will  reset ThreadLocalStoragePointer, will fail.

By static TLS I mean the 64 entries starting from 0xE10(FS) on 386 (and some
similar big number on amd64):
http://en.wikipedia.org/wiki/Win32_Thread_Information_Block

They intended to be allocated during link time and do not interfere with the
cells allocated by TlsAlloc()

> 
> I think dynamic TLS is simple and reliable, should use it instead of static
TLS.
> 
> >
> > http://codereview.appspot.com/4952058/
> >

[jp, 2011-09-09 14:19:21]

On 2011/09/08 15:40:38, jp wrote:
> On 2011/09/08 14:52:36, vcc wrote:
> > 2011/9/8  <jp@webmaster.ms>:
> > > On 2011/09/08 10:42:45, vcc wrote:
> > > Wouldn't something like "if(TlsAlloc()!=1 && TlsAlloc()!=2) panic()" in
> > > the beginning of execution be a simpler solution ?
> > 
> > Not work, I got TlsAlloc() return 6 for one test program, can't assume
> > TlsAlloc() return values.
> > 
> > > Or switching to the static TLS.
> > 
> > We are already using static TLS, 0x58(GS) is thread's
> > ThreadLocalStoragePointer for static TLS, but we just set it to one
> > buffer and don't setup and manage .tls, so if other dlls has static
> > TLS will  reset ThreadLocalStoragePointer, will fail.
> 
> By static TLS I mean the 64 entries starting from 0xE10(FS) on 386 (and some
> similar big number on amd64):
> http://en.wikipedia.org/wiki/Win32_Thread_Information_Block
> 
> They intended to be allocated during link time and do not interfere with the
> cells allocated by TlsAlloc()

Sorry again, these 64 cell are first 64 allocated by TlsAlloc.
So they do not interfere with the cells pointed by ThreadLocalStoragePointer
which is currently used in Go runtime.

Can you provide a test case of TLS corruption you are fighting with ?
I did the following test with no problem
var (
	modkernel32 = syscall.NewLazyDLL("kernel32.dll")

	procTlsAlloc = modkernel32.NewProc("TlsAlloc")
	procTlsSetValue = modkernel32.NewProc("TlsSetValue")
)
func TlsAlloc() int {
	r1, _, _ := syscall.Syscall(procTlsAlloc.Addr(), 0, 0, 0, 0)
	return int(r1)
}
func TlsSetValue(i int, v uintptr) {
	syscall.Syscall(procTlsSetValue.Addr(), 2, uintptr(i), v, 0)
}
func main() {
	for j:=0; j<1000; j++ {
		i := TlsAlloc();
		TlsSetValue(i, 100)
	}
}


> > 
> > I think dynamic TLS is simple and reliable, should use it instead of static
> TLS.
> > 
> > >
> > > http://codereview.appspot.com/4952058/
> > >

[vcc, 2011-09-10 03:08:29]

In real world, my problem is use
go-odbc<https://github.com/weigj/go-odbc> with Oracle Database using
Oracle ODBC driver.

2011/9/9  <jp@webmaster.ms>:
> On 2011/09/08 15:40:38, jp wrote:
>>
>> On 2011/09/08 14:52:36, vcc wrote:
>> > 2011/9/8  <jp@webmaster.ms>:
>> > > On 2011/09/08 10:42:45, vcc wrote:
>> > > Wouldn't something like "if(TlsAlloc()!=1 && TlsAlloc()!=2)
>
> panic()" in
>>
>> > > the beginning of execution be a simpler solution ?
>> >
>> > Not work, I got TlsAlloc() return 6 for one test program, can't
>
> assume
>>
>> > TlsAlloc() return values.
>> >
>> > > Or switching to the static TLS.
>> >
>> > We are already using static TLS, 0x58(GS) is thread's
>> > ThreadLocalStoragePointer for static TLS, but we just set it to one
>> > buffer and don't setup and manage .tls, so if other dlls has static
>> > TLS will  reset ThreadLocalStoragePointer, will fail.
>
>> By static TLS I mean the 64 entries starting from 0xE10(FS) on 386
>
> (and some
>>
>> similar big number on amd64):
>> http://en.wikipedia.org/wiki/Win32_Thread_Information_Block
>
>> They intended to be allocated during link time and do not interfere
>
> with the
>>
>> cells allocated by TlsAlloc()
>
> Sorry again, these 64 cell are first 64 allocated by TlsAlloc.
> So they do not interfere with the cells pointed by
> ThreadLocalStoragePointer which is currently used in Go runtime.
>
> Can you provide a test case of TLS corruption you are fighting with ?
> I did the following test with no problem
> var (
>        modkernel32 = syscall.NewLazyDLL("kernel32.dll")
>
>        procTlsAlloc = modkernel32.NewProc("TlsAlloc")
>        procTlsSetValue = modkernel32.NewProc("TlsSetValue")
> )
> func TlsAlloc() int {
>        r1, _, _ := syscall.Syscall(procTlsAlloc.Addr(), 0, 0, 0, 0)
>        return int(r1)
> }
> func TlsSetValue(i int, v uintptr) {
>        syscall.Syscall(procTlsSetValue.Addr(), 2, uintptr(i), v, 0)
> }
> func main() {
>        for j:=0; j<1000; j++ {
>                i := TlsAlloc();
>                TlsSetValue(i, 100)
>        }
> }
>
>
>> >
>> > I think dynamic TLS is simple and reliable, should use it instead of
>
> static
>>
>> TLS.
>> >
>> > >
>> > > http://codereview.appspot.com/4952058/
>> > >
>
>
>
> http://codereview.appspot.com/4952058/
>

[jp, 2011-09-10 09:47:30]

On 2011/09/10 03:08:29, vcc wrote:

Can you send me the DLLs loaded into Go process ?
Your odbc32.dll and all the Oracle's DLLs which are loaded in the Go process.

> In real world, my problem is use
> go-odbc<https://github.com/weigj/go-odbc> with Oracle Database using
> Oracle ODBC driver.
> 
> 2011/9/9  <jp@webmaster.ms>:
> > On 2011/09/08 15:40:38, jp wrote:
> >>
> >> On 2011/09/08 14:52:36, vcc wrote:
> >> > 2011/9/8 &nbsp;<jp@webmaster.ms>:
> >> > > On 2011/09/08 10:42:45, vcc wrote:
> >> > > Wouldn't something like "if(TlsAlloc()!=1 && TlsAlloc()!=2)
> >
> > panic()" in
> >>
> >> > > the beginning of execution be a simpler solution ?
> >> >
> >> > Not work, I got TlsAlloc() return 6 for one test program, can't
> >
> > assume
> >>
> >> > TlsAlloc() return values.
> >> >
> >> > > Or switching to the static TLS.
> >> >
> >> > We are already using static TLS, 0x58(GS) is thread's
> >> > ThreadLocalStoragePointer for static TLS, but we just set it to one
> >> > buffer and don't setup and manage .tls, so if other dlls has static
> >> > TLS will &nbsp;reset ThreadLocalStoragePointer, will fail.
> >
> >> By static TLS I mean the 64 entries starting from 0xE10(FS) on 386
> >
> > (and some
> >>
> >> similar big number on amd64):
> >> http://en.wikipedia.org/wiki/Win32_Thread_Information_Block
> >
> >> They intended to be allocated during link time and do not interfere
> >
> > with the
> >>
> >> cells allocated by TlsAlloc()
> >
> > Sorry again, these 64 cell are first 64 allocated by TlsAlloc.
> > So they do not interfere with the cells pointed by
> > ThreadLocalStoragePointer which is currently used in Go runtime.
> >
> > Can you provide a test case of TLS corruption you are fighting with ?
> > I did the following test with no problem
> > var (
> > &nbsp; &nbsp; &nbsp; &nbsp;modkernel32 = syscall.NewLazyDLL("kernel32.dll")
> >
> > &nbsp; &nbsp; &nbsp; &nbsp;procTlsAlloc = modkernel32.NewProc("TlsAlloc")
> > &nbsp; &nbsp; &nbsp; &nbsp;procTlsSetValue =
modkernel32.NewProc("TlsSetValue")
> > )
> > func TlsAlloc() int {
> > &nbsp; &nbsp; &nbsp; &nbsp;r1, _, _ := syscall.Syscall(procTlsAlloc.Addr(),
0, 0, 0, 0)
> > &nbsp; &nbsp; &nbsp; &nbsp;return int(r1)
> > }
> > func TlsSetValue(i int, v uintptr) {
> > &nbsp; &nbsp; &nbsp; &nbsp;syscall.Syscall(procTlsSetValue.Addr(), 2,
uintptr(i), v, 0)
> > }
> > func main() {
> > &nbsp; &nbsp; &nbsp; &nbsp;for j:=0; j<1000; j++ {
> > &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;i := TlsAlloc();
> > &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;TlsSetValue(i, 100)
> > &nbsp; &nbsp; &nbsp; &nbsp;}
> > }
> >
> >
> >> >
> >> > I think dynamic TLS is simple and reliable, should use it instead of
> >
> > static
> >>
> >> TLS.
> >> >
> >> > >
> >> > > http://codereview.appspot.com/4952058/
> >> > >
> >
> >
> >
> > http://codereview.appspot.com/4952058/
> >

[rsc, 2012-06-03 04:31:23]

removing reviewers