Issue 7433043: code review 7433043: cmd/cgo: extend implementation comment (Closed)
Descriptioncmd/cgo: extend implementation comment
This is the plan for how to make host linking work with
the rest of the system.
There are two complications:
1. It is a goal to preserve the property that pure Go programs
(even ones importing "net") can be compiled without needing
gcc, so that a Go toolchain download works out of the box.
This forces the support for two linking modes: with and without
gcc.
2. It is a goal to allow users with old copies of SWIG to continue
to use those copies. This forces the support for "internal only"
packages. Perhaps it is reasonable to require a new SWIG.
I don't know.
Patch Set 1 #Patch Set 2 : diff -r 62bf913b4f40 https://go.googlecode.com/hg #
Total comments: 21
Patch Set 3 : diff -r 62bf913b4f40 https://go.googlecode.com/hg #
Total comments: 7
Patch Set 4 : diff -r 7e7041319c25 https://go.googlecode.com/hg #
Total comments: 1
MessagesTotal messages: 7
Hello iant (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg
Sign in to reply to this message.
https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go File src/cmd/cgo/doc.go (right): https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode437 src/cmd/cgo/doc.go:437: compiled in "internal only", "internal+external", or "external only" modes. I'm not quite understanding this. Is there going to be some option to decide the mode in which a package is built? https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode444 src/cmd/cgo/doc.go:444: must be supported for compatiblity with existing copies of SWIG. I don't mind saying that people must upgrade to a new version of SWIG. We've done it before. It's sort of obnoxious but I think it's acceptable. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode465 src/cmd/cgo/doc.go:465: In the <remote>, # can be used as an alias for @, to avoid The whole @ thing is a mess and I think it's best to not focus on it. The @ characters are essentially imaginary. Suggest we say something like "You can specify a symbol version, where needed, by adding '@' or '#' followed by the version string." https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode471 src/cmd/cgo/doc.go:471: #pragma cgo_dynamic_import puts puts#GLIBC_2.2.5 "libc.so.6" libc.so.6 is not a path. You don't really mean a path, you mean something that can appear in a DT_NEEDED tag. This is either a path or an soname. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode482 src/cmd/cgo/doc.go:482: #pragma dynimport is an alias for #pragma cgo_dynamic_import. Are you describing the future here? As far as I can see the current tools support dynimport, not cgo_dynamic_import. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode492 src/cmd/cgo/doc.go:492: For compatibility with current versions of SWIG, SWIG never uses #pragma dynlinker. Why would it? https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode506 src/cmd/cgo/doc.go:506: #pragma cgo_static_import <local> Why no optional <remote> here? https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode518 src/cmd/cgo/doc.go:518: In external linking mode, invoke the host linker with "<arg>" May be worth mentioning again that "the host linker" means GCC, it doesn't literally mean the linker. They take different options. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode529 src/cmd/cgo/doc.go:529: subset for the chosen linking mode. When is a package internal-only? https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode534 src/cmd/cgo/doc.go:534: and suppose the package has been compiled in "internal+external" mode. Is the package compiled in internal+external mode? If so, how? Or is this a function of some argument passed to cgo? https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode581 src/cmd/cgo/doc.go:581: reference to sin in foo.cgo2.o should be rewritten to refer to sin@GLIBC_2.2.5 s/sin@GLIBC_2.2.5/the symbol sin with version GLIBC_2.2.5/ https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode642 src/cmd/cgo/doc.go:642: Having 6l invoke the host linker will require support in the lib9 This is an interesting point but I'm not sure it belongs here.
Sign in to reply to this message.
Hello iant@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/7433043/diff/8001/src/cmd/cgo/doc.go File src/cmd/cgo/doc.go (right): https://codereview.appspot.com/7433043/diff/8001/src/cmd/cgo/doc.go#newcode416 src/cmd/cgo/doc.go:416: command) then invokes the host linker (usually gcc) to combine the Later it sounds like 6l, not the go command, is going to invoke the host linker. https://codereview.appspot.com/7433043/diff/8001/src/cmd/cgo/doc.go#newcode490 src/cmd/cgo/doc.go:490: In both internal and external linking modes, the Go symbol s/the Go symbol/put the Go symbol/ or something like that. https://codereview.appspot.com/7433043/diff/8001/src/cmd/cgo/doc.go#newcode519 src/cmd/cgo/doc.go:519: A package is compiled in "external-only" mode will only I think you mean something like "When cgo is run in external-only mode, it will generate a package that only uses cgo_export, cgo_static_import, and cgo_ldflag directives. Such a package can only be linked in external mode. When cgo is run in internal+external mode, it will use all five kinds of directives, and the linker will select the appropriate subset for the chosen linking mode." https://codereview.appspot.com/7433043/diff/8001/src/cmd/cgo/doc.go#newcode528 src/cmd/cgo/doc.go:528: and suppose the package has been compiled in "internal+external" mode. s/the package has been compiled/cgo has been run/
Sign in to reply to this message.
Thanks. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go File src/cmd/cgo/doc.go (right): https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode437 src/cmd/cgo/doc.go:437: compiled in "internal only", "internal+external", or "external only" modes. On 2013/02/28 05:50:30, iant wrote: > I'm not quite understanding this. Is there going to be some option to decide > the mode in which a package is built? It's a bit unclear to me too. At first I thought there would have to be, but now I think that cgo/swig can emit pragmas for both cases no matter what, leaving the final decision completely to link time. We can run with that for now and see how far we get. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode444 src/cmd/cgo/doc.go:444: must be supported for compatiblity with existing copies of SWIG. On 2013/02/28 05:50:30, iant wrote: > I don't mind saying that people must upgrade to a new version of SWIG. We've > done it before. It's sort of obnoxious but I think it's acceptable. Okay, good. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode465 src/cmd/cgo/doc.go:465: In the <remote>, # can be used as an alias for @, to avoid On 2013/02/28 05:50:30, iant wrote: > The whole @ thing is a mess and I think it's best to not focus on it. The @ > characters are essentially imaginary. Suggest we say something like "You can > specify a symbol version, where needed, by adding '@' or '#' followed by the > version string." Done. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode471 src/cmd/cgo/doc.go:471: #pragma cgo_dynamic_import puts puts#GLIBC_2.2.5 "libc.so.6" On 2013/02/28 05:50:30, iant wrote: > libc.so.6 is not a path. You don't really mean a path, you mean something that > can appear in a DT_NEEDED tag. This is either a path or an soname. Done. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode482 src/cmd/cgo/doc.go:482: #pragma dynimport is an alias for #pragma cgo_dynamic_import. On 2013/02/28 05:50:30, iant wrote: > Are you describing the future here? As far as I can see the current tools > support dynimport, not cgo_dynamic_import. Yes, sorry, this whole CL describes the near future. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode492 src/cmd/cgo/doc.go:492: For compatibility with current versions of SWIG, On 2013/02/28 05:50:30, iant wrote: > SWIG never uses #pragma dynlinker. Why would it? Cgo puts it in every package, so I assumed SWIG did what cgo does. The linker only needs one, so it would I suppose suffice to put it in the runtime/cgo package and not bother elsewhere. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode506 src/cmd/cgo/doc.go:506: #pragma cgo_static_import <local> On 2013/02/28 05:50:30, iant wrote: > Why no optional <remote> here? The main use of the remote in the dynamic case is to specify versions and such. At least as used by cgo, the name being imported is going to refer to a .o file that is known to be coming along during the build (i.e. we will be doing an import of _cgo_gcc_Cfunc_sin and not sin aka sin#GLIBC_2.2.5). I am happy to add remote but at least for now I think I don't need it. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode581 src/cmd/cgo/doc.go:581: reference to sin in foo.cgo2.o should be rewritten to refer to sin@GLIBC_2.2.5 On 2013/02/28 05:50:30, iant wrote: > s/sin@GLIBC_2.2.5/the symbol sin with version GLIBC_2.2.5/ Done. https://codereview.appspot.com/7433043/diff/2001/src/cmd/cgo/doc.go#newcode642 src/cmd/cgo/doc.go:642: Having 6l invoke the host linker will require support in the lib9 On 2013/02/28 05:50:30, iant wrote: > This is an interesting point but I'm not sure it belongs here. Done. https://codereview.appspot.com/7433043/diff/8001/src/cmd/cgo/doc.go File src/cmd/cgo/doc.go (right): https://codereview.appspot.com/7433043/diff/8001/src/cmd/cgo/doc.go#newcode416 src/cmd/cgo/doc.go:416: command) then invokes the host linker (usually gcc) to combine the On 2013/02/28 17:06:11, iant wrote: > Later it sounds like 6l, not the go command, is going to invoke the host linker. Indeed. This line is wrong. My thinking changed by the time I got to the end. https://codereview.appspot.com/7433043/diff/8001/src/cmd/cgo/doc.go#newcode519 src/cmd/cgo/doc.go:519: A package is compiled in "external-only" mode will only On 2013/02/28 17:06:11, iant wrote: > I think you mean something like "When cgo is run in external-only mode, it will > generate a package that only uses cgo_export, cgo_static_import, and cgo_ldflag > directives. Such a package can only be linked in external mode. When cgo is > run in internal+external mode, it will use all five kinds of directives, and the > linker will select the appropriate subset for the chosen linking mode." Yes, this is wrong, now that there's no "compilation mode". New text: A package compiled with cgo will include directives for both internal and external linking; the linker will select the appropriate subset for the chosen linking mode. https://codereview.appspot.com/7433043/diff/8001/src/cmd/cgo/doc.go#newcode528 src/cmd/cgo/doc.go:528: and suppose the package has been compiled in "internal+external" mode. On 2013/02/28 17:06:11, iant wrote: > s/the package has been compiled/cgo has been run/ Deleted this entire line.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=1a9cccfde340 *** cmd/cgo: extend implementation comment This is the plan for how to make host linking work with the rest of the system. There are two complications: 1. It is a goal to preserve the property that pure Go programs (even ones importing "net") can be compiled without needing gcc, so that a Go toolchain download works out of the box. This forces the support for two linking modes: with and without gcc. 2. It is a goal to allow users with old copies of SWIG to continue to use those copies. This forces the support for "internal only" packages. Perhaps it is reasonable to require a new SWIG. I don't know. R=iant CC=golang-dev https://codereview.appspot.com/7433043
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/7433043/diff/14001/src/cmd/cgo/doc.go File src/cmd/cgo/doc.go (right): https://codereview.appspot.com/7433043/diff/14001/src/cmd/cgo/doc.go#newcode480 src/cmd/cgo/doc.go:480: in the final binary. This directive is only needed from one i think allowing it to appear more than one could help diagnosing problems where runtime/cgo is compiled on another system with a different dynlinker than the current system. the linker will raise error for this case and so it won't generate a binary that is not runnable on the current system. (running a binary with wrong interpreter used to cause very confusing error messages on some systems) note that even on linux, the elf interpreter might be different for different distributions and/or versions (and that's why this pragma is introduced).
Sign in to reply to this message.
|
