code review 6853059: cgo: enable cgo on openbsd

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.

[jsing, 2012-11-16 16:00:37]

Hello golang-dev@googlegroups.com,

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

[minux1, 2012-11-16 17:38:11]

what if extern c code creates a thread, and a signal is
accidentally delivered to that thread?

[jsing, 2012-11-17 14:33:18]

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.

[minux1, 2012-11-17 17:06:27]

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?

[jsing, 2012-11-22 12:37:47]

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?

[minux1, 2012-11-23 19:03:45]

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)?

[iant2, 2012-11-24 18:49:49]

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

[minux1, 2012-11-24 19:32:29]

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.

[minux1, 2012-11-24 21:03:05]

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).

[jsing, 2012-11-26 15:31:53]

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.

[jsing, 2012-11-26 16:40:49]

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.

[iant2, 2012-11-26 17:02:02]

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

[minux1, 2012-11-26 17:10:29]

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

[jsing, 2012-11-27 16:19:00]

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...

[iant2, 2012-11-27 16:48:43]

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

[jsing, 2012-11-27 17:15:34]

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.

[iant2, 2012-11-27 18:45:03]

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

[minux1, 2012-11-27 20:54:19]

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.

[iant2, 2012-11-27 21:06:29]

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

[rsc, 2012-11-27 21:07:51]

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

[minux1, 2012-11-27 21:45:31]

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?

[minux1, 2012-11-27 21:51:04]

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;

[jsing, 2012-11-28 00:30:34]

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...

[minux1, 2012-12-18 15:06:08]

do you want to fix various docs in this CL or in a subsequent CL?

[minux1, 2012-12-18 15:28:29]

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

[iant, 2012-12-19 20:19:33]

LGTM

https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_ope...
File src/pkg/runtime/cgo/gcc_openbsd_386.c (right):

https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_ope...
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_ope...
File src/pkg/runtime/cgo/gcc_openbsd_amd64.c (right):

https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_ope...
src/pkg/runtime/cgo/gcc_openbsd_amd64.c:61: free(arg);
Fetch args.func and args.arg before freeing arg.

[minux1, 2012-12-19 20:21:11]

https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_ope...
File src/pkg/runtime/cgo/gcc_openbsd_386.c (right):

https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_ope...
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.

[iant, 2012-12-19 20:27:32]

https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_ope...
File src/pkg/runtime/cgo/gcc_openbsd_386.c (right):

https://codereview.appspot.com/6853059/diff/13003/src/pkg/runtime/cgo/gcc_ope...
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

[jsing, 2012-12-20 14:43:28]

*** 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