Code review - Issue 6875063: code review 6875063: runtime: display go version and operating system inform...https://codereview.appspot.com/2012-12-06T07:06:13+00:00rietveld
Message from unknown
2012-12-05T08:12:42+00:00dfcurn:md5:c1f418292478139693b4c4b96c06173f
Message from unknown
2012-12-05T08:20:09+00:00dfcurn:md5:a5fa2d3a01246a5a2dae33463cd688d9
Message from unknown
2012-12-05T21:02:19+00:00dfcurn:md5:e8e1305d12dcf0ceae2d7ae8554d5c4e
Message from dave@cheney.net
2012-12-05T21:02:29+00:00dfcurn:md5:12338d367159647dd32c9064fe809b65
Hello golang-dev@googlegroups.com,
I'd like you to review this change to
https://go.googlecode.com/hg/
Message from dave@cheney.net
2012-12-05T22:19:16+00:00dfcurn:md5:7d2dd9e923d46626145722c4dd8a9ac0
On 2012/12/05 21:02:29, dfc wrote:
> Hello mailto:golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
Counter proposal for the wording of the panic
throw: all goroutines are asleep - deadlock!
version: devel +a32219a715c5 Wed Dec 05 15:26:18 2012 +1100
host: linux/amd64
...
Message from remyoudompheng@gmail.com
2012-12-05T22:32:20+00:00remyoudomphengurn:md5:602e7eb449339a132c02d90c8ebbd582
https://codereview.appspot.com/6875063/diff/9001/src/pkg/runtime/extern.go
File src/pkg/runtime/extern.go (right):
https://codereview.appspot.com/6875063/diff/9001/src/pkg/runtime/extern.go#newcode144
src/pkg/runtime/extern.go:144: var panicvers = "Go runtime version: " + theVersion + " " + GOOS + "/" + GOARCH + "\n"
I wonder if panicvers should be a constant. Anyway, both const and var allow to override the string contents using the -X flag.
I am particularly interested in embedding my own libraries version into the message if this is going to happen. I already do that in other places where I display debugging information.
Message from unknown
2012-12-05T22:53:24+00:00dfcurn:md5:32fa7c24e35490b1a4c1419684fd1457
Message from dave@cheney.net
2012-12-05T22:53:33+00:00dfcurn:md5:48611ff467f9cef83a618aa72f346dae
Hello golang-dev@googlegroups.com, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com),
I'd like you to review this change to
https://code.google.com/p/go
Message from dave@cheney.net
2012-12-05T22:55:20+00:00dfcurn:md5:cf17d005af0acc6c8fcae80061bd234a
On 2012/12/05 22:53:33, dfc wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:remyoudompheng@gmail.com (cc:
> mailto:golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
Here is another suggestion for the panic message. I am not wedded to it, but I would like to see the runtime details as close to the top of the panic as possible to reduce the possibility of them being truncated. In other words, I don't want to see the runtime details moved below the goroutine dump.
Message from dave@cheney.net
2012-12-06T04:12:36+00:00dfcurn:md5:93274d70eb22bc72bcf938289eba07c0
*** Abandoned ***
Message from dave@cheney.net
2012-12-06T06:21:39+00:00dfcurn:md5:e7899c466041c2468d17ba33b6f437b4
Good point, it is easy to see the path from the current panic message
to the kitchen sink you get from the JDK.
On Thu, Dec 6, 2012 at 2:57 PM, Russ Cox <rsc@golang.org> wrote:
> This is a very slippery slope. Once you start printing a little bit
> about the binary you inevitably want to print more and more in these
> messages. Right now they only contain details about the *failure*, not
> details about the compilation, and I think that's probably a good
> thing. I understand wanting to remove the 'please run go version and
> go tool dist env' round trip on IRC, but I don't think it justifies
> editing the panic message.
>
> I suggest a compromise though: make 'go version' add goos/goarch at
> the end of the output, so that you'd have:
>
> go version devel +106824fa4abe Tue Nov 13 13:12:11 2012 -0500 darwin/amd64
>
> Then at least you only have to ask for one command's output.
>
> Russ
Message from dave@cheney.net
2012-12-06T06:34:18+00:00dfcurn:md5:64523682afd467585868c2e61e45cb3a
Please consider the output format to be a suggestion only. For me the idea of reporting the runtime version in the panic is the important part, the actual format is less important.
Also remember that most folks will have a much shorter version.
On 06/12/2012, at 8:07, Rémy Oudompheng <remyoudompheng@gmail.com> wrote:
> It's interesting but it looks really weird to me.
>
> Rémy.
Message from bradfitz@golang.org
2012-12-06T06:34:58+00:00bradfitzurn:md5:22b54d002cc555ee7e608361149e2695
Love the idea.
On Dec 5, 2012 1:02 PM, <dave@cheney.net> wrote:
> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> runtime: display go version and operating system information in panic
> message
>
> Fixes issue 4492.
>
> throw: all goroutines are asleep - deadlock!
> Go runtime version: devel +a32219a715c5 Wed Dec 05 15:26:18 2012 +1100
> linux/amd64
>
> goroutine 1 [select (no cases)]:
> main.main()
> /home/dfc/src/panic.go:3 +0x18
>
> goroutine 2 [syscall]:
> created by runtime.main
> /home/dfc/go/src/pkg/runtime/**proc.c:225
> exit status 2
>
> Please review this at https://codereview.appspot.**com/6875063/<https://codereview.appspot.com/6875063/>
>
> Affected files:
> M src/pkg/runtime/extern.go
> M src/pkg/runtime/panic.c
>
>
> Index: src/pkg/runtime/extern.go
> ==============================**==============================**=======
> --- a/src/pkg/runtime/extern.go
> +++ b/src/pkg/runtime/extern.go
> @@ -139,3 +139,6 @@
> // GOARCH is the running program's architecture target:
> // 386, amd64, or arm.
> const GOARCH string = theGoarch
> +
> +// panicvers is printed by panic
> +var panicvers = "Go runtime version: " + theVersion + " " + GOOS + "/" +
> GOARCH + "\n"
> Index: src/pkg/runtime/panic.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/panic.c
> +++ b/src/pkg/runtime/panic.c
> @@ -301,6 +301,8 @@
> runtime·lock(&paniclk);
> }
>
> +extern String runtime·panicvers; // defined in extern.go
> +
> void
> runtime·dopanic(int32 unused)
> {
> @@ -310,6 +312,7 @@
> runtime·printf("[signal %x code=%p addr=%p pc=%p]\n",
> g->sig, g->sigcode0, g->sigcode1, g->sigpc);
>
> + runtime·printstring(runtime·**panicvers);
> if(runtime·gotraceback()){
> if(g != m->g0) {
> runtime·printf("\n");
>
>
>
Message from rsc@golang.org
2012-12-06T06:43:24+00:00rscurn:md5:601a26c0992385165ba51344c494b80b
This is a very slippery slope. Once you start printing a little bit
about the binary you inevitably want to print more and more in these
messages. Right now they only contain details about the *failure*, not
details about the compilation, and I think that's probably a good
thing. I understand wanting to remove the 'please run go version and
go tool dist env' round trip on IRC, but I don't think it justifies
editing the panic message.
I suggest a compromise though: make 'go version' add goos/goarch at
the end of the output, so that you'd have:
go version devel +106824fa4abe Tue Nov 13 13:12:11 2012 -0500 darwin/amd64
Then at least you only have to ask for one command's output.
Russ
Message from remyoudompheng@gmail.com
2012-12-06T06:43:45+00:00remyoudomphengurn:md5:8da93077f4efb7728ef38a17f341a89f
ON 2012/12/5 Dave Cheney <dave@cheney.net> wrote:
>> theVersion + " " + GOOS + "/" + GOARCH + "\n"
>> I wonder if panicvers should be a constant. Anyway, both const and var
>> allow to override the string contents using the -X flag.
>
> The trouble is, if panicvers is a const, then it is a go constant, not
> a c constant, and is not visible to the linker. ie, i couldn't find a
> way to access runtime.theVersion from runtime.printstring, so this is
> why panicvers is a var. Can you show me how to do this ?
No you're right I was confused.
>> I am particularly interested in embedding my own libraries version into
>> the message if this is going to happen. I already do that in other
>> places where I display debugging information.
>
> I like it, but I don't want to paint all the sides of the bike shed in one go.
I didn't mean it to be part of any patch. It's not necessary since the
user can override it itself with linker flags.
Message from dave@cheney.net
2012-12-06T06:45:01+00:00dfcurn:md5:21b57c83aa8b8b295b8b190e75248037
> theVersion + " " + GOOS + "/" + GOARCH + "\n"
> I wonder if panicvers should be a constant. Anyway, both const and var
> allow to override the string contents using the -X flag.
The trouble is, if panicvers is a const, then it is a go constant, not
a c constant, and is not visible to the linker. ie, i couldn't find a
way to access runtime.theVersion from runtime.printstring, so this is
why panicvers is a var. Can you show me how to do this ?
> I am particularly interested in embedding my own libraries version into
> the message if this is going to happen. I already do that in other
> places where I display debugging information.
I like it, but I don't want to paint all the sides of the bike shed in one go.
Message from remyoudompheng@gmail.com
2012-12-06T07:06:13+00:00remyoudomphengurn:md5:2f3e60af6c289e8d3f12b1413cc6756e
It's interesting but it looks really weird to me.
Rémy.