Code review - Issue 6853059: code review 6853059: cgo: enable cgo on openbsdhttps://codereview.appspot.com/2012-12-20T14:43:28+00:00rietveld
Message from unknown
2012-11-16T13:53:40+00:00jsingurn:md5:92790aaf384af81a7cd25b60934bc7a5
Message from unknown
2012-11-16T13:53:46+00:00jsingurn:md5:b0bf7fad47acf23ecb27323f139c2b8a
Message from unknown
2012-11-16T15:15:21+00:00jsingurn:md5:6676f3b993c0622011aa9436cae81afd
Message from unknown
2012-11-16T15:58:15+00:00jsingurn:md5:0466422162ae8130ec5878e5c6f4d00d
Message from unknown
2012-11-16T16:00:27+00:00jsingurn:md5:8434a39fdde29221dcdfb6acf7768f2e
Message from jsing@google.com
2012-11-16T16:00:37+00:00jsingurn:md5:2d298955d6e123fc3e093e04028805e0
Hello golang-dev@googlegroups.com,
I'd like you to review this change to
https://go.googlecode.com/hg/
Message from minux.ma@gmail.com
2012-11-16T17:38:11+00:00minux1urn:md5:61599ca08f26ba1cae3dd9134cdc200d
what if extern c code creates a thread, and a signal is
accidentally delivered to that thread?
Message from jsing@google.com
2012-11-17T14:33:18+00:00jsingurn:md5:b5fd06636ea6044d521a4fe2418fa91e
On 17 November 2012 04:38, minux <minux.ma@gmail.com> wrote:
> what if extern c code creates a thread, and a signal is
> accidentally delivered to that thread?
Good catch. Obviously we cannot rely on m being 0, which is what the
current test is sigtramp checks. I should be able to add a canary to
the cgo allocated TLS and we can look for that in the sigtramp - if
cgo is enabled and the canary does not exist then we can assume Go did
not create the thread.
Message from minux.ma@gmail.com
2012-11-17T17:06:27+00:00minux1urn:md5:407933b872c88b530f432b97bf24aefe
On Sat, Nov 17, 2012 at 10:33 PM, Joel Sing <jsing@google.com> wrote:
> On 17 November 2012 04:38, minux <minux.ma@gmail.com> wrote:
> > what if extern c code creates a thread, and a signal is
> > accidentally delivered to that thread?
>
> Good catch. Obviously we cannot rely on m being 0, which is what the
> current test is sigtramp checks. I should be able to add a canary to
> the cgo allocated TLS and we can look for that in the sigtramp - if
> cgo is enabled and the canary does not exist then we can assume Go did
> not create the thread.
>
my proposed solution is that we wrap and override pthread_create so that
we can always allocate our own TLS when creating new threads. WDYT?
Message from unknown
2012-11-17T17:32:21+00:00jsingurn:md5:c8a67c2b1acbb184a4bf8584b80ad3f8
Message from jsing@google.com
2012-11-22T12:37:47+00:00jsingurn:md5:8bec18a121ef5790953ff29badcd7a92
On 18 November 2012 04:06, minux <minux.ma@gmail.com> wrote:
>
> On Sat, Nov 17, 2012 at 10:33 PM, Joel Sing <jsing@google.com> wrote:
>>
>> On 17 November 2012 04:38, minux <minux.ma@gmail.com> wrote:
>> > what if extern c code creates a thread, and a signal is
>> > accidentally delivered to that thread?
>>
>> Good catch. Obviously we cannot rely on m being 0, which is what the
>> current test is sigtramp checks. I should be able to add a canary to
>> the cgo allocated TLS and we can look for that in the sigtramp - if
>> cgo is enabled and the canary does not exist then we can assume Go did
>> not create the thread.
>
> my proposed solution is that we wrap and override pthread_create so that
> we can always allocate our own TLS when creating new threads. WDYT?
That would be an option, however I cannot see how this would provide
any advantage over what has been implemented in this change. In the
case of a non-cgo thread, we still cannot assign a g/m even if we
allocated TLS to store it. Or am I overlooking something?
Message from minux.ma@gmail.com
2012-11-23T19:03:45+00:00minux1urn:md5:646f574b3924933f820f359f227445ce
On Thu, Nov 22, 2012 at 8:37 PM, Joel Sing <jsing@google.com> wrote:
> On 18 November 2012 04:06, minux <minux.ma@gmail.com> wrote:
> > On Sat, Nov 17, 2012 at 10:33 PM, Joel Sing <jsing@google.com> wrote:
> > my proposed solution is that we wrap and override pthread_create so that
> > we can always allocate our own TLS when creating new threads. WDYT?
>
> That would be an option, however I cannot see how this would provide
> any advantage over what has been implemented in this change. In the
> case of a non-cgo thread, we still cannot assign a g/m even if we
> allocated TLS to store it. Or am I overlooking something?
>
my proposal is this:
we define and export a pthread_create in runtime/cgo that calls the real
pthread_create
in libpthread, and then allocate two more slots for TLS for the new thread
so that
the signal handler will read zero m & g and correctly detect the signal is
received
on a foreign thread.
the question is, can we override symbols this way (i think so)?
Message from iant@google.com
2012-11-24T18:49:49+00:00iant2urn:md5:4b07c3a535da8e3b4a2261132bad4038
On Fri, Nov 23, 2012 at 11:03 AM, minux <minux.ma@gmail.com> wrote:
>
> On Thu, Nov 22, 2012 at 8:37 PM, Joel Sing <jsing@google.com> wrote:
>>
>> On 18 November 2012 04:06, minux <minux.ma@gmail.com> wrote:
>> > On Sat, Nov 17, 2012 at 10:33 PM, Joel Sing <jsing@google.com> wrote:
>> > my proposed solution is that we wrap and override pthread_create so that
>> > we can always allocate our own TLS when creating new threads. WDYT?
>>
>> That would be an option, however I cannot see how this would provide
>> any advantage over what has been implemented in this change. In the
>> case of a non-cgo thread, we still cannot assign a g/m even if we
>> allocated TLS to store it. Or am I overlooking something?
>
> my proposal is this:
> we define and export a pthread_create in runtime/cgo that calls the real
> pthread_create
> in libpthread, and then allocate two more slots for TLS for the new thread
> so that
> the signal handler will read zero m & g and correctly detect the signal is
> received
> on a foreign thread.
>
> the question is, can we override symbols this way (i think so)?
If the executable defines a function named pthread_create, then any
code linked into the executable, or any code in shared libraries
dynamically linked in, should call the pthread_create defined in the
executable. The harder part is calling the pthread_create defined in
libpthread. If it has an alias, e.g., __pthread_create, then it is
easy. Otherwise the code needs to explicitly dlopen libpthread.so and
then use dlsym.
I assume that the problem is that OpenBSD doesn't actually support
__thread variables?
Ian
Message from minux.ma@gmail.com
2012-11-24T19:32:29+00:00minux1urn:md5:744f713a56d9afefed44e08bd65c337f
On Sun, Nov 25, 2012 at 2:49 AM, Ian Lance Taylor <iant@google.com> wrote:
> > my proposal is this:
> > we define and export a pthread_create in runtime/cgo that calls the real
> > pthread_create
> > in libpthread, and then allocate two more slots for TLS for the new
> thread
> > so that
> > the signal handler will read zero m & g and correctly detect the signal
> is
> > received
> > on a foreign thread.
> >
> > the question is, can we override symbols this way (i think so)?
>
> If the executable defines a function named pthread_create, then any
> code linked into the executable, or any code in shared libraries
> dynamically linked in, should call the pthread_create defined in the
> executable. The harder part is calling the pthread_create defined in
>
great. that's also my understanding of ELF. thank you for the confirmation.
> libpthread. If it has an alias, e.g., __pthread_create, then it is
> easy. Otherwise the code needs to explicitly dlopen libpthread.so and
> then use dlsym.
>
i think this is acceptable, and i will try to write a proof of concept
implementation.
>
> I assume that the problem is that OpenBSD doesn't actually support
> __thread variables?
>
no, it doesn't support __thread storage class.
Message from minux.ma@gmail.com
2012-11-24T21:03:05+00:00minux1urn:md5:5fd74060af826b3ea6c2d53f185added
my proposal: https://codereview.appspot.com/6855086/
(only tested on openbsd/amd64).
My CL doesn't need to store a special TLS_CANARY in TLS and it can also
handle the case where the tls slot used to store the canary can't be read
(for threads not created by Go runtime).
Message from unknown
2012-11-26T15:23:56+00:00jsingurn:md5:944da4c731676d523431482c103eb7d8
Message from jsing@google.com
2012-11-26T15:31:53+00:00jsingurn:md5:fe5e432475e5b71ccdfa4ed609a1b0ea
On 25 November 2012 08:02, minux <minux.ma@gmail.com> wrote:
> my proposal: https://codereview.appspot.com/6855086/
> (only tested on openbsd/amd64).
>
> My CL doesn't need to store a special TLS_CANARY in TLS and it can also
> handle the case where the tls slot used to store the canary can't be read
> (for threads not created by Go runtime).
Thanks!
I understood what you were proposing, however I could not see the
benefit in doing this over what I had suggested - as you point out, it
is possible that the memory above the TCB is inaccessible and trying
to read it could result in a fault inside the signal trampoline. On
one hand we would be about to terminate with a "bad signal" anyway,
however this could result in programs that just hang, which would be
poor form.
I have merged your changes into my change, with a couple of minor
differences and one key difference - when we create a thread for the
Go runtime we do not need to use the pthread_create wrapper, hence we
can call the system pthread_create directly and avoid the additional
overhead. I have specifically created a library to test the external
pthread_create and signalling of the externally created threads. This
has been tested successfully on OpenBSD/amd64 -current, OpenBSD/amd64
5.2 and OpenBSD/i386 5.2.
This also ponders the question - since we are wrapping the
pthread_create we could potentially use sigprocmask to prevent signals
being delivered to the thread, although this may lead to some weird
failures if the C code was signalling itself and expecting the signal
to be delivered (if the C library is messing with signal handlers then
we probably already have bigger problems though).
PTAL.
Message from jsing@google.com
2012-11-26T16:40:49+00:00jsingurn:md5:bc7705bb2d942e5b479b456cbfb9e0fd
On 27 November 2012 02:31, Joel Sing <jsing@google.com> wrote:
> On 25 November 2012 08:02, minux <minux.ma@gmail.com> wrote:
>> my proposal: https://codereview.appspot.com/6855086/
>> (only tested on openbsd/amd64).
>>
>> My CL doesn't need to store a special TLS_CANARY in TLS and it can also
>> handle the case where the tls slot used to store the canary can't be read
>> (for threads not created by Go runtime).
>
> Thanks!
>
> I understood what you were proposing, however I could not see the
> benefit in doing this over what I had suggested - as you point out, it
> is possible that the memory above the TCB is inaccessible and trying
> to read it could result in a fault inside the signal trampoline. On
> one hand we would be about to terminate with a "bad signal" anyway,
> however this could result in programs that just hang, which would be
> poor form.
>
> I have merged your changes into my change, with a couple of minor
> differences and one key difference - when we create a thread for the
> Go runtime we do not need to use the pthread_create wrapper, hence we
> can call the system pthread_create directly and avoid the additional
> overhead. I have specifically created a library to test the external
> pthread_create and signalling of the externally created threads. This
> has been tested successfully on OpenBSD/amd64 -current, OpenBSD/amd64
> 5.2 and OpenBSD/i386 5.2.
>
> This also ponders the question - since we are wrapping the
> pthread_create we could potentially use sigprocmask to prevent signals
> being delivered to the thread, although this may lead to some weird
> failures if the C code was signalling itself and expecting the signal
> to be delivered (if the C library is messing with signal handlers then
> we probably already have bigger problems though).
>
> PTAL.
Hrmmm... bad news - while this works correctly for Go created threads,
the use of pthread_create in an external library is still resulting in
the system libpthread version being used directly.
Message from iant@google.com
2012-11-26T17:02:02+00:00iant2urn:md5:17c7044e0e1336d3736b40b387f6ccd7
On Mon, Nov 26, 2012 at 8:40 AM, Joel Sing <jsing@google.com> wrote:
> On 27 November 2012 02:31, Joel Sing <jsing@google.com> wrote:
>> On 25 November 2012 08:02, minux <minux.ma@gmail.com> wrote:
>>> my proposal: https://codereview.appspot.com/6855086/
>>> (only tested on openbsd/amd64).
>>>
>>> My CL doesn't need to store a special TLS_CANARY in TLS and it can also
>>> handle the case where the tls slot used to store the canary can't be read
>>> (for threads not created by Go runtime).
>>
>> Thanks!
>>
>> I understood what you were proposing, however I could not see the
>> benefit in doing this over what I had suggested - as you point out, it
>> is possible that the memory above the TCB is inaccessible and trying
>> to read it could result in a fault inside the signal trampoline. On
>> one hand we would be about to terminate with a "bad signal" anyway,
>> however this could result in programs that just hang, which would be
>> poor form.
>>
>> I have merged your changes into my change, with a couple of minor
>> differences and one key difference - when we create a thread for the
>> Go runtime we do not need to use the pthread_create wrapper, hence we
>> can call the system pthread_create directly and avoid the additional
>> overhead. I have specifically created a library to test the external
>> pthread_create and signalling of the externally created threads. This
>> has been tested successfully on OpenBSD/amd64 -current, OpenBSD/amd64
>> 5.2 and OpenBSD/i386 5.2.
>>
>> This also ponders the question - since we are wrapping the
>> pthread_create we could potentially use sigprocmask to prevent signals
>> being delivered to the thread, although this may lead to some weird
>> failures if the C code was signalling itself and expecting the signal
>> to be delivered (if the C library is messing with signal handlers then
>> we probably already have bigger problems though).
>>
>> PTAL.
>
> Hrmmm... bad news - while this works correctly for Go created threads,
> the use of pthread_create in an external library is still resulting in
> the system libpthread version being used directly.
Does pthread_create appear in the dynamic symbol table of the Go
binary? You can dump the dynamic symbol table using readelf -s and
looking at the .dynsym section, or by using nm --dynamic.
Ian
Message from minux.ma@gmail.com
2012-11-26T17:10:29+00:00minux1urn:md5:b3ca702da6432d2e65f8c7fa85b3013d
On Tue, Nov 27, 2012 at 1:02 AM, Ian Lance Taylor <iant@google.com> wrote:
> On Mon, Nov 26, 2012 at 8:40 AM, Joel Sing <jsing@google.com> wrote:
> > Hrmmm... bad news - while this works correctly for Go created threads,
> > the use of pthread_create in an external library is still resulting in
> > the system libpthread version being used directly.
>
This is pretty strange.
>
> Does pthread_create appear in the dynamic symbol table of the Go
> binary? You can dump the dynamic symbol table using readelf -s and
> looking at the .dynsym section, or by using nm --dynamic.
>
it does.
Symbol table '.dynsym' contains 31 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
1: 000000000060144f 0 OBJECT GLOBAL DEFAULT 11 crosscall2
2: 0000000000600f10 0 FUNC GLOBAL DEFAULT 11 _cgo_allocate
3: 0000000000600fc0 0 FUNC GLOBAL DEFAULT 11 _cgo_panic
4: 000000000099ab60 0 OBJECT GLOBAL DEFAULT 14 environ
5: 000000000099aa88 0 OBJECT GLOBAL DEFAULT 14 __progname
6: 000000000099aa44 0 OBJECT GLOBAL DEFAULT 14 __guard_local
7: 0000000000601220 0 OBJECT GLOBAL DEFAULT 11 pthread_create
Message from unknown
2012-11-27T16:05:16+00:00jsingurn:md5:9e5ed04f9cc8c082ed9702b7278a03f1
Message from jsing@google.com
2012-11-27T16:19:00+00:00jsingurn:md5:709bd025fabdea216ad4132f5fe59377
On 27 November 2012 04:10, minux <minux.ma@gmail.com> wrote:
>
> On Tue, Nov 27, 2012 at 1:02 AM, Ian Lance Taylor <iant@google.com> wrote:
>>
>> On Mon, Nov 26, 2012 at 8:40 AM, Joel Sing <jsing@google.com> wrote:
>> > Hrmmm... bad news - while this works correctly for Go created threads,
>> > the use of pthread_create in an external library is still resulting in
>> > the system libpthread version being used directly.
>
> This is pretty strange.
>>
>>
>> Does pthread_create appear in the dynamic symbol table of the Go
>> binary? You can dump the dynamic symbol table using readelf -s and
>> looking at the .dynsym section, or by using nm --dynamic.
>
> it does.
> Symbol table '.dynsym' contains 31 entries:
> Num: Value Size Type Bind Vis Ndx Name
> 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
> 1: 000000000060144f 0 OBJECT GLOBAL DEFAULT 11 crosscall2
> 2: 0000000000600f10 0 FUNC GLOBAL DEFAULT 11 _cgo_allocate
> 3: 0000000000600fc0 0 FUNC GLOBAL DEFAULT 11 _cgo_panic
> 4: 000000000099ab60 0 OBJECT GLOBAL DEFAULT 14 environ
> 5: 000000000099aa88 0 OBJECT GLOBAL DEFAULT 14 __progname
> 6: 000000000099aa44 0 OBJECT GLOBAL DEFAULT 14 __guard_local
> 7: 0000000000601220 0 OBJECT GLOBAL DEFAULT 11 pthread_create
>
Ugh, so I missed an important part in the merge (the dynsym export).
Having fixed this I now get the pthread_create symbol in the .dynsym
table and the external library is using the correct pthread_create
function. However, we now have another issue...
The pthread_create entry in the .dynsym table generated by Go has a
size of 0. The libpthread entry has an actual size (currently 603
bytes). This is also referenced in the external library. When the
binding occurs ld.so loads compares the sizes and if they are not the
same, it generates a warning. This means that any external code that
calls pthread_create will result in:
WARNING: symbol(pthread_create) size mismatch, relink your program
The only real solution would be to make the .dynsym pthread_create
entry generated by Go match the size up with the libpthread .dynsym
entry, which is a pretty large hack.
So I guess we either put up with this, or we return to the idea of a
TLS canary slot and run the risk of faulting on read...
Message from iant@google.com
2012-11-27T16:48:43+00:00iant2urn:md5:ca254c2b6e8ba4bccaf03f10576c11b2
On Tue, Nov 27, 2012 at 8:18 AM, Joel Sing <jsing@google.com> wrote:
>
> The pthread_create entry in the .dynsym table generated by Go has a
> size of 0. The libpthread entry has an actual size (currently 603
> bytes). This is also referenced in the external library. When the
> binding occurs ld.so loads compares the sizes and if they are not the
> same, it generates a warning. This means that any external code that
> calls pthread_create will result in:
>
> WARNING: symbol(pthread_create) size mismatch, relink your program
That warning would be appropriate for a variable, but it makes no
sense for a function. Make sure that pthread_create has type STT_FUNC
in your entry in the dynamic symbol table.
If your dynamic linker is really issuing a warning about a size change
in an STT_FUNC symbol, fix the dynamic linker.
Ian
Message from jsing@google.com
2012-11-27T17:15:34+00:00jsingurn:md5:1ec27db6b3dd99c4a89c90720e016479
On 28 November 2012 03:48, Ian Lance Taylor <iant@google.com> wrote:
> On Tue, Nov 27, 2012 at 8:18 AM, Joel Sing <jsing@google.com> wrote:
>>
>> The pthread_create entry in the .dynsym table generated by Go has a
>> size of 0. The libpthread entry has an actual size (currently 603
>> bytes). This is also referenced in the external library. When the
>> binding occurs ld.so loads compares the sizes and if they are not the
>> same, it generates a warning. This means that any external code that
>> calls pthread_create will result in:
>>
>> WARNING: symbol(pthread_create) size mismatch, relink your program
>
> That warning would be appropriate for a variable, but it makes no
> sense for a function. Make sure that pthread_create has type STT_FUNC
> in your entry in the dynamic symbol table.
Indeed - thanks for the pointer. The Go generated binary has
pthread_create as STT_OBJECT rather than STT_FUNC:
7: 0000000000601220 0 OBJECT GLOBAL DEFAULT 11 pthread_create
So it would appear that "#pragma dynexport pthread_create
pthread_create" is resulting in the wrong symbol type. Is there
something else that we should be adding/doing, or is this a bug in the
Go linker?
> If your dynamic linker is really issuing a warning about a size change
> in an STT_FUNC symbol, fix the dynamic linker.
The ld.so code appears correct - it only warns for
ELF_ST_TYPE(*this)->st_info) != STT_FUNC.
Message from iant@google.com
2012-11-27T18:45:03+00:00iant2urn:md5:9233fe879a611c060478d3e897a410b3
On Tue, Nov 27, 2012 at 9:15 AM, Joel Sing <jsing@google.com> wrote:
>
> So it would appear that "#pragma dynexport pthread_create
> pthread_create" is resulting in the wrong symbol type. Is there
> something else that we should be adding/doing, or is this a bug in the
> Go linker?
I think this is something that has to be changed in the Go linker
itself, yes. Offhand I'm not sure what is involved.
Ian
Message from minux.ma@gmail.com
2012-11-27T20:54:19+00:00minux1urn:md5:bd9bb8075a2ce94ffc4a9b1648ae0bf0
On Wed, Nov 28, 2012 at 2:45 AM, Ian Lance Taylor <iant@google.com> wrote:
> On Tue, Nov 27, 2012 at 9:15 AM, Joel Sing <jsing@google.com> wrote:
> > So it would appear that "#pragma dynexport pthread_create
> > pthread_create" is resulting in the wrong symbol type. Is there
> > something else that we should be adding/doing, or is this a bug in the
> > Go linker?
>
> I think this is something that has to be changed in the Go linker
> itself, yes. Offhand I'm not sure what is involved.
>
I propose we extent format of #pragma dynexport to something like this:
#pragma dynexport pthread_create pthread_create func
this is a backward compatible change.
If this sounds good to you, I will prepare a CL.
Message from iant@google.com
2012-11-27T21:06:29+00:00iant2urn:md5:3cfe34a2cfa0fb6be0fe05a08dfc3a04
On Tue, Nov 27, 2012 at 12:53 PM, minux <minux.ma@gmail.com> wrote:
>
> On Wed, Nov 28, 2012 at 2:45 AM, Ian Lance Taylor <iant@google.com> wrote:
>>
>> On Tue, Nov 27, 2012 at 9:15 AM, Joel Sing <jsing@google.com> wrote:
>> > So it would appear that "#pragma dynexport pthread_create
>> > pthread_create" is resulting in the wrong symbol type. Is there
>> > something else that we should be adding/doing, or is this a bug in the
>> > Go linker?
>>
>> I think this is something that has to be changed in the Go linker
>> itself, yes. Offhand I'm not sure what is involved.
>
> I propose we extent format of #pragma dynexport to something like this:
> #pragma dynexport pthread_create pthread_create func
> this is a backward compatible change.
>
> If this sounds good to you, I will prepare a CL.
I'm not opposed to that but I wonder if it is necessary. We know
whether the internal symbol is TEXT or not, don't we? Could we just
base the decision on that?
Ian
Message from rsc@golang.org
2012-11-27T21:07:51+00:00rscurn:md5:ed7a3f9a7eb94da28bed83a15bc59867
I like what Ian said. If for some reason that doesn't work let's just
make ld strcmp(name, "pthread_create") == 0 to make its decision for
now. We still haven't actually seen this work, and it's pretty
disgusting already.
Russ
Message from minux.ma@gmail.com
2012-11-27T21:45:31+00:00minux1urn:md5:54e9ee0fc635ed6cab0e95f55d2e58ac
On Wed, Nov 28, 2012 at 5:06 AM, Ian Lance Taylor <iant@google.com> wrote:
> On Tue, Nov 27, 2012 at 12:53 PM, minux <minux.ma@gmail.com> wrote:
> > I propose we extent format of #pragma dynexport to something like this:
> > #pragma dynexport pthread_create pthread_create func
> > this is a backward compatible change.
> > If this sounds good to you, I will prepare a CL.
> I'm not opposed to that but I wonder if it is necessary. We know
> whether the internal symbol is TEXT or not, don't we? Could we just
> base the decision on that?
>
Silly me. It should be a bug in 6l. CL in a moment.
Possibly related to issue 4267?
Message from minux.ma@gmail.com
2012-11-27T21:51:04+00:00minux1urn:md5:0444c04020002863bdc9ecf64229a942
On Wed, Nov 28, 2012 at 5:45 AM, minux <minux.ma@gmail.com> wrote:
>
> On Wed, Nov 28, 2012 at 5:06 AM, Ian Lance Taylor <iant@google.com> wrote:
>
>> On Tue, Nov 27, 2012 at 12:53 PM, minux <minux.ma@gmail.com> wrote:
>> > I propose we extent format of #pragma dynexport to something like this:
>> > #pragma dynexport pthread_create pthread_create func
>> > this is a backward compatible change.
>> > If this sounds good to you, I will prepare a CL.
>> I'm not opposed to that but I wonder if it is necessary. We know
>> whether the internal symbol is TEXT or not, don't we? Could we just
>> base the decision on that?
>>
> Silly me. It should be a bug in 6l. CL in a moment.
> Possibly related to issue 4267?
>
This should fix the 6l for this issue, 8l could be similarly be fixed.
I still need to audit other part of the linker for this same problem.
diff -r ffd1e075c260 src/cmd/6l/asm.c
--- a/src/cmd/6l/asm.c Fri Nov 23 22:15:26 2012 -0800
+++ b/src/cmd/6l/asm.c Wed Nov 28 09:35:05 2012 +0800
@@ -455,7 +455,7 @@
adduint32(d, addstring(lookup(".dynstr", 0), name));
/* type */
t = STB_GLOBAL << 4;
- if(s->dynexport && s->type == STEXT)
+ if(s->dynexport && (s->type&SMASK) == STEXT)
t |= STT_FUNC;
else
t |= STT_OBJECT;
Message from jsing@google.com
2012-11-28T00:30:34+00:00jsingurn:md5:6c441b5294bf02001dac9bfd0dcf0d86
On 28 November 2012 08:50, minux <minux.ma@gmail.com> wrote:
>
> On Wed, Nov 28, 2012 at 5:45 AM, minux <minux.ma@gmail.com> wrote:
>>
>> On Wed, Nov 28, 2012 at 5:06 AM, Ian Lance Taylor <iant@google.com> wrote:
>>>
>>> On Tue, Nov 27, 2012 at 12:53 PM, minux <minux.ma@gmail.com> wrote:
>>> > I propose we extent format of #pragma dynexport to something like this:
>>> > #pragma dynexport pthread_create pthread_create func
>>> > this is a backward compatible change.
>>> > If this sounds good to you, I will prepare a CL.
>>> I'm not opposed to that but I wonder if it is necessary. We know
>>> whether the internal symbol is TEXT or not, don't we? Could we just
>>> base the decision on that?
>>
>> Silly me. It should be a bug in 6l. CL in a moment.
>> Possibly related to issue 4267?
>
> This should fix the 6l for this issue, 8l could be similarly be fixed.
> I still need to audit other part of the linker for this same problem.
>
> diff -r ffd1e075c260 src/cmd/6l/asm.c
> --- a/src/cmd/6l/asm.c Fri Nov 23 22:15:26 2012 -0800
> +++ b/src/cmd/6l/asm.c Wed Nov 28 09:35:05 2012 +0800
> @@ -455,7 +455,7 @@
> adduint32(d, addstring(lookup(".dynstr", 0), name));
> /* type */
> t = STB_GLOBAL << 4;
> - if(s->dynexport && s->type == STEXT)
> + if(s->dynexport && (s->type&SMASK) == STEXT)
> t |= STT_FUNC;
> else
> t |= STT_OBJECT;
>
Yes, this fixes the issue:
Symbol table '.dynsym' contains 29 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
1: 000000000041b81f 0 FUNC GLOBAL DEFAULT 11 crosscall2
2: 000000000041b2d0 0 FUNC GLOBAL DEFAULT 11 _cgo_allocate
3: 000000000041b380 0 FUNC GLOBAL DEFAULT 11 _cgo_panic
4: 0000000000520400 0 OBJECT GLOBAL DEFAULT 14 environ
5: 00000000005203b8 0 OBJECT GLOBAL DEFAULT 14 __progname
6: 0000000000520374 0 OBJECT GLOBAL DEFAULT 14 __guard_local
7: 000000000041b6c0 0 FUNC GLOBAL DEFAULT 11 pthread_create
From a quick glance there appear to be a lot of comparisons without masking...
Message from minux.ma@gmail.com
2012-12-18T15:06:08+00:00minux1urn:md5:5c76d658b47dacd850d297d17f463f93
do you want to fix various docs in this CL or in a subsequent CL?
Message from minux.ma@gmail.com
2012-12-18T15:28:29+00:00minux1urn:md5:6f7838ff4fb2776dc28ac067f49587f5
This CL is ready to go!
LGTM assuming you will fix docs and related things in a subsequent CL.
a partial list of things need to be updated:
doc/install.html: please add an entry for OpenBSD
doc/debugging_with_gdb.html
misc/dist/bindist.go: OpenBSD binary dist. supprt
Message from iant@golang.org
2012-12-19T20:19:33+00:00ianturn:md5:4221c25b20b24a30430352fd65f29050
LGTM
https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_openbsd_386.c
File src/pkg/runtime/cgo/gcc_openbsd_386.c (right):
https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_openbsd_386.c#newcode61
src/pkg/runtime/cgo/gcc_openbsd_386.c:61: free(arg);
I think it would be a lot cleaner if you fetched args.func and args.arg into local variables before you free arg. Right now you are assuming that free will not change the contents of the memory block.
https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_openbsd_amd64.c
File src/pkg/runtime/cgo/gcc_openbsd_amd64.c (right):
https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_openbsd_amd64.c#newcode61
src/pkg/runtime/cgo/gcc_openbsd_amd64.c:61: free(arg);
Fetch args.func and args.arg before freeing arg.
Message from minux.ma@gmail.com
2012-12-19T20:21:11+00:00minux1urn:md5:29a415907912dd05b06b142c84897a33
https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_openbsd_386.c
File src/pkg/runtime/cgo/gcc_openbsd_386.c (right):
https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_openbsd_386.c#newcode61
src/pkg/runtime/cgo/gcc_openbsd_386.c:61: free(arg);
On 2012/12/19 20:19:33, iant wrote:
> I think it would be a lot cleaner if you fetched args.func and args.arg into
> local variables before you free arg. Right now you are assuming that free will
> not change the contents of the memory block.
the code dereferences the arg pointer so it actually creates a copy
on the stack.
Message from iant@golang.org
2012-12-19T20:27:32+00:00ianturn:md5:2eaf13ef83e57fc4151822ab0903a2ce
https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_openbsd_386.c
File src/pkg/runtime/cgo/gcc_openbsd_386.c (right):
https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_openbsd_386.c#newcode61
src/pkg/runtime/cgo/gcc_openbsd_386.c:61: free(arg);
On 2012/12/19 20:21:11, minux wrote:
> On 2012/12/19 20:19:33, iant wrote:
> > I think it would be a lot cleaner if you fetched args.func and args.arg into
> > local variables before you free arg. Right now you are assuming that free
> will
> > not change the contents of the memory block.
> the code dereferences the arg pointer so it actually creates a copy
> on the stack.
Oh, sorry. My mistake.
LGTM
Message from unknown
2012-12-20T14:43:06+00:00jsingurn:md5:67a9ea63f6d7ecf869a991ae38e814a1
Message from jsing@google.com
2012-12-20T14:43:28+00:00jsingurn:md5:4db4b92ddeb2da9b3d323caa81a87bab
*** Submitted as https://code.google.com/p/go/source/detail?r=220ddfb20066 ***
cgo: enable cgo on openbsd
Enable cgo on OpenBSD.
The OpenBSD ld.so(1) does not currently support PT_TLS sections. Work
around this by fixing up the TCB that has been provided by librthread
and reallocating a TCB with additional space for TLS. Also provide a
wrapper for pthread_create, allowing zeroed TLS to be allocated for
threads created externally to Go.
Joint work with Shenghou Ma (minux).
Requires change 6846064.
Fixes issue 3205.
R=golang-dev, minux.ma, iant, rsc, iant
CC=golang-dev
https://codereview.appspot.com/6853059