runtime: Support for shared libraries (generic parts and linux/x64+arm only for now).
Assembler changes to avoid assembler instructions that cannot use RIP-relative addressing.
Scheduler changes to allow the main go routine to exit cleanly and
relinquish control back to the main program.
See also:
https://codereview.appspot.com/6926049
https://codereview.appspot.com/7064048
and
https://groups.google.com/d/topic/golang-nuts/P05BDjLcQ5k/discussion
On 2012/12/26 12:52:31, taruti wrote: > Will it be possible to use Go executables even ...
12 years, 4 months ago
(2012-12-27 17:12:59 UTC)
#3
On 2012/12/26 12:52:31, taruti wrote:
> Will it be possible to use Go executables even after this in linux chroots
> without /proc?
No, it probably wouldn't, so I've updated the CL with a change that keeps the
old method, and select between that and the /proc method depending on whether
argc is > 0 (executable case).
Thanks for spotting that.
On Friday, December 28, 2012, wrote: > On 2012/12/26 12:52:31, taruti wrote: > >> Will ...
12 years, 4 months ago
(2012-12-28 07:01:48 UTC)
#4
On Friday, December 28, 2012, wrote:
> On 2012/12/26 12:52:31, taruti wrote:
>
>> Will it be possible to use Go executables even after this in linux
>>
> chroots
>
>> without /proc?
>
> No, it probably wouldn't, so I've updated the CL with a change that
> keeps the old method, and select between that and the /proc method
> depending on whether argc is > 0 (executable case).
>
it's ok to depend on /proc, as it is also used by std. library
in many cases. there are things that are not doable without /proc.
Maybe. I haven't looked at the changes very much, but I imagine this is going ...
12 years, 4 months ago
(2012-12-28 07:42:44 UTC)
#5
Maybe. I haven't looked at the changes very much, but I imagine this
is going to require changes to FreeBSD (happy to look into that) and
OS X (and other BSDs) at the least?
--dho
2012/12/28 minux <minux.ma@gmail.com>:
>
> On Friday, December 28, 2012, wrote:
>>
>> On 2012/12/26 12:52:31, taruti wrote:
>>>
>>> Will it be possible to use Go executables even after this in linux
>>
>> chroots
>>>
>>> without /proc?
>>
>> No, it probably wouldn't, so I've updated the CL with a change that
>> keeps the old method, and select between that and the /proc method
>> depending on whether argc is > 0 (executable case).
>
> it's ok to depend on /proc, as it is also used by std. library
> in many cases. there are things that are not doable without /proc.
On 2012/12/28 07:42:44, dho wrote: > Maybe. I haven't looked at the changes very much, ...
12 years, 4 months ago
(2012-12-28 15:46:27 UTC)
#6
On 2012/12/28 07:42:44, dho wrote:
> Maybe. I haven't looked at the changes very much, but I imagine this
> is going to require changes to FreeBSD (happy to look into that) and
> OS X (and other BSDs) at the least?
>
> --dho
>
For building go shared libraries on the BSDs and OS X (and the other archs and
os'es), yes, changes are needed, though the work should be less since a fair
amount of this CL is generic. I'm currently working on the linux/arm combo,
though progress is slow since I only have an Android phone and no ARM assembler
experience to work with.
- elias
PTAL Thank you very much for the detailed review :) I've since added linux/arm support ...
12 years, 3 months ago
(2013-01-06 04:16:52 UTC)
#8
PTAL
Thank you very much for the detailed review :)
I've since added linux/arm support to this and the tools CL, so a look at
that would also be much appreciated!
On Wed, Jan 2, 2013 at 10:36 PM, <rsc@golang.org> wrote:
> Some low-level comments below. I apologize that we don't have gofmt for
> C code.
>
> I would like to understand a bit better what the model is that we want
> to provide when invoking Go code from a shared library. Can you write a
> paragraph or two (in email) about what users should expect?
>
> For example:
> - It appears that os.Args will not be initialized. What other changes
> are there?
> - Can you load multiple Go shared objects or just one?
> - When does package initialization happen?
I've changed as little as possible in the runtime, because I wanted to
focus on getting the low level details sorted first (PIC mostly). The
resulting shared library is very similar to a cgo enabled (non static)
executable, with a few user-visible differences:
1. No os.Args
2. Initialization and main.main is run on library init (DT_INIT) which
happens immediately after the dynamic linker loaded and initialized the
library. That means that main.main should be regarded as a "dllmain"
routine and should probably be empty or only perform additional
initialization. When main.main returns, the linker completes library load
and return control to the main executable again.
3. The main executable can call into go functions exported through cgo, but
only on the thread that initialized the library (typically the main
thread). This restriction mirrors the cgo restriction where native shared
libraries loaded from a go program through cgo cannot create threads and
call into go on them.
4. Multiple go shared libraries are not supported. It is theoretically
possible, but changes would be needed to enable support. At the very least,
all Go runtime symbols would need to be private to avoid name clashing. A
more advanced solution would share the Go runtime between libraries.
>
>
>
>
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/asm_amd64.s<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/asm_amd64.s>
> File src/pkg/runtime/asm_amd64.s (right):
>
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/asm_amd64.s#**newcode84<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/asm_amd64.s#newcode84>
> src/pkg/runtime/asm_amd64.s:**84: libret:
> elfret:
> // ELF shared library return
>
>
Done.
> extra points for another comment with a link to the spec for this
> procedure
>
>
I don't understand this point? The return is simply restoring registers
according to the ABI.
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/asm_amd64.s#**newcode85<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/asm_amd64.s#newcode85>
> src/pkg/runtime/asm_amd64.s:**85: ADDQ $(4*8 + 16), SP //
> 2args 2auto
> delete comment
>
>
Done. I opted to save SP on the stack instead of computing it, since
the ANDQ $~15, SP alignment ruins the reversability of SP.
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/asm_amd64.s#**newcode177<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/asm_amd64.s#newcode177>
> src/pkg/runtime/asm_amd64.s:**177: CMPB runtime·islibrary(SB), $1
> I don't understand this. mcall is not supposed to return in most of the
> contexts where it is called. Being in a library does not change all
> those contexts. Which context is supposed to expect the return? Perhaps
> we can put a flag in the m struct or something like that.
>
>
After initialization and main.main has run, control needs to be
relinquished to the dynamic linker/loader to allow the main executable to
continue/start. The runtime·badmcall2 call would prevent that if not for
that check.
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/os_linux.h<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/os_linux.h>
> File src/pkg/runtime/os_linux.h (right):
>
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/os_linux.h#**newcode11<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/os_linux.h#newcode11>
> src/pkg/runtime/os_linux.h:11: int32 runtime·open(uint8*, int32, int32);
> tabs, like the surrounding lines
>
>
Done.
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/proc.c<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/proc.c>
> File src/pkg/runtime/proc.c (right):
>
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/proc.c#newcode15<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/proc.c#newcode15>
> src/pkg/runtime/proc.c:15: bool runtime·islibrary = 0;
> = 0 is redundant. remove.
>
>
Done.
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/proc.c#newcode250<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/proc.c#newcode250>
> src/pkg/runtime/proc.c:250: if (!runtime·islibrary) {
> no space
> But really this should be
>
> if(runtime.islibrary)
> return;
>
> and then the rest of the function can be normal.
>
>
Done
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/proc.c#newcode625<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/proc.c#newcode625>
> src/pkg/runtime/proc.c:625: if (&m->g0->sched.pc == (void *)-1)
> No space between if and (.
> Also I think the new condition would be clearer as
>
> if(!runtime.islibrary)
>
>
Done.
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/proc.c#newcode768<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/proc.c#newcode768>
> src/pkg/runtime/proc.c:768: static bool completed = 0;
> s/ = 0//
>
>
Done
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/proc.c#newcode768<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/proc.c#newcode768>
> src/pkg/runtime/proc.c:768: static bool completed = 0;
> s/ = 0//
> s/completed/m0done/
>
>
Done
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/proc.c#newcode786<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/proc.c#newcode786>
> src/pkg/runtime/proc.c:786: if (completed && m == &runtime·m0)
> no space
>
>
Done
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/proc.c#newcode788<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/proc.c#newcode788>
> src/pkg/runtime/proc.c:788: if (!runtime·islibrary)
> no space
>
>
Done
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/rt0_linux_amd64.s<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/rt0_linux_amd64.s>
> File src/pkg/runtime/rt0_linux_**amd64.s (right):
>
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/rt0_linux_amd64.s#**newcode13<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/rt0_linux_amd64.s#newcode13>
> src/pkg/runtime/rt0_linux_**amd64.s:13: SUBQ $0x58, SP /* keeps
> stack
> pointer 32-byte aligned */
> This comment is unclear. (0x58 > 32)
>
>
Done. TBH, I stole it from pkg/runtime/cgo/gcc_amd64.S since library init
is conceptually the same as a C->Go cgo call.
> Perhaps instead of that new code in rt0_amd64, the JMP below can become
> a call, and then the restoring code can be done here instead of in
> rt0_amd64?
>
>
Done.
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/runtime.c<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/runtime.c>
> File src/pkg/runtime/runtime.c (right):
>
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/runtime.c#**newcode100<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/runtime.c#newcode100>
> src/pkg/runtime/runtime.c:100: int32 i, n = 0;
> no initialization in variable declarations.
>
>
Done.
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/runtime.c#**newcode102<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/runtime.c#newcode102>
> src/pkg/runtime/runtime.c:102: if (argc > 0)
> no space
>
> n = 0;
> if(argc > 0)
> while(argv[argc+1+n] != 0)
> n++;
>
>
Done
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/symtab.c<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/symtab.c>
> File src/pkg/runtime/symtab.c (right):
>
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/symtab.c#**newcode105<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/symtab.c#newcode105>
> src/pkg/runtime/symtab.c:105: if (runtime·strcmp((byte
> *)"runtime.findfunc", sym->name) == 0)
> no space between if and (
> no space between (byte and *)
>
> Perhaps the linker could define a variable at shared library 0 instead
> of needing to compute it this way?
>
> xdefine("reloffset", SRODATA, 0);
>
> seems like it would work.
>
>
Done. It works, but I'm not sure why :)
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/vdso_linux_amd64.c<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/vdso_linux_amd64.c>
> File src/pkg/runtime/vdso_linux_**amd64.c (right):
>
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/vdso_linux_amd64.**c#newcode296<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/vdso_linux_amd64.c#newcode296>
> src/pkg/runtime/vdso_linux_**amd64.c:296: static uint64
> runtime·atolhex(byte *s, uint32 count) {
> \n before runtime
> \n before {
>
>
Done. I took the liberty to reformat a few other functions in that file.
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/vdso_linux_amd64.**c#newcode302<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/vdso_linux_amd64.c#newcode302>
> src/pkg/runtime/vdso_linux_**amd64.c:302: if(*s >= '0' && *s <= '9')
> insert n *= 16;
> and then you can use n += below
>
>
Done.
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/vdso_linux_amd64.**c#newcode345<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/vdso_linux_amd64.c#newcode345>
> src/pkg/runtime/vdso_linux_**amd64.c:345: byte vdso_name[] = {'[', 'v',
> 'd', 's', 'o', ']'};
> make this a global
>
>
Done.
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/vdso_linux_amd64.**c#newcode346<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/vdso_linux_amd64.c#newcode346>
> src/pkg/runtime/vdso_linux_**amd64.c:346: int32 vdso_name_idx = 0,
> start_idx = 0;
> no initializations in declarations please
>
>
Done.
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/vdso_linux_amd64.**c#newcode351<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/vdso_linux_amd64.c#newcode351>
> src/pkg/runtime/vdso_linux_**amd64.c:351: if (maps_fd < 0)
> no space. etc.
>
>
Done
> https://codereview.appspot.**com/6822078/diff/15001/src/**
>
pkg/runtime/vdso_linux_amd64.**c#newcode360<https://codereview.appspot.com/6822078/diff/15001/src/pkg/runtime/vdso_linux_amd64.c#newcode360>
> src/pkg/runtime/vdso_linux_**amd64.c:360: case '\n':
> do not indent case statements. the words switch and case should be
> aligned.
>
>
Done
>
https://codereview.appspot.**com/6822078/<https://codereview.appspot.com/6822...
>
I've hg sync'ed and updated the linker to output relative offsets for the GC_CALL gc ...
12 years, 3 months ago
(2013-01-19 13:58:23 UTC)
#9
I've hg sync'ed and updated the linker to output relative offsets for the
GC_CALL gc instruction, to avoid having to dynamically relocate .gcbss and
.gcdata.
Please split the symtab and mgc0 changes, along with the needed change to ld/lib.c, into ...
12 years, 2 months ago
(2013-01-31 01:30:17 UTC)
#11
Please split the symtab and mgc0 changes, along with the needed change to
ld/lib.c, into a separate CL. Those I'm comfortable with and we can get them
submitted.
Then please also split the auxv stuff into a separate CL. I'm not 100% sure
about that - I am holding out hope that there is some way to find auxv without
reading it from /proc - but it is certainly something we can do as a distinct
step.
This CL will then contain only the new design decisions about this "library"
mode. This is the part I am fundamentally unsure about. There are complications
having to do with what happens when the m0 exits, how you handle multiple
threads calling into Go simultaneously, why there is no os.Args, whether signal
handlers should persist on return, whether other M's that are started during the
call should be killed at return, and so on. But at least if we get the other
mechanical details done we can have that conversation.
Thanks.
On 2013/01/31 01:30:17, rsc wrote: > Please split the symtab and mgc0 changes, along with ...
12 years, 2 months ago
(2013-01-31 18:43:20 UTC)
#12
On 2013/01/31 01:30:17, rsc wrote:
> Please split the symtab and mgc0 changes, along with the needed change to
> ld/lib.c, into a separate CL. Those I'm comfortable with and we can get them
> submitted.
>
Done. https://codereview.appspot.com/7248048
> Then please also split the auxv stuff into a separate CL. I'm not 100% sure
> about that - I am holding out hope that there is some way to find auxv without
> reading it from /proc - but it is certainly something we can do as a distinct
> step.
>
Done. https://codereview.appspot.com/7221080/
Instead of getting auxv from a different place, maybe we can lift the dependency
on auxv altogether when in library mode.
VDSO can be probably be handled by letting the dynamic linker link to the vdso
library. Normal, statically linked executables will still need the auxv loader,
of course.
That leaves the information go gets from auxv on arm, which might similarly be
obtained elsewhere.
> This CL will then contain only the new design decisions about this "library"
> mode. This is the part I am fundamentally unsure about. There are
complications
> having to do with what happens when the m0 exits, how you handle multiple
> threads calling into Go simultaneously, why there is no os.Args, whether
signal
> handlers should persist on return, whether other M's that are started during
the
> call should be killed at return, and so on. But at least if we get the other
> mechanical details done we can have that conversation.
>
> Thanks.
FYI: I'm working on the signal handler issue, and another issue that seems to
involve assumptions about thread local storage. I'm not sure why you wouldn't
persist the signal handlers? The go-implemented library is bound to be called
into again from the main executable, so imho a way to call up through the signal
handler chain should be devised.
Thank you for the reviews and help submitting this, it helps my further work to
have the "easy", but extensive changes submitted and out of the way.
Issue 6822078: code review 6822078: runtime: Support for shared libraries (generic parts an...
(Closed)
Created 12 years, 5 months ago by elias.naur
Modified 11 years, 10 months ago
Reviewers: taruti, minux1, dho, rsc
Base URL:
Comments: 21
