Grokbase › Groups › Go › golang-dev › December 2012
FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello [email protected],

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/

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");

Search Discussions

  • Rémy Oudompheng at Dec 5, 2012 at 9:07 pm ⇧
    It's interesting but it looks really weird to me.

    Rémy.
  • Dave Cheney at Dec 5, 2012 at 9:27 pm ⇧
    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 wrote:

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

    Rémy.
  • Brad Fitzpatrick at Dec 5, 2012 at 9:36 pm ⇧
    Love the idea.
    On Dec 5, 2012 1:02 PM, wrote:

    Reviewers: golang-dev_googlegroups.com,

    Message:
    Hello [email protected],

    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");

  • Dave at Dec 5, 2012 at 10:19 pm ⇧

    On 2012/12/05 21:02:29, dfc wrote:
    Hello mailto:[email protected],
    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

    ...

    https://codereview.appspot.com/6875063/
  • Remyoudompheng at Dec 5, 2012 at 10:32 pm ⇧
    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.

    https://codereview.appspot.com/6875063/
  • Dave Cheney at Dec 5, 2012 at 10:36 pm ⇧

    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.
  • Rémy Oudompheng at Dec 5, 2012 at 10:47 pm ⇧

    ON 2012/12/5 Dave Cheney 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 at Dec 5, 2012 at 10:53 pm ⇧
    Hello [email protected], [email protected] (cc:
    [email protected]),

    I'd like you to review this change to
    https://code.google.com/p/go


    https://codereview.appspot.com/6875063/
  • Dave at Dec 5, 2012 at 10:55 pm ⇧

    On 2012/12/05 22:53:33, dfc wrote:
    Hello mailto:[email protected],
    mailto:[email protected] (cc:
    mailto:[email protected]),
    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.

    https://codereview.appspot.com/6875063/
  • Russ Cox at Dec 6, 2012 at 4:00 am ⇧
    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 at Dec 6, 2012 at 4:12 am ⇧
    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 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 at Dec 6, 2012 at 4:12 am ⇧
    *** Abandoned ***

    https://codereview.appspot.com/6875063/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 5, '12 at 9:02p
activeDec 6, '12 at 4:12a
posts13
users4
websitegolang.org

4 users in discussion

Dave: 8 posts Rémy Oudompheng: 3 posts Russ Cox: 1 post Brad Fitzpatrick: 1 post

Content

People

Support

Translate

site design / logo © 2023 Grokbase