From 696ed961d4a91d087200e23e028c6ff8ff35bcce Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Mon, 24 Nov 2014 20:18:44 -0500 Subject: go/build: build $GOOS_test.go always We decided to build $GOOS.go always but forgot to test $GOOS_test.go. Fixes issue 9159. LGTM=r R=r CC=golang-codereviews https://codereview.appspot.com/176290043 --- src/go/build/build.go | 8 +++++--- src/go/build/build_test.go | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/go/build/build.go b/src/go/build/build.go index 7a51cf3c0..311ecb01f 100644 --- a/src/go/build/build.go +++ b/src/go/build/build.go @@ -1310,11 +1310,13 @@ func (ctxt *Context) goodOSArchFile(name string, allTags map[string]bool) bool { // auto-tagging to apply only to files with a non-empty prefix, so // "foo_linux.go" is tagged but "linux.go" is not. This allows new operating // sytems, such as android, to arrive without breaking existing code with - // innocuous source code in "android.go". The easiest fix: files without - // underscores are always included. - if !strings.ContainsRune(name, '_') { + // innocuous source code in "android.go". The easiest fix: cut everything + // in the name before the initial _. + i := strings.Index(name, "_") + if i < 0 { return true } + name = name[i:] // ignore everything before first _ l := strings.Split(name, "_") if n := len(l); n > 0 && l[n-1] == "test" { diff --git a/src/go/build/build_test.go b/src/go/build/build_test.go index 43d09cbd1..a40def0fa 100644 --- a/src/go/build/build_test.go +++ b/src/go/build/build_test.go @@ -189,6 +189,7 @@ var matchFileTests = []struct { {ctxtAndroid, "foo_plan9.go", "", false}, {ctxtAndroid, "android.go", "", true}, {ctxtAndroid, "plan9.go", "", true}, + {ctxtAndroid, "plan9_test.go", "", true}, {ctxtAndroid, "arm.s", "", true}, {ctxtAndroid, "amd64.s", "", true}, } -- cgit v1.2.1 From 66be71687665f9511c060208dacf6e56adcd3bce Mon Sep 17 00:00:00 2001 From: Andrew Gerrand Date: Tue, 25 Nov 2014 15:41:33 +1100 Subject: doc: tidy up "Projects" page; add Go 1.4 LGTM=r R=r CC=golang-codereviews https://codereview.appspot.com/182750043 --- doc/contrib.html | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/doc/contrib.html b/doc/contrib.html index a615fc67a..8a674d647 100644 --- a/doc/contrib.html +++ b/doc/contrib.html @@ -30,21 +30,16 @@ We encourage all Go users to subscribe to

Version history

Release History

-

A summary of the changes between Go releases.

-

Go 1 Release Notes

-

-A guide for updating your code to work with Go 1. -

+

A summary of the changes between Go releases. Notes for the major releases:

-

Go 1.1 Release Notes

-

-A list of significant changes in Go 1.1, with instructions for updating -your code where necessary. -Each point release includes a similar document appropriate for that -release: Go 1.2, Go 1.3, -and so on. -

+

Go 1 and the Future of Go Programs

-- cgit v1.2.1 From ae637f5f9923cb1b798695597cdd0d0f4dea9464 Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Mon, 1 Dec 2014 07:52:09 -0800 Subject: reflect: Fix reflect.funcLayout. The GC bitmap has two bits per pointer, not one. Fixes issue 9179 LGTM=iant, rsc R=golang-codereviews, iant, rsc CC=golang-codereviews https://codereview.appspot.com/182160043 --- src/reflect/all_test.go | 101 +++++++++++++++++++++++++++++++++++++++++++++ src/reflect/export_test.go | 19 +++++++++ src/reflect/type.go | 4 +- 3 files changed, 122 insertions(+), 2 deletions(-) diff --git a/src/reflect/all_test.go b/src/reflect/all_test.go index 268a9e319..7a01c95d8 100644 --- a/src/reflect/all_test.go +++ b/src/reflect/all_test.go @@ -4055,3 +4055,104 @@ func TestLargeGCProg(t *testing.T) { fv := ValueOf(func([256]*byte) {}) fv.Call([]Value{ValueOf([256]*byte{})}) } + +// Issue 9179. +func TestCallGC(t *testing.T) { + f := func(a, b, c, d, e string) { + } + g := func(in []Value) []Value { + runtime.GC() + return nil + } + typ := ValueOf(f).Type() + f2 := MakeFunc(typ, g).Interface().(func(string, string, string, string, string)) + f2("four", "five5", "six666", "seven77", "eight888") +} + +type funcLayoutTest struct { + rcvr, t Type + argsize, retOffset uintptr + stack []byte +} + +var funcLayoutTests []funcLayoutTest + +func init() { + var argAlign = PtrSize + if runtime.GOARCH == "amd64p32" { + argAlign = 2 * PtrSize + } + roundup := func(x uintptr, a uintptr) uintptr { + return (x + a - 1) / a * a + } + + funcLayoutTests = append(funcLayoutTests, + funcLayoutTest{ + nil, + ValueOf(func(a, b string) string { return "" }).Type(), + 4 * PtrSize, + 4 * PtrSize, + []byte{BitsPointer, BitsScalar, BitsPointer}, + }) + + var r []byte + if PtrSize == 4 { + r = []byte{BitsScalar, BitsScalar, BitsScalar, BitsPointer} + } else { + r = []byte{BitsScalar, BitsScalar, BitsPointer} + } + funcLayoutTests = append(funcLayoutTests, + funcLayoutTest{ + nil, + ValueOf(func(a, b, c uint32, p *byte, d uint16) {}).Type(), + roundup(3*4, PtrSize) + PtrSize + 2, + roundup(roundup(3*4, PtrSize)+PtrSize+2, argAlign), + r, + }) + + funcLayoutTests = append(funcLayoutTests, + funcLayoutTest{ + nil, + ValueOf(func(a map[int]int, b uintptr, c interface{}) {}).Type(), + 4 * PtrSize, + 4 * PtrSize, + []byte{BitsPointer, BitsScalar, BitsPointer, BitsPointer}, + }) + + type S struct { + a, b uintptr + c, d *byte + } + funcLayoutTests = append(funcLayoutTests, + funcLayoutTest{ + nil, + ValueOf(func(a S) {}).Type(), + 4 * PtrSize, + 4 * PtrSize, + []byte{BitsScalar, BitsScalar, BitsPointer, BitsPointer}, + }) + + funcLayoutTests = append(funcLayoutTests, + funcLayoutTest{ + ValueOf((*byte)(nil)).Type(), + ValueOf(func(a uintptr, b *int) {}).Type(), + 3 * PtrSize, + roundup(3*PtrSize, argAlign), + []byte{BitsPointer, BitsScalar, BitsPointer}, + }) +} + +func TestFuncLayout(t *testing.T) { + for _, lt := range funcLayoutTests { + _, argsize, retOffset, stack := FuncLayout(lt.t, lt.rcvr) + if argsize != lt.argsize { + t.Errorf("funcLayout(%v, %v).argsize=%d, want %d", lt.t, lt.rcvr, argsize, lt.argsize) + } + if retOffset != lt.retOffset { + t.Errorf("funcLayout(%v, %v).retOffset=%d, want %d", lt.t, lt.rcvr, retOffset, lt.retOffset) + } + if !bytes.Equal(stack, lt.stack) { + t.Errorf("funcLayout(%v, %v).stack=%v, want %v", lt.t, lt.rcvr, stack, lt.stack) + } + } +} diff --git a/src/reflect/export_test.go b/src/reflect/export_test.go index 0778ad37f..caaf51a50 100644 --- a/src/reflect/export_test.go +++ b/src/reflect/export_test.go @@ -17,3 +17,22 @@ func IsRO(v Value) bool { var ArrayOf = arrayOf var CallGC = &callGC + +const PtrSize = ptrSize +const BitsPointer = bitsPointer +const BitsScalar = bitsScalar + +func FuncLayout(t Type, rcvr Type) (frametype Type, argSize, retOffset uintptr, stack []byte) { + var ft *rtype + var s *bitVector + if rcvr != nil { + ft, argSize, retOffset, s = funcLayout(t.(*rtype), rcvr.(*rtype)) + } else { + ft, argSize, retOffset, s = funcLayout(t.(*rtype), nil) + } + frametype = ft + for i := uint32(0); i < s.n; i += 2 { + stack = append(stack, s.data[i/8]>>(i%8)&3) + } + return +} diff --git a/src/reflect/type.go b/src/reflect/type.go index 572e611fa..c0ddfcad0 100644 --- a/src/reflect/type.go +++ b/src/reflect/type.go @@ -1894,14 +1894,14 @@ func addTypeBits(bv *bitVector, offset *uintptr, t *rtype) { switch Kind(t.kind & kindMask) { case Chan, Func, Map, Ptr, Slice, String, UnsafePointer: // 1 pointer at start of representation - for bv.n < uint32(*offset/uintptr(ptrSize)) { + for bv.n < 2*uint32(*offset/uintptr(ptrSize)) { bv.append2(bitsScalar) } bv.append2(bitsPointer) case Interface: // 2 pointers - for bv.n < uint32(*offset/uintptr(ptrSize)) { + for bv.n < 2*uint32(*offset/uintptr(ptrSize)) { bv.append2(bitsScalar) } bv.append2(bitsPointer) -- cgit v1.2.1 From 329c8eb00165253162ba8932acca5677aa519e0d Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Mon, 1 Dec 2014 16:32:06 -0500 Subject: runtime: fix hang in GC due to shrinkstack vs netpoll race During garbage collection, after scanning a stack, we think about shrinking it to reclaim some memory. The shrinking code (called while the world is stopped) checked that the status was Gwaiting or Grunnable and then changed the state to Gcopystack, to essentially lock the stack so that no other GC thread is scanning it. The same locking happens for stack growth (and is more necessary there). oldstatus = runtime?readgstatus(gp); oldstatus &= ~Gscan; if(oldstatus == Gwaiting || oldstatus == Grunnable) runtime?casgstatus(gp, oldstatus, Gcopystack); // oldstatus is Gwaiting or Grunnable else runtime?throw("copystack: bad status, not Gwaiting or Grunnable"); Unfortunately, "stop the world" doesn't stop everything. It stops all normal goroutine execution, but the network polling thread is still blocked in epoll and may wake up. If it does, and it chooses a goroutine to mark runnable, and that goroutine is the one whose stack is shrinking, then it can happen that between readgstatus and casgstatus, the status changes from Gwaiting to Grunnable. casgstatus assumes that if the status is not what is expected, it is a transient change (like from Gwaiting to Gscanwaiting and back, or like from Gwaiting to Gcopystack and back), and it loops until the status has been restored to the expected value. In this case, the status has changed semi-permanently from Gwaiting to Grunnable - it won't change again until the GC is done and the world can continue, but the GC is waiting for the status to change back. This wedges the program. To fix, call a special variant of casgstatus that accepts either Gwaiting or Grunnable as valid statuses. Without the fix bug with the extra check+throw in casgstatus, the program below dies in a few seconds (2-10) with GOMAXPROCS=8 on a 2012 Retina MacBook Pro. With the fix, it runs for minutes and minutes. package main import ( "io" "log" "net" "runtime" ) func main() { const N = 100 for i := 0; i < N; i++ { l, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { log.Fatal(err) } ch := make(chan net.Conn, 1) go func() { var err error c1, err := net.Dial("tcp", l.Addr().String()) if err != nil { log.Fatal(err) } ch <- c1 }() c2, err := l.Accept() if err != nil { log.Fatal(err) } c1 := <-ch l.Close() go netguy(c1, c2) go netguy(c2, c1) c1.Write(make([]byte, 100)) } for { runtime.GC() } } func netguy(r, w net.Conn) { buf := make([]byte, 100) for { bigstack(1000) _, err := io.ReadFull(r, buf) if err != nil { log.Fatal(err) } w.Write(buf) } } var g int func bigstack(n int) { var buf [100]byte if n > 0 { bigstack(n - 1) } g = int(buf[0]) + int(buf[99]) } Fixes issue 9186. LGTM=rlh R=austin, rlh CC=dvyukov, golang-codereviews, iant, khr, r https://codereview.appspot.com/179680043 --- src/runtime/proc.c | 32 ++++++++++++++++++++++++++++++++ src/runtime/runtime.h | 2 ++ src/runtime/stack.c | 7 +------ 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/runtime/proc.c b/src/runtime/proc.c index 91e3fe16d..8462c4b1d 100644 --- a/src/runtime/proc.c +++ b/src/runtime/proc.c @@ -402,6 +402,7 @@ runtime·castogscanstatus(G *gp, uint32 oldval, uint32 newval) static void badcasgstatus(void); static void helpcasgstatus(void); +static void badgstatusrunnable(void); // If asked to move to or from a Gscanstatus this will throw. Use the castogscanstatus // and casfromgscanstatus instead. @@ -423,6 +424,10 @@ runtime·casgstatus(G *gp, uint32 oldval, uint32 newval) // loop if gp->atomicstatus is in a scan state giving // GC time to finish and change the state to oldval. while(!runtime·cas(&gp->atomicstatus, oldval, newval)) { + if(oldval == Gwaiting && gp->atomicstatus == Grunnable) { + fn = badgstatusrunnable; + runtime·onM(&fn); + } // Help GC if needed. if(gp->preemptscan && !gp->gcworkdone && (oldval == Grunning || oldval == Gsyscall)) { gp->preemptscan = false; @@ -433,6 +438,33 @@ runtime·casgstatus(G *gp, uint32 oldval, uint32 newval) } } +static void +badgstatusrunnable(void) +{ + runtime·throw("casgstatus: waiting for Gwaiting but is Grunnable"); +} + +// casgstatus(gp, oldstatus, Gcopystack), assuming oldstatus is Gwaiting or Grunnable. +// Returns old status. Cannot call casgstatus directly, because we are racing with an +// async wakeup that might come in from netpoll. If we see Gwaiting from the readgstatus, +// it might have become Grunnable by the time we get to the cas. If we called casgstatus, +// it would loop waiting for the status to go back to Gwaiting, which it never will. +#pragma textflag NOSPLIT +uint32 +runtime·casgcopystack(G *gp) +{ + uint32 oldstatus; + + for(;;) { + oldstatus = runtime·readgstatus(gp) & ~Gscan; + if(oldstatus != Gwaiting && oldstatus != Grunnable) + runtime·throw("copystack: bad status, not Gwaiting or Grunnable"); + if(runtime·cas(&gp->atomicstatus, oldstatus, Gcopystack)) + break; + } + return oldstatus; +} + static void badcasgstatus(void) { diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h index 977c4547d..177a1287e 100644 --- a/src/runtime/runtime.h +++ b/src/runtime/runtime.h @@ -666,6 +666,8 @@ enum { uint32 runtime·readgstatus(G*); void runtime·casgstatus(G*, uint32, uint32); +void runtime·casgstatus(G*, uint32, uint32); +uint32 runtime·casgcopystack(G*); void runtime·quiesce(G*); bool runtime·stopg(G*); void runtime·restartg(G*); diff --git a/src/runtime/stack.c b/src/runtime/stack.c index 072bc242b..cb9557243 100644 --- a/src/runtime/stack.c +++ b/src/runtime/stack.c @@ -637,12 +637,7 @@ copystack(G *gp, uintptr newsize) } runtime·memmove((byte*)new.hi - used, (byte*)old.hi - used, used); - oldstatus = runtime·readgstatus(gp); - oldstatus &= ~Gscan; - if(oldstatus == Gwaiting || oldstatus == Grunnable) - runtime·casgstatus(gp, oldstatus, Gcopystack); // oldstatus is Gwaiting or Grunnable - else - runtime·throw("copystack: bad status, not Gwaiting or Grunnable"); + oldstatus = runtime·casgcopystack(gp); // cas from Gwaiting or Grunnable to Gcopystack, return old status // Swap out old stack for new one gp->stack = new; -- cgit v1.2.1 -- cgit v1.2.1 From 3b2bc49486d98ef1eb104dfbc2f82dd20f05f7b3 Mon Sep 17 00:00:00 2001 From: Dominik Honnef Date: Wed, 3 Dec 2014 10:28:54 +1100 Subject: cmd/go: regenerate doc.go Move change from CL 170770043 to correct file and regenerate docs for changes from CL 164120043. LGTM=adg R=golang-codereviews, adg, bradfitz CC=golang-codereviews https://codereview.appspot.com/183000043 Committer: Andrew Gerrand --- src/cmd/go/doc.go | 7 ++++++- src/cmd/go/generate.go | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/cmd/go/doc.go b/src/cmd/go/doc.go index 43a315944..879fc7f8b 100644 --- a/src/cmd/go/doc.go +++ b/src/cmd/go/doc.go @@ -317,7 +317,7 @@ Download and install packages and dependencies Usage: - go get [-d] [-fix] [-t] [-u] [build flags] [packages] + go get [-d] [-f] [-fix] [-t] [-u] [build flags] [packages] Get downloads and installs the packages named by the import paths, along with their dependencies. @@ -325,6 +325,11 @@ along with their dependencies. The -d flag instructs get to stop after downloading the packages; that is, it instructs get not to install the packages. +The -f flag, valid only when -u is set, forces get -u not to verify that +each package has been checked out from the source control repository +implied by its import path. This can be useful if the source is a local fork +of the original. + The -fix flag instructs get to run the fix tool on the downloaded packages before resolving dependencies or building the code. diff --git a/src/cmd/go/generate.go b/src/cmd/go/generate.go index a83cce8f7..2772452dd 100644 --- a/src/cmd/go/generate.go +++ b/src/cmd/go/generate.go @@ -45,7 +45,7 @@ The arguments are space-separated tokens or double-quoted strings passed to the generator as individual arguments when it is run. Quoted strings use Go syntax and are evaluated before execution; a -quoted string appears a single argument to the generator. +quoted string appears as a single argument to the generator. Go generate sets several variables when it runs the generator: -- cgit v1.2.1 From 18b416bab75c23aa867bee5a31075d78bb428502 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 3 Dec 2014 14:14:00 -0500 Subject: cmd/pprof: fix symbol resolution for remote profiles Fixes issue 9199. LGTM=iant R=golang-codereviews, iant CC=austin, golang-codereviews, minux https://codereview.appspot.com/183080043 --- src/cmd/pprof/internal/symbolizer/symbolizer.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/cmd/pprof/internal/symbolizer/symbolizer.go b/src/cmd/pprof/internal/symbolizer/symbolizer.go index cabddaa76..86de5640d 100644 --- a/src/cmd/pprof/internal/symbolizer/symbolizer.go +++ b/src/cmd/pprof/internal/symbolizer/symbolizer.go @@ -32,6 +32,10 @@ func Symbolize(mode string, prof *profile.Profile, obj plugin.ObjTool, ui plugin } } + if len(prof.Mapping) == 0 { + return fmt.Errorf("no known mappings") + } + mt, err := newMapping(prof, obj, ui, force) if err != nil { return err -- cgit v1.2.1 From 8f0f9d447fd3028e4f4cf890a577c00eaaacdc65 Mon Sep 17 00:00:00 2001 From: David Symonds Date: Thu, 4 Dec 2014 09:29:29 +1100 Subject: spec: add comment marker for consistency. LGTM=r R=gri, r CC=golang-codereviews https://codereview.appspot.com/185830043 --- doc/go_spec.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/go_spec.html b/doc/go_spec.html index ca0deb56a..3b67f307f 100644 --- a/doc/go_spec.html +++ b/doc/go_spec.html @@ -5579,7 +5579,7 @@ s3 := append(s2, s0...) // append a slice s3 == []int{0, s4 := append(s3[3:6], s3[2:]...) // append overlapping slice s4 == []int{3, 5, 7, 2, 3, 5, 7, 0, 0} var t []interface{} -t = append(t, 42, 3.1415, "foo") t == []interface{}{42, 3.1415, "foo"} +t = append(t, 42, 3.1415, "foo") // t == []interface{}{42, 3.1415, "foo"} var b []byte b = append(b, "bar"...) // append string contents b == []byte{'b', 'a', 'r' } -- cgit v1.2.1 From 00e1a8acf51edb67cd3abc678f4b0bb6b5fe7fd5 Mon Sep 17 00:00:00 2001 From: Rob Pike Date: Wed, 3 Dec 2014 20:07:48 -0800 Subject: lib/time: update to ICANN time zone database 2014j Fixes issue 9189. LGTM=dsymonds R=golang-codereviews, dsymonds CC=golang-codereviews https://codereview.appspot.com/178660043 --- lib/time/update.bash | 4 ++-- lib/time/zoneinfo.zip | Bin 358933 -> 360713 bytes 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/time/update.bash b/lib/time/update.bash index 8e1662afd..caa8450fa 100755 --- a/lib/time/update.bash +++ b/lib/time/update.bash @@ -7,8 +7,8 @@ # downloaded from the ICANN/IANA distribution. # Versions to use. -CODE=2014d -DATA=2014d +CODE=2014j +DATA=2014j set -e rm -rf work diff --git a/lib/time/zoneinfo.zip b/lib/time/zoneinfo.zip index e0d3afe07..425d7c98f 100644 Binary files a/lib/time/zoneinfo.zip and b/lib/time/zoneinfo.zip differ -- cgit v1.2.1 From 8208a8b2f9a29cabb4dff8aff17fbb739b6e8471 Mon Sep 17 00:00:00 2001 From: Shenghou Ma Date: Thu, 4 Dec 2014 11:24:23 -0500 Subject: cmd/pprof/internal/commands: add command to open browser on windows While we're at there, also add a message to prompt the user to install Graphviz if "dot" command is not found. Fixes issue 9178. LGTM=adg, alex.brainman, cookieo9, rsc R=rsc, adg, bradfitz, alex.brainman, cookieo9, smyrman CC=golang-codereviews https://codereview.appspot.com/180380043 Committer: Russ Cox --- src/cmd/pprof/internal/commands/commands.go | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/cmd/pprof/internal/commands/commands.go b/src/cmd/pprof/internal/commands/commands.go index 6d3582018..51397a3c6 100644 --- a/src/cmd/pprof/internal/commands/commands.go +++ b/src/cmd/pprof/internal/commands/commands.go @@ -11,6 +11,7 @@ import ( "io" "os" "os/exec" + "runtime" "strings" "cmd/pprof/internal/plugin" @@ -71,15 +72,27 @@ func PProf(c Completer, interactive **bool, svgpan **string) Commands { "eog": {c, report.Dot, invokeVisualizer(interactive, invokeDot("svg"), "svg", []string{"eog"}), false, "Visualize graph through eog"}, "evince": {c, report.Dot, invokeVisualizer(interactive, invokeDot("pdf"), "pdf", []string{"evince"}), false, "Visualize graph through evince"}, "gv": {c, report.Dot, invokeVisualizer(interactive, invokeDot("ps"), "ps", []string{"gv --noantialias"}), false, "Visualize graph through gv"}, - "web": {c, report.Dot, invokeVisualizer(interactive, saveSVGToFile(svgpan), "svg", browsers), false, "Visualize graph through web browser"}, + "web": {c, report.Dot, invokeVisualizer(interactive, saveSVGToFile(svgpan), "svg", browsers()), false, "Visualize graph through web browser"}, // Visualize HTML directly generated by report. - "weblist": {c, report.WebList, invokeVisualizer(interactive, awayFromTTY("html"), "html", browsers), true, "Output annotated source in HTML for functions matching regexp or address"}, + "weblist": {c, report.WebList, invokeVisualizer(interactive, awayFromTTY("html"), "html", browsers()), true, "Output annotated source in HTML for functions matching regexp or address"}, } } -// List of web browsers to attempt for web visualization -var browsers = []string{"chrome", "google-chrome", "firefox", "/usr/bin/open"} +// browsers returns a list of commands to attempt for web visualization +// on the current platform +func browsers() []string { + cmds := []string{"chrome", "google-chrome", "firefox"} + switch runtime.GOOS { + case "darwin": + cmds = append(cmds, "/usr/bin/open") + case "windows": + cmds = append(cmds, "cmd /c start") + default: + cmds = append(cmds, "xdg-open") + } + return cmds +} // NewCompleter creates an autocompletion function for a set of commands. func NewCompleter(cs Commands) Completer { @@ -142,6 +155,10 @@ func awayFromTTY(format string) PostProcessor { func invokeDot(format string) PostProcessor { divert := awayFromTTY(format) return func(input *bytes.Buffer, output io.Writer, ui plugin.UI) error { + if _, err := exec.LookPath("dot"); err != nil { + ui.PrintErr("Cannot find dot, have you installed Graphviz?") + return err + } cmd := exec.Command("dot", "-T"+format) var buf bytes.Buffer cmd.Stdin, cmd.Stdout, cmd.Stderr = input, &buf, os.Stderr @@ -174,6 +191,7 @@ func invokeVisualizer(interactive **bool, format PostProcessor, suffix string, v if err = format(input, tempFile, ui); err != nil { return err } + tempFile.Close() // on windows, if the file is Open, start cannot access it. // Try visualizers until one is successful for _, v := range visualizers { // Separate command and arguments for exec.Command. -- cgit v1.2.1 From 5223a2c9fbc6a2633a77586dbfec379e584c779e Mon Sep 17 00:00:00 2001 From: Rob Pike Date: Fri, 5 Dec 2014 09:15:38 +0900 Subject: cmd/go: avoid use of bufio.Scanner in generate Scanner can't handle stupid long lines and there are reports of stupid long lines in production. Note the issue isn't long "//go:generate" lines, but any long line in any Go source file. To be fair, if you're going to have a stupid long line it's not a bad bet you'll want to run it through go generate, because it's some embeddable asset that has been machine generated. (One could ask why that generation process didn't add a newline or two, but we should cope anyway.) Rewrite the file scanner in "go generate" so it can handle arbitrarily long lines, and only stores in memory those lines that start "//go:generate". Also: Adjust the documentation to make clear that it does not parse the file. Fixes issue 9143. Fixes issue 9196. LGTM=rsc, dominik.honnef R=rsc, cespare, minux, dominik.honnef CC=golang-codereviews https://codereview.appspot.com/182970043 --- src/cmd/go/doc.go | 21 ++++++++++----- src/cmd/go/generate.go | 71 +++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 70 insertions(+), 22 deletions(-) diff --git a/src/cmd/go/doc.go b/src/cmd/go/doc.go index 879fc7f8b..65640fb48 100644 --- a/src/cmd/go/doc.go +++ b/src/cmd/go/doc.go @@ -234,17 +234,24 @@ create or update Go source files, for instance by running yacc. Go generate is never run automatically by go build, go get, go test, and so on. It must be run explicitly. -Directives are written as a whole-line comment of the form +Go generate scans the file for directives, which are lines of +the form, //go:generate command argument... -(note: no space in "//go") where command is the generator to be -run, corresponding to an executable file that can be run locally. -It must either be in the shell path (gofmt), a fully qualified path -(/usr/you/bin/mytool), or a command alias, described below. +(note: no leading spaces and no space in "//go") where command +is the generator to be run, corresponding to an executable file +that can be run locally. It must either be in the shell path +(gofmt), a fully qualified path (/usr/you/bin/mytool), or a +command alias, described below. -The arguments are space-separated tokens or double-quoted strings -passed to the generator as individual arguments when it is run. +Note that go generate does not parse the file, so lines that look +like directives in comments or multiline strings will be treated +as directives. + +The arguments to the directive are space-separated tokens or +double-quoted strings passed to the generator as individual +arguments when it is run. Quoted strings use Go syntax and are evaluated before execution; a quoted string appears as a single argument to the generator. diff --git a/src/cmd/go/generate.go b/src/cmd/go/generate.go index 2772452dd..88f7efa0f 100644 --- a/src/cmd/go/generate.go +++ b/src/cmd/go/generate.go @@ -32,17 +32,24 @@ create or update Go source files, for instance by running yacc. Go generate is never run automatically by go build, go get, go test, and so on. It must be run explicitly. -Directives are written as a whole-line comment of the form +Go generate scans the file for directives, which are lines of +the form, //go:generate command argument... -(note: no space in "//go") where command is the generator to be -run, corresponding to an executable file that can be run locally. -It must either be in the shell path (gofmt), a fully qualified path -(/usr/you/bin/mytool), or a command alias, described below. +(note: no leading spaces and no space in "//go") where command +is the generator to be run, corresponding to an executable file +that can be run locally. It must either be in the shell path +(gofmt), a fully qualified path (/usr/you/bin/mytool), or a +command alias, described below. -The arguments are space-separated tokens or double-quoted strings -passed to the generator as individual arguments when it is run. +Note that go generate does not parse the file, so lines that look +like directives in comments or multiline strings will be treated +as directives. + +The arguments to the directive are space-separated tokens or +double-quoted strings passed to the generator as individual +arguments when it is run. Quoted strings use Go syntax and are evaluated before execution; a quoted string appears as a single argument to the generator. @@ -178,13 +185,43 @@ func (g *Generator) run() (ok bool) { fmt.Fprintf(os.Stderr, "%s\n", shortPath(g.path)) } - s := bufio.NewScanner(g.r) - for s.Scan() { - g.lineNum++ - if !bytes.HasPrefix(s.Bytes(), []byte("//go:generate ")) && !bytes.HasPrefix(s.Bytes(), []byte("//go:generate\t")) { + // Scan for lines that start "//go:generate". + // Can't use bufio.Scanner because it can't handle long lines, + // which are likely to appear when using generate. + input := bufio.NewReader(g.r) + var err error + // One line per loop. + for { + g.lineNum++ // 1-indexed. + var buf []byte + buf, err = input.ReadSlice('\n') + if err == bufio.ErrBufferFull { + // Line too long - consume and ignore. + if isGoGenerate(buf) { + g.errorf("directive too long") + } + for err == bufio.ErrBufferFull { + _, err = input.ReadSlice('\n') + } + if err != nil { + break + } + continue + } + + if err != nil { + // Check for marker at EOF without final \n. + if err == io.EOF && isGoGenerate(buf) { + err = io.ErrUnexpectedEOF + } + break + } + + if !isGoGenerate(buf) { continue } - words := g.split(s.Text()) + + words := g.split(string(buf)) if len(words) == 0 { g.errorf("no arguments to directive") } @@ -201,19 +238,23 @@ func (g *Generator) run() (ok bool) { } g.exec(words) } - if s.Err() != nil { - g.errorf("error reading %s: %s", shortPath(g.path), s.Err()) + if err != nil && err != io.EOF { + g.errorf("error reading %s: %s", shortPath(g.path), err) } return true } +func isGoGenerate(buf []byte) bool { + return bytes.HasPrefix(buf, []byte("//go:generate ")) || bytes.HasPrefix(buf, []byte("//go:generate\t")) +} + // split breaks the line into words, evaluating quoted // strings and evaluating environment variables. // The initial //go:generate element is dropped. func (g *Generator) split(line string) []string { // Parse line, obeying quoted strings. var words []string - line = line[len("//go:generate "):] + line = line[len("//go:generate ") : len(line)-1] // Drop preamble and final newline. // One (possibly quoted) word per iteration. Words: for { -- cgit v1.2.1 From 3ebebda3a7495402239db4369d59d73749c1bfa2 Mon Sep 17 00:00:00 2001 From: Rob Pike Date: Fri, 5 Dec 2014 09:37:56 +0900 Subject: cmd/go: fix build The new semantics of split require the newline be present. The test was stale. LGTM=adg R=golang-codereviews, adg CC=golang-codereviews https://codereview.appspot.com/182480043 --- src/cmd/go/generate.go | 2 +- src/cmd/go/generate_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cmd/go/generate.go b/src/cmd/go/generate.go index 88f7efa0f..baf4d2b55 100644 --- a/src/cmd/go/generate.go +++ b/src/cmd/go/generate.go @@ -250,7 +250,7 @@ func isGoGenerate(buf []byte) bool { // split breaks the line into words, evaluating quoted // strings and evaluating environment variables. -// The initial //go:generate element is dropped. +// The initial //go:generate element is present in line. func (g *Generator) split(line string) []string { // Parse line, obeying quoted strings. var words []string diff --git a/src/cmd/go/generate_test.go b/src/cmd/go/generate_test.go index 93c0ae66e..660ebabbe 100644 --- a/src/cmd/go/generate_test.go +++ b/src/cmd/go/generate_test.go @@ -40,7 +40,7 @@ func TestGenerateCommandParse(t *testing.T) { } g.setShorthand([]string{"-command", "yacc", "go", "tool", "yacc"}) for _, test := range splitTests { - got := g.split("//go:generate " + test.in) + got := g.split("//go:generate " + test.in + "\n") if !reflect.DeepEqual(got, test.out) { t.Errorf("split(%q): got %q expected %q", test.in, got, test.out) } -- cgit v1.2.1