cmd/compile: fix logopt log directory naming for windows

Allow Windows absolute paths, also fixed URI decoding on Windows.
Added a test, reorganized to make the test cleaner.
Also put some doc comments on exported functions that did not have them.

Fixes #41614.

Change-Id: I2871be0e5183fbd53ffb309896d6fe56c15a7727
Reviewed-on: https://go-review.googlesource.com/c/go/+/257677
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

2 files changed, 89 insertions, 28 deletions

diff --git a/src/cmd/compile/internal/logopt/log_opts.go b/src/cmd/compile/internal/logopt/log_opts.go
index 22a94b0f2d..37a049d640 100644
--- a/src/cmd/compile/internal/logopt/log_opts.go
+++ b/src/cmd/compile/internal/logopt/log_opts.go
@@ -19,6 +19,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"unicode"
 )
 
 // This implements (non)optimization logging for -json option to the Go compiler
@@ -223,11 +224,11 @@ type Diagnostic struct {
 // A LoggedOpt is what the compiler produces and accumulates,
 // to be converted to JSON for human or IDE consumption.
 type LoggedOpt struct {
-	pos    src.XPos      // Source code position at which the event occurred. If it is inlined, outer and all inlined locations will appear in JSON.
-	pass   string        // For human/adhoc consumption; does not appear in JSON (yet)
-	fname  string        // For human/adhoc consumption; does not appear in JSON (yet)
-	what   string        // The (non) optimization; "nilcheck", "boundsCheck", "inline", "noInline"
-	target []interface{} // Optional target(s) or parameter(s) of "what" -- what was inlined, why it was not, size of copy, etc. 1st is most important/relevant.
+	pos          src.XPos      // Source code position at which the event occurred. If it is inlined, outer and all inlined locations will appear in JSON.
+	compilerPass string        // Compiler pass.  For human/adhoc consumption; does not appear in JSON (yet)
+	functionName string        // Function name.  For human/adhoc consumption; does not appear in JSON (yet)
+	what         string        // The (non) optimization; "nilcheck", "boundsCheck", "inline", "noInline"
+	target       []interface{} // Optional target(s) or parameter(s) of "what" -- what was inlined, why it was not, size of copy, etc. 1st is most important/relevant.
 }
 
 type logFormat uint8
@@ -240,12 +241,13 @@ const (
 var Format = None
 var dest string
 
+// LogJsonOption parses and validates the version,directory value attached to the -json compiler flag.
 func LogJsonOption(flagValue string) {
 	version, directory := parseLogFlag("json", flagValue)
 	if version != 0 {
 		log.Fatal("-json version must be 0")
 	}
-	checkLogPath("json", directory)
+	dest = checkLogPath(directory)
 	Format = Json0
 }
 
@@ -268,51 +270,80 @@ func parseLogFlag(flag, value string) (version int, directory string) {
 	return
 }
 
-// checkLogPath does superficial early checking of the string specifying
-// the directory to which optimizer logging is directed, and if
-// it passes the test, stores the string in LO_dir
-func checkLogPath(flag, destination string) {
-	sep := string(os.PathSeparator)
-	if strings.HasPrefix(destination, "/") || strings.HasPrefix(destination, sep) {
-		err := os.MkdirAll(destination, 0755)
-		if err != nil {
-			log.Fatalf("optimizer logging destination '<version>,<directory>' but could not create <directory>: err=%v", err)
-		}
-	} else if strings.HasPrefix(destination, "file://") { // IKWIAD, or Windows C:\foo\bar\baz
+// isWindowsDriveURI returns true if the file URI is of the format used by
+// Windows URIs. The url.Parse package does not specially handle Windows paths
+// (see golang/go#6027), so we check if the URI path has a drive prefix (e.g. "/C:").
+// (copied from tools/internal/span/uri.go)
+// this is less comprehensive that the processing in filepath.IsAbs on Windows.
+func isWindowsDriveURIPath(uri string) bool {
+	if len(uri) < 4 {
+		return false
+	}
+	return uri[0] == '/' && unicode.IsLetter(rune(uri[1])) && uri[2] == ':'
+}
+
+func parseLogPath(destination string) (string, string) {
+	if filepath.IsAbs(destination) {
+		return filepath.Clean(destination), ""
+	}
+	if strings.HasPrefix(destination, "file://") { // IKWIAD, or Windows C:\foo\bar\baz
 		uri, err := url.Parse(destination)
 		if err != nil {
-			log.Fatalf("optimizer logging destination looked like file:// URI but failed to parse: err=%v", err)
+			return "", fmt.Sprintf("optimizer logging destination looked like file:// URI but failed to parse: err=%v", err)
 		}
 		destination = uri.Host + uri.Path
-		err = os.MkdirAll(destination, 0755)
-		if err != nil {
-			log.Fatalf("optimizer logging destination '<version>,<directory>' but could not create %s: err=%v", destination, err)
+		if isWindowsDriveURIPath(destination) {
+			// strip leading / from /C:
+			// unlike tools/internal/span/uri.go, do not uppercase the drive letter -- let filepath.Clean do what it does.
+			destination = destination[1:]
 		}
-	} else {
-		log.Fatalf("optimizer logging destination %s was neither %s-prefixed directory nor file://-prefixed file URI", destination, sep)
+		return filepath.Clean(destination), ""
+	}
+	return "", fmt.Sprintf("optimizer logging destination %s was neither %s-prefixed directory nor file://-prefixed file URI", destination, string(filepath.Separator))
+}
+
+// checkLogPath does superficial early checking of the string specifying
+// the directory to which optimizer logging is directed, and if
+// it passes the test, stores the string in LO_dir
+func checkLogPath(destination string) string {
+	path, complaint := parseLogPath(destination)
+	if complaint != "" {
+		log.Fatalf(complaint)
+	}
+	err := os.MkdirAll(path, 0755)
+	if err != nil {
+		log.Fatalf("optimizer logging destination '<version>,<directory>' but could not create <directory>: err=%v", err)
 	}
-	dest = destination
+	return path
 }
 
 var loggedOpts []*LoggedOpt
 var mu = sync.Mutex{} // mu protects loggedOpts.
 
-func NewLoggedOpt(pos src.XPos, what, pass, fname string, args ...interface{}) *LoggedOpt {
+// NewLoggedOpt allocates a new LoggedOpt, to later be passed to either NewLoggedOpt or LogOpt as "args".
+// Pos is the source position (including inlining), what is the message, pass is which pass created the message,
+// funcName is the name of the function
+// A typical use for this to accumulate an explanation for a missed optimization, for example, why did something escape?
+func NewLoggedOpt(pos src.XPos, what, pass, funcName string, args ...interface{}) *LoggedOpt {
 	pass = strings.Replace(pass, " ", "_", -1)
-	return &LoggedOpt{pos, pass, fname, what, args}
+	return &LoggedOpt{pos, pass, funcName, what, args}
 }
 
-func LogOpt(pos src.XPos, what, pass, fname string, args ...interface{}) {
+// Logopt logs information about a (usually missed) optimization performed by the compiler.
+// Pos is the source position (including inlining), what is the message, pass is which pass created the message,
+// funcName is the name of the function
+func LogOpt(pos src.XPos, what, pass, funcName string, args ...interface{}) {
 	if Format == None {
 		return
 	}
-	lo := NewLoggedOpt(pos, what, pass, fname, args...)
+	lo := NewLoggedOpt(pos, what, pass, funcName, args...)
 	mu.Lock()
 	defer mu.Unlock()
 	// Because of concurrent calls from back end, no telling what the order will be, but is stable-sorted by outer Pos before use.
 	loggedOpts = append(loggedOpts, lo)
 }
 
+// Enabled returns whether optimization logging is enabled.
 func Enabled() bool {
 	switch Format {
 	case None:
@@ -459,11 +490,13 @@ func FlushLoggedOpts(ctxt *obj.Link, slashPkgPath string) {
 	}
 }
 
+// newPointRange returns a single-position Range for the compiler source location p.
 func newPointRange(p src.Pos) Range {
 	return Range{Start: Position{p.Line(), p.Col()},
 		End: Position{p.Line(), p.Col()}}
 }
 
+// newLocation returns the Location for the compiler source location p
 func newLocation(p src.Pos) Location {
 	loc := Location{URI: uriIfy(uprootedPath(p.Filename())), Range: newPointRange(p)}
 	return loc
diff --git a/src/cmd/compile/internal/logopt/logopt_test.go b/src/cmd/compile/internal/logopt/logopt_test.go
index df3e70a614..b57a07f12c 100644
--- a/src/cmd/compile/internal/logopt/logopt_test.go
+++ b/src/cmd/compile/internal/logopt/logopt_test.go
@@ -55,6 +55,34 @@ func wantN(t *testing.T, out string, desired string, n int) {
 	}
 }
 
+func TestPathStuff(t *testing.T) {
+	sep := string(filepath.Separator)
+	if path, whine := parseLogPath("file:///c:foo"); path != "c:foo" || whine != "" { // good path
+		t.Errorf("path='%s', whine='%s'", path, whine)
+	}
+	if path, whine := parseLogPath("file:///foo"); path != sep+"foo" || whine != "" { // good path
+		t.Errorf("path='%s', whine='%s'", path, whine)
+	}
+	if path, whine := parseLogPath("foo"); path != "" || whine == "" { // BAD path
+		t.Errorf("path='%s', whine='%s'", path, whine)
+	}
+	if sep == "\\" { // On WINDOWS ONLY
+		if path, whine := parseLogPath("C:/foo"); path != "C:\\foo" || whine != "" { // good path
+			t.Errorf("path='%s', whine='%s'", path, whine)
+		}
+		if path, whine := parseLogPath("c:foo"); path != "" || whine == "" { // BAD path
+			t.Errorf("path='%s', whine='%s'", path, whine)
+		}
+		if path, whine := parseLogPath("/foo"); path != "" || whine == "" { // BAD path
+			t.Errorf("path='%s', whine='%s'", path, whine)
+		}
+	} else { // ON UNIX ONLY
+		if path, whine := parseLogPath("/foo"); path != sep+"foo" || whine != "" { // good path
+			t.Errorf("path='%s', whine='%s'", path, whine)
+		}
+	}
+}
+
 func TestLogOpt(t *testing.T) {
 	t.Parallel()