[go] runtime: inform windows of encoding before writing to console.

[Daniel Theophanes (Gerrit)]

Daniel Theophanes uploaded a change:
https://go-review.googlesource.com/16714

runtime: inform windows of encoding before writing to console.

The Windows console is usually set to a non-unicode encoding.
Inform windows of the proper encoding about to be written.

Fixes #7864

Change-Id: Id13369352aeccac8387876f0b911e383c543c28e
---
M src/runtime/os1_windows.go
1 file changed, 41 insertions(+), 1 deletion(-)



diff --git a/src/runtime/os1_windows.go b/src/runtime/os1_windows.go
index 99c6df4..047dfff 100644
--- a/src/runtime/os1_windows.go
+++ b/src/runtime/os1_windows.go
@@ -25,6 +25,9 @@
//go:cgo_import_dynamic runtime._GetProcessAffinityMask
GetProcessAffinityMask%3 "kernel32.dll"
//go:cgo_import_dynamic runtime._GetQueuedCompletionStatus
GetQueuedCompletionStatus%5 "kernel32.dll"
//go:cgo_import_dynamic runtime._GetStdHandle GetStdHandle%1 "kernel32.dll"
+//go:cgo_import_dynamic runtime._GetConsoleMode
GetConsoleMode%2 "kernel32.dll"
+//go:cgo_import_dynamic runtime._GetConsoleOutputCP
GetConsoleOutputCP%0 "kernel32.dll"
+//go:cgo_import_dynamic runtime._SetConsoleOutputCP
SetConsoleOutputCP%1 "kernel32.dll"
//go:cgo_import_dynamic runtime._GetSystemInfo
GetSystemInfo%1 "kernel32.dll"
//go:cgo_import_dynamic runtime._GetThreadContext
GetThreadContext%2 "kernel32.dll"
//go:cgo_import_dynamic runtime._LoadLibraryW LoadLibraryW%1 "kernel32.dll"
@@ -67,6 +70,9 @@
_GetProcessAffinityMask,
_GetQueuedCompletionStatus,
_GetStdHandle,
+ _GetConsoleMode,
+ _GetConsoleOutputCP,
+ _SetConsoleOutputCP,
_GetSystemInfo,
_GetThreadContext,
_LoadLibraryW,
@@ -238,7 +244,7 @@
}

//go:nosplit
-func write(fd uintptr, buf unsafe.Pointer, n int32) int32 {
+func writeSimple(fd uintptr, buf unsafe.Pointer, n int32) int32 {
const (
_STD_OUTPUT_HANDLE = ^uintptr(10) // -11
_STD_ERROR_HANDLE = ^uintptr(11) // -12
@@ -258,6 +264,40 @@
return int32(written)
}

+func write(fd uintptr, buf unsafe.Pointer, n int32) int32 {
+ const (
+ _STD_OUTPUT_HANDLE = ^uintptr(10) // -11
+ _STD_ERROR_HANDLE = ^uintptr(11) // -12
+ )
+ var handle uintptr
+ switch fd {
+ case 1:
+ handle = stdcall1(_GetStdHandle, _STD_OUTPUT_HANDLE)
+ case 2:
+ handle = stdcall1(_GetStdHandle, _STD_ERROR_HANDLE)
+ default:
+ // assume fd is real windows handle.
+ handle = fd
+ }
+
+ // Detect if this is attached to a console or not.
+ var m uint32
+ isConsole := stdcall2(_GetConsoleMode, handle,
uintptr(unsafe.Pointer(&m))) != 0
+ // If this is a console output, various non-unicode code pages can be in
use.
+ // Rather then try to detect and convert here, let Windows do the work.
+ var origCP uintptr
+ if isConsole {
+ origCP = stdcall(_GetConsoleOutputCP)
+ stdcall1(_SetConsoleOutputCP, 65001) // UTF-8 code page.
+ }
+ var written uint32
+ stdcall5(_WriteFile, handle, uintptr(buf), uintptr(n),
uintptr(unsafe.Pointer(&written)), 0)
+ if isConsole {
+ stdcall1(_SetConsoleOutputCP, origCP)
+ }
+ return int32(written)
+}
+
//go:nosplit
func semasleep(ns int64) int32 {
// store ms in ns to save stack space

--
https://go-review.googlesource.com/16714

[Daniel Theophanes (Gerrit)]

Daniel Theophanes uploaded a new patch set:

https://go-review.googlesource.com/16714

runtime: inform windows of encoding before writing to console.

The Windows console is usually set to a non-unicode encoding.
Inform windows of the proper encoding about to be written.

Fixes #7864

Change-Id: Id13369352aeccac8387876f0b911e383c543c28e
---
M src/runtime/os1_windows.go

M src/runtime/proc.go
2 files changed, 42 insertions(+), 2 deletions(-)


--
https://go-review.googlesource.com/16714

[Daniel Theophanes (Gerrit)]

Daniel Theophanes uploaded a new patch set:

https://go-review.googlesource.com/16714

runtime: inform windows of encoding before writing to console.

The Windows console is usually set to a non-unicode encoding.
Inform windows of the proper encoding about to be written.

This adds at one new syscall for every print to without a
console and four new syscalls for every print to with an
attached console. The Windows console is extreemly slow so
the extra syscalls won't be noticable. It may be possible to
cache the detection of the console but I'm unsure if that can
be done safely.

Fixes #7864

Change-Id: Id13369352aeccac8387876f0b911e383c543c28e
---
M src/runtime/os1_windows.go

[Alex Brainman (Gerrit)]

Alex Brainman has posted comments on this change.

runtime: inform windows of encoding before writing to console.

Patch Set 3:

I really don't see how your change fixes #7864. This

https://gist.github.com/alexbrainman/964a1c25de03b67a4925#file-cl16714-jpg

is what I see after I've applied your CL here. I see the same outupt
without your change. How does your change improve the situation?

Alex

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-HasComments: No

[Daniel Theophanes (Gerrit)]

Daniel Theophanes has posted comments on this change.

runtime: inform windows of encoding before writing to console.

Patch Set 3:

Hi Alex. "It works on my box" I've commented on your gist here specifying
why my win7 x64 box outputs:
https://gist.github.com/alexbrainman/964a1c25de03b67a4925#gistcomment-1616194
If you don't mind horribly, could you instrument SetConsoleOutputCP to see
if it errors and call GetLastError?

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>

Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>
Gerrit-HasComments: No

[Alex Brainman (Gerrit)]

Alex Brainman has posted comments on this change.

runtime: inform windows of encoding before writing to console.

Patch Set 3:

I have changed your CL:

diff --git a/src/runtime/os1_windows.go b/src/runtime/os1_windows.go
index 047dfff..fb01b28 100644
--- a/src/runtime/os1_windows.go
+++ b/src/runtime/os1_windows.go
@@ -264,6 +264,28 @@ func writeSimple(fd uintptr, buf unsafe.Pointer, n
int32) int32 {
return int32(written)
}

+func winprint(handle uintptr, buf []byte) {
+ var written uint32
+ stdcall5(_WriteFile, handle, uintptr(unsafe.Pointer(&buf[0])),
uintptr(len(buf)), uintptr(unsafe.Pointer(&written)), 0)
+}
+
+func winprintstring(handle uintptr, str string) {
+ winprint(handle, []byte(str))
+}
+
+func winprintuint(handle uintptr, v uint64) {
+ var buf [100]byte
+ i := len(buf)
+ for i--; i > 0; i-- {
+ buf[i] = byte(v%10 + '0')
+ if v < 10 {
+ break
+ }
+ v /= 10
+ }
+ winprint(handle, buf[i:])
+}
+

func write(fd uintptr, buf unsafe.Pointer, n int32) int32 {

const (
_STD_OUTPUT_HANDLE = ^uintptr(10) // -11

@@ -288,12 +310,18 @@ func write(fd uintptr, buf unsafe.Pointer, n int32)
int32 {
var origCP uintptr
if isConsole {
origCP = stdcall(_GetConsoleOutputCP)
- stdcall1(_SetConsoleOutputCP, 65001) // UTF-8 code page.
+ ret := stdcall1(_SetConsoleOutputCP, 65001) // UTF-8 code page.
+ winprintstring(handle, "\nSetConsoleOutputCP(65001): ")
+ winprintuint(handle, uint64(ret))
+ winprintstring(handle, "\n")
}
var written uint32

stdcall5(_WriteFile, handle, uintptr(buf), uintptr(n),
uintptr(unsafe.Pointer(&written)), 0)

if isConsole {
- stdcall1(_SetConsoleOutputCP, origCP)
+ ret := stdcall1(_SetConsoleOutputCP, origCP)
+ winprintstring(handle, "\nSetConsoleOutputCP(origCP): ")
+ winprintuint(handle, uint64(ret))
+ winprintstring(handle, "\n")
}
return int32(written)
}

and my program now prints
```
c:\dev\src\t>chcp
Active code page: 850

c:\dev\src\t>chcp 437
Active code page: 437

c:\dev\src\t>go install -v runtime && go run main.go

SetConsoleOutputCP(65001): 1
Hello, друг
SetConsoleOutputCP(origCP): 1

SetConsoleOutputCP(65001): 1


SetConsoleOutputCP(origCP): 1

c:\dev\src\t>
```

So the output still the same. SetConsoleOutputCP succeeds. I also changed
my code page to match yours. Perhaps it is font you use in your console
window.

Alex

[Daniel Theophanes (Gerrit)]

Daniel Theophanes has posted comments on this change.

runtime: inform windows of encoding before writing to console.

Patch Set 3:

Thanks Alex. I'll test this on a vista box today. The font used is "Lucida
Console".

[Daniel Theophanes (Gerrit)]

Daniel Theophanes has posted comments on this change.

runtime: inform windows of encoding before writing to console.

Patch Set 3:

Alex, If my console is set to "Raster Fonts" it doesn't "fix" it. But if it
is set to "Lucida Console" it does "fix" it.

However, it looks like the correct thing to do is to call WriteConsole if
attached. I'll upload a revision with that change.

[Daniel Theophanes (Gerrit)]

Daniel Theophanes uploaded a new patch set:

https://go-review.googlesource.com/16714

runtime: inform windows of encoding before writing to console.

Use WriteConsoleW to write to the console. This requires
convertint to utf16 so bundle the utf8 and uft16 packages.

Fixes #7864

Change-Id: Id13369352aeccac8387876f0b911e383c543c28e
---
M src/runtime/os1_windows.go

M src/runtime/proc.go
A src/runtime/utf16_windows.go
A src/runtime/utf8_windows.go
4 files changed, 598 insertions(+), 5 deletions(-)

[Russ Cox (Gerrit)]

Russ Cox has posted comments on this change.

runtime: inform windows of encoding before writing to console.

Patch Set 4:

Thanks very much for working on this. Let's find a way to do it without
bundling the entire package utf16 and utf8 into the runtime. In particular,
there is already a utf8 decoder in the runtime; we should not need a
second. The utf16 code may be unavoidable, but even there I think it would
be reasonable to use bundle once and delete the code you don't need. That
code will literally never change.

The utf8 dependency can be removed by using runes := []rune(b). This would
include any final partial rune at the end of b, but since write cannot
return a partial result without being treated as an error, this is probably
okay. In practice the runtime should not be sending partial utf8 bytes to
begin with.

But more importantly, I do not think we want print allocating memory at
all. For example, when malloc is broken it needs to print errors. It can't
do that if print mallocs. So while you could avoid the utf8 dependency,
there would still be two allocation+copy loops here: one for the utf8->rune
conversion and one for the rune->utf16 conversion.

I think that instead we want a smallish buffer, say [1000]uint16, with a
lock around it, and writeConsole would acquire the lock, use the buffer,
release the lock. Locks during printing are a bit worrisome too, but it's
at least better than allocating. You can loop over the input one rune at a
time by calling charntorune, and when the 1000-uint16 buffer has fewer than
2 elements left, flush it. To convert from rune to utf16 you only need
utf16.EncodeRune, which is two lines and not worth importing. The relevant
constants are already defined in the runtime.

I think the entire conversion might boil down to something without any
allocations nor package bundling, like:

const surr2 = (surrogateMin+surrogateMax+1)/2

w := 0
for len(b) > 0 {
if w >= cap(utf16tmp) - 2 {
writeConsoleUTF16(utf16tmp[:w])
w = 0
}
r, n := charntorune(b)
b = b[n:]
if r < 0x10000 {
utf16tmp = utf16tmp[:w+1]
utf16tmp[w] = uint16(r)
} else {
utf16tmp = utf16tmp[:w+2]
utf16tmp[w] = surrogateMin + uint16(r>>10)&0x3ff
utf16tmp[w+1] = surr2 + uint16(r)&0x3ff
}
}
writeConsoleUTF16(utf16tmp[:w])

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>

Gerrit-Reviewer: Russ Cox <r...@golang.org>
Gerrit-HasComments: No

[Russ Cox (Gerrit)]

Russ Cox has posted comments on this change.

runtime: inform windows of encoding before writing to console.

Patch Set 4:

> if r < 0x10000 {
> ...
> } else {

The first line of the else block is missing and should say "r -= 0x10000".

[Alex Brainman (Gerrit)]

Alex Brainman has posted comments on this change.

runtime: inform windows of encoding before writing to console.

Patch Set 4:

(2 comments)

I am just wandering if all this extra complexity is worth it.

Alex

https://go-review.googlesource.com/#/c/16714/4//COMMIT_MSG
Commit Message:

Line 7: runtime: inform windows of encoding before writing to console.
s/inform windows of encoding before writing to console./use WriteConsole to
implement print and panic on windows/

or similar.


Line 9: Use WriteConsoleW to write to the console. This requires
You won't need these, if you update the subject line.

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: Yes

[Russ Cox (Gerrit)]

Russ Cox has posted comments on this change.

runtime: inform windows of encoding before writing to console.

Patch Set 4:

> I am just wandering if all this extra complexity is worth it.

FWIW, I believe it is. I really think that

package main

func main() {
println("hello, 世界")
}

needs to work. It's unfortunate that it hasn't to date.

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: No

[Daniel Theophanes (Gerrit)]

Daniel Theophanes uploaded a new patch set:
https://go-review.googlesource.com/16714

runtime: use WriteConsole to implement print and panic on windows

Fixes #7864

Change-Id: Id13369352aeccac8387876f0b911e383c543c28e
---
M src/runtime/os1_windows.go

M src/runtime/proc.go
2 files changed, 103 insertions(+), 5 deletions(-)

[Russ Cox (Gerrit)]

Russ Cox has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 5:

(6 comments)

Getting close, thanks.

https://go-review.googlesource.com/#/c/16714/5/src/runtime/os1_windows.go
File src/runtime/os1_windows.go:

Line 313: if w >= cap(utf16tmp)-2 {
Can use len here.


Line 318: if n <= 0 || r == bad {
Delete this if statement.

We can't just stop printing a string because of one bad rune. The point of
the replacement rune is that it's a sensible thing to print for an encoding
error. The loop must continue and process the whole buffer. We know len(s)
> 0, so charntorune will return n >= 1.


Line 324: utf16tmp = utf16tmp[:w+1]
The code is reslicing utf16tmp on the arguments to the writeConsoleUTF16
calls anyway, so this reslicing is redundant. (My fault.) Delete.


Line 326: w += 1
w++


Line 329: utf16tmp = utf16tmp[:w+2]
Delete.


https://go-review.googlesource.com/#/c/16714/5/src/runtime/proc.go
File src/runtime/proc.go:

Line 1255: writeSimple(2, unsafe.Pointer(&earlycgocallback[0]),
int32(len(earlycgocallback)))
writeSimple is not defined on non-Wndows systems, and I'd rather it wasn't.
This needs to be just 'write'. What are the implications of that for
Windows? I don't understand why it's changing anyway.

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: Yes

[Daniel Theophanes (Gerrit)]

Daniel Theophanes has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 5:

(6 comments)

Sorry for not cleaning it up first. Outstanding question on nosplit. Once
answered I'll mail the change.

https://go-review.googlesource.com/#/c/16714/5/src/runtime/os1_windows.go
File src/runtime/os1_windows.go:

Line 313: if w >= cap(utf16tmp)-2 {
> Can use len here.

Done

Line 318: if n <= 0 || r == bad {
> Delete this if statement.

Done

Line 324: utf16tmp = utf16tmp[:w+1]
> The code is reslicing utf16tmp on the arguments to the writeConsoleUTF16
> ca

Done

Line 326: w += 1
> w++

Done

Line 329: utf16tmp = utf16tmp[:w+2]
> Delete.

Done

https://go-review.googlesource.com/#/c/16714/5/src/runtime/proc.go
File src/runtime/proc.go:

Line 1255: writeSimple(2, unsafe.Pointer(&earlycgocallback[0]),
int32(len(earlycgocallback)))
> writeSimple is not defined on non-Wndows systems, and I'd rather it
> wasn't.

write is decorated with //go:nosplit. The only calling function that was
also //go:nosplit is here. I thought that they had to be nosplit in all
calling functions, but I'm probably wrong.

I'm unsure if I should decorate all of the functions as nosplit or even the
precise implication of it.

[Daniel Theophanes (Gerrit)]

Daniel Theophanes uploaded a new patch set:
https://go-review.googlesource.com/16714

runtime: use WriteConsole to implement print and panic on windows

Fixes #7864

Change-Id: Id13369352aeccac8387876f0b911e383c543c28e
---
M src/runtime/os1_windows.go

1 file changed, 77 insertions(+), 0 deletions(-)

[Minux Ma (Gerrit)]

Minux Ma has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 5:

(1 comment)

https://go-review.googlesource.com/#/c/16714/5/src/runtime/proc.go
File src/runtime/proc.go:

Line 1255: writeSimple(2, unsafe.Pointer(&earlycgocallback[0]),
int32(len(earlycgocallback)))

> write is decorated with //go:nosplit. The only calling function that was
> al

The restriction on //go:nosplit functions is that
they can't call non-go:nosplit functions or use
too much stack space. Other functions are free to
call them.

The linker will check the property for you.

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>

Gerrit-Reviewer: Minux Ma <mi...@golang.org>

[Daniel Theophanes (Gerrit)]

Daniel Theophanes has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 6:

> (1 comment)

Thanks minux. In that case this is ready now.
PTAL

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>
Gerrit-Reviewer: Minux Ma <mi...@golang.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: No

[Russ Cox (Gerrit)]

Russ Cox has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 6:

(3 comments)

https://go-review.googlesource.com/#/c/16714/6/src/runtime/os1_windows.go
File src/runtime/os1_windows.go:

Line 261:
isASCII := true
b := (*[1<<30]byte)(buf)[:n]
for _, x := range b {
if x >= 0x80 {
isASCII = false
break
}
}


Line 267: if isConsole {
if isConsole && !isASCII


https://go-review.googlesource.com/#/c/16714/5/src/runtime/proc.go
File src/runtime/proc.go:

Line 1255: if iscgo && !cgoHasExtraM {

> The restriction on //go:nosplit functions is that

The use of go:nosplit on write in this case is because we are running
without a g or m, so we cannot do a prologue check. For that matter we
can't do much of anything. Perhaps the Windows write should first scan the
byte slice for bytes >= 0x80. If it doesn't find any, it can use the ASCII
write DLL call and no conversions.

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>
Gerrit-Reviewer: Minux Ma <mi...@golang.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: Yes

[Daniel Theophanes (Gerrit)]

Daniel Theophanes uploaded a new patch set:
https://go-review.googlesource.com/16714

runtime: use WriteConsole to implement print and panic on windows

Fixes #7864

Change-Id: Id13369352aeccac8387876f0b911e383c543c28e
---
M src/runtime/os1_windows.go

1 file changed, 85 insertions(+), 0 deletions(-)

[Russ Cox (Gerrit)]

Russ Cox has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 7: Code-Review+1

(3 comments)

Looks good, thanks. Every time I open this CL, it's shorter, which is
great. :-)

https://go-review.googlesource.com/#/c/16714/7/src/runtime/os1_windows.go
File src/runtime/os1_windows.go:

Line 290: //go:nosplit
Delete. This is not necessary anymore since we handled the ASCII messages
without calling writeConsole.


Line 302: total := 0
total := len(s)


Line 310: total += n
Delete.
No need for multiple variables doing the same work
(total is counting up and s is counting down).

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>
Gerrit-Reviewer: Minux Ma <mi...@golang.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: Yes

[Alex Brainman (Gerrit)]

Alex Brainman has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 7:

Since we don't have any way to test this change (on our builder or
otherwise), I am offer to test this visually here before submitting. (Also
feel free to ping mattn to do the same - he actually has real computer to
test this change with).

Just let me know.

Alex

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>
Gerrit-Reviewer: Minux Ma <mi...@golang.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: No

[Daniel Theophanes (Gerrit)]

Reviewers: Russ Cox

Daniel Theophanes uploaded a new patch set:
https://go-review.googlesource.com/16714

runtime: use WriteConsole to implement print and panic on windows

Fixes #7864

Change-Id: Id13369352aeccac8387876f0b911e383c543c28e
---
M src/runtime/os1_windows.go

1 file changed, 82 insertions(+), 0 deletions(-)

[Daniel Theophanes (Gerrit)]

Daniel Theophanes has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 8:

> Since we don't have any way to test this change (on our builder or
> otherwise), I am offer to test this visually here before
> submitting. (Also feel free to ping mattn to do the same - he
> actually has real computer to test this change with).
>
> Just let me know.
>
> Alex

Thanks Alex. I sent an email to mattn to see if he could test. Pending rsc,
I think it is ready to go.

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>
Gerrit-Reviewer: Minux Ma <mi...@golang.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-HasComments: No

[Yasuhiro MATSUMOTO (Gerrit)]

Yasuhiro MATSUMOTO has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 8:

(1 comment)

https://go-review.googlesource.com/#/c/16714/8/src/runtime/os1_windows.go
File src/runtime/os1_windows.go:

Line 334: stdcall5(_WriteConsoleW,
If WriteConsoleW return an error, write should return the error?

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>
Gerrit-Reviewer: Minux Ma <mi...@golang.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>

Gerrit-Reviewer: Yasuhiro MATSUMOTO <matt...@gmail.com>
Gerrit-HasComments: Yes

[Alex Brainman (Gerrit)]

Alex Brainman has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 8:

(3 comments)

Even after changing my console font to "Lucida Console"

https://cloud.githubusercontent.com/assets/9796621/11083822/e4be7c66-8887-11e5-8135-56222a318702.jpg

Russ's wish cannot be fulfilled. It does work for my cyrilic example.

mattn, does it work for you? I suspect it does work, but you use different
font.

Alex

https://go-review.googlesource.com/#/c/16714/8/src/runtime/os1_windows.go
File src/runtime/os1_windows.go:

Line 270: // Detect if this is attached to a console or not.
I don't see benefit of having comments here. Your code is clear enough. Up
to you to decide either way.


Line 272: isConsole := stdcall2(_GetConsoleMode, handle,
uintptr(unsafe.Pointer(&m))) != 0
Do not call GetConsoleMode unless isASCII is true.


Line 294: lock(&utf16ConsoleBackLock)
This new lock means multiple print statements won't be able to interleave
with each other. I am not sure if it is important.

[Daniel Theophanes (Gerrit)]

Daniel Theophanes has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 8:

(4 comments)

https://go-review.googlesource.com/#/c/16714/8/src/runtime/os1_windows.go
File src/runtime/os1_windows.go:

Line 270: // Detect if this is attached to a console or not.
> I don't see benefit of having comments here. Your code is clear enough. Up

Done

Line 272: isConsole := stdcall2(_GetConsoleMode, handle,
uintptr(unsafe.Pointer(&m))) != 0
> Do not call GetConsoleMode unless isASCII is true.

Done

Line 294: lock(&utf16ConsoleBackLock)
> This new lock means multiple print statements won't be able to interleave
> w

True. I think that behavior is fine.

Line 334: stdcall5(_WriteConsoleW,
> If WriteConsoleW return an error, write should return the error?

I don't think it should. This shouldn't error. If it does during a panic or
last ditch print, I'm not sure what this error would help.

[Daniel Theophanes (Gerrit)]

Daniel Theophanes has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 8:

> (3 comments)
>
> Even after changing my console font to "Lucida Console"
>
>
https://cloud.githubusercontent.com/assets/9796621/11083822/e4be7c66-8887-11e5-8135-56222a318702.jpg
>
> Russ's wish cannot be fulfilled. It does work for my cyrilic
> example.
>
> mattn, does it work for you? I suspect it does work, but you use
> different font.
>
> Alex

Alex, the characters are now correct. If you copy and paste them into a
capable editor you'll see the correct characters. This is just a font issue
now. Before we had corrupt characters; now we do not.

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>
Gerrit-Reviewer: Minux Ma <mi...@golang.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>
Gerrit-Reviewer: Yasuhiro MATSUMOTO <matt...@gmail.com>

Gerrit-HasComments: No

[Daniel Theophanes (Gerrit)]

Reviewers: Russ Cox

Daniel Theophanes uploaded a new patch set:
https://go-review.googlesource.com/16714

runtime: use WriteConsole to implement print and panic on windows

Fixes #7864

Change-Id: Id13369352aeccac8387876f0b911e383c543c28e
---
M src/runtime/os1_windows.go

1 file changed, 83 insertions(+), 0 deletions(-)

[Alex Brainman (Gerrit)]

Alex Brainman has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 8: Code-Review+1

Thank you. Leaving for rsc to check utf8 to utf16 conversion (since we
don't have test).

Alex

--
https://go-review.googlesource.com/16714
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Daniel Theophanes <kard...@gmail.com>
Gerrit-Reviewer: Minux Ma <mi...@golang.org>
Gerrit-Reviewer: Russ Cox <r...@golang.org>
Gerrit-Reviewer: Yasuhiro MATSUMOTO <matt...@gmail.com>

Gerrit-HasComments: No

[Alex Brainman (Gerrit)]

Alex Brainman has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 8:

> Alex, the characters are now correct. If you copy and paste them
> into a capable editor you'll see the correct characters. This is
> just a font issue now. Before we had corrupt characters; now we do
> not.

I understand.

[Yasuhiro MATSUMOTO (Gerrit)]

Yasuhiro MATSUMOTO has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 8:

> (3 comments)
>
> Even after changing my console font to "Lucida Console"
>
>
https://cloud.githubusercontent.com/assets/9796621/11083822/e4be7c66-8887-11e5-8135-56222a318702.jpg
>
> Russ's wish cannot be fulfilled. It does work for my cyrilic
> example.
>
> mattn, does it work for you? I suspect it does work, but you use
> different font.
>
> Alex

seems good to me.

http://go-gyazo.appspot.com/af16cf5b940c1ae4.png

[Alex Brainman (Gerrit)]

Alex Brainman has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 9:

> seems good to me.
>
> http://go-gyazo.appspot.com/af16cf5b940c1ae4.png

What is the console font you use?

Alex

[Yasuhiro MATSUMOTO (Gerrit)]

Yasuhiro MATSUMOTO has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 9:

> >
> > seems good to me.
> >
> > http://go-gyazo.appspot.com/af16cf5b940c1ae4.png
>
> What is the console font you use?
>
> Alex

I'm using Raster font bundled in windows. And it's possible to display
with "MS Gothic".
Probably, if using font which contains ANSI charset, for example SHIFT_JIS,
you can display output of my example.

[Alex Brainman (Gerrit)]

Alex Brainman has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 9: Code-Review+1

> I'm using Raster font bundled in windows. And it's possible to
> display with "MS Gothic".
> Probably, if using font which contains ANSI charset, for example
> SHIFT_JIS, you can display output of my example.

Thank you for the info.

Alex

[Russ Cox (Gerrit)]

Russ Cox has posted comments on this change.

runtime: use WriteConsole to implement print and panic on windows

Patch Set 9: Code-Review+2

[Russ Cox (Gerrit)]

Russ Cox has submitted this change and it was merged.

runtime: use WriteConsole to implement print and panic on windows

Fixes #7864

Change-Id: Id13369352aeccac8387876f0b911e383c543c28e
Reviewed-on: https://go-review.googlesource.com/16714
Reviewed-by: Alex Brainman <alex.b...@gmail.com>
Reviewed-by: Russ Cox <r...@golang.org>

---
M src/runtime/os1_windows.go
1 file changed, 83 insertions(+), 0 deletions(-)

Approvals:
Russ Cox: Looks good to me, approved
Alex Brainman: Looks good to me, but someone else must approve