code review 6875063: runtime: display go version and operating system inform...

runtime: display go version and operating system information in panic message

Fixes issue 4492.

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x400c03]
runtime version: devel +a32219a715c5 Wed Dec 05 15:26:18 2012 +1100
runtime host: linux/amd64

goroutine 1 [running]:
main.main()
	/home/dfc/src/panic.go:3 +0x3

goroutine 2 [syscall]:
created by runtime.main
	/home/dfc/go/src/pkg/runtime/proc.c:225
exit status 2

[dave_cheney.net, 2012-12-05 21:02:29]

Hello golang-dev@googlegroups.com,

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

[dave_cheney.net, 2012-12-05 22:19:16]

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

...

[remyoudompheng, 2012-12-05 22:32:20]

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

[dave_cheney.net, 2012-12-05 22:53:33]

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

[dave_cheney.net, 2012-12-05 22:55:20]

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.

[dave_cheney.net, 2012-12-06 04:12:36]

*** Abandoned ***

[dave_cheney.net, 2012-12-06 06:21:39]

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

[dave_cheney.net, 2012-12-06 06:34:18]

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.

[bradfitz, 2012-12-06 06:34:58]

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/6875...
>
> 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");
>
>
>

[rsc, 2012-12-06 06:43:24]

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

[remyoudompheng, 2012-12-06 06:43:45]

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.

[dave_cheney.net, 2012-12-06 06:45:01]

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

[remyoudompheng, 2012-12-06 07:06:13]

It's interesting but it looks really weird to me.

Rémy.