code review 43150043: liblink: rewrite '\\' in paths to '/' on windows

liblink: rewrite '\\' in paths to '/' on windows
At least three Go tests rely on that (log, runtime/{pprof,debug}).

Fixes issue 6972.
Fixes issue 6974.
Fixes issue 6975.

[minux1, 2013-12-17 05:27:26]

Hello alex.brainman@gmail.com (cc: golang-dev@googlegroups.com),

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

[brainman, 2013-12-17 05:28:43]

I am not sure this is right fix. That is how paths were written before (with /
directory delimiter). Perhaps it is time to make them "proper" windows paths
(with \ directory delimiter). I am not sure what Russ's plan is.

Alex

[minux1, 2013-12-17 05:44:37]

On Dec 17, 2013 12:28 AM, <alex.brainman@gmail.com> wrote:
> I am not sure this is right fix. That is how paths were written before
> (with / directory delimiter). Perhaps it is time to make them "proper"
> windows paths (with \ directory delimiter). I am not sure what Russ's
> plan is.
this restores the original behavior,
as `/` is also a valid windows path separator, i don't find why this CL is
unacceptable. (i also think 3rd-party code might also rely on this fact,
it's not wise to break them given our code sets the precedence)

btw, i also tried to fix the tests, but i gave up because a proper fix that
i'm comfortable with required much more effort than worthwhile IMO. for
example, we should allow \ in unix paths, so a lot of constants containing
path separators need to be changed into variables or conditionally compiled.

[minux1, 2013-12-17 05:51:06]

please note that the c compiler and assembler are still using forward slash
unconditionally. there are also code in cmd/gc/lex.c that uses forward
slash to format a path.

[mattn, 2013-12-17 06:19:07]

On 2013/12/17 05:27:26, minux wrote:
> Hello mailto:alex.brainman@gmail.com (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go

Not LGTM.

Japanese shift_jis encoding have some characters which contains backslash in
trailing byte for double byte character set.
It should use isleadbyte() or IsDBCSLeadByte() on windows.

[brainman, 2013-12-17 06:23:00]

On 2013/12/17 06:19:07, mattn wrote:
> 
> Japanese shift_jis encoding have some characters ...

This part was always (still is) broken
https://code.google.com/p/go/issues/detail?id=3754. So no big deal.

Alex

[minux1, 2013-12-17 06:45:00]

On Dec 17, 2013 1:19 AM, <mattn.jp@gmail.com> wrote:
> Japanese shift_jis encoding have some characters which contains
> backslash in trailing byte for double byte character set.
> It should use isleadbyte() or IsDBCSLeadByte() on windows.
cmd/gc always makes the (incorrect) assumption that file arguments are in
utf-8.

try make a multibyte filename X.go, and compare these two:
go build X.go
go tool 6g X.go

you will find that 6g creates the wrong output filename.

[brainman, 2013-12-17 06:47:14]

On 2013/12/17 05:44:37, minux wrote:
> as `/` is also a valid windows path separator, i don't find why this CL is
> unacceptable. ...

`/` is valid, but is never used. That is why we have path/filepath package.
These are not different to my mind.

> ... (i also think 3rd-party code might also rely on this fact,
> it's not wise to break them given our code sets the precedence)

Good point. But I cannot make this call one way or the other.

> btw, i also tried to fix the tests, but i gave up because a proper fix that
> i'm comfortable with required much more effort than worthwhile IMO. ...

I don't buy this. Package needs to do the "right" thing. Tests come second.

Alex

[minux1, 2013-12-17 06:47:41]

this change breaks test/run.go errorcheck. i think i will have to do the
filename translation at obj creation time so that error output of gc
behaves like before. (or we can fix test/run.go)

[brainman, 2013-12-17 06:50:48]

On 2013/12/17 06:47:41, minux wrote:
> this change breaks test/run.go errorcheck. i think i will have to do the
> filename translation at obj creation time so that error output of gc
> behaves like before. (or we can fix test/run.go)

Lets wait for Russ. He said that linker changes will allow us to use Go for most
of it. Many of these will become simpler then. Lets see if that is his plan.

Alex

[mattn, 2013-12-17 06:56:27]

On 2013/12/17 06:45:00, minux wrote:
> On Dec 17, 2013 1:19 AM, <mailto:mattn.jp@gmail.com> wrote:
> > Japanese shift_jis encoding have some characters which contains
> > backslash in trailing byte for double byte character set.
> > It should use isleadbyte() or IsDBCSLeadByte() on windows.
> cmd/gc always makes the (incorrect) assumption that file arguments are in
> utf-8.
> 
> try make a multibyte filename X.go, and compare these two:
> go build X.go
> go tool 6g X.go
> 
> you will find that 6g creates the wrong output filename.

Yes, I confirmed this problem with 8g. In most of unix, 

http://go-gyazo.appspot.com/1e62666e56ead7f5.png

http://go-gyazo.appspot.com/7726ef07295118c2.png

[minux1, 2013-12-17 06:57:39]

please keep in mind one can cross compile from unix, so the filenames in go
object file cannot always be using windows notations anyway.

i believe the object file shouldn't contain platform specific features like
backward slashes (also given that forward slash is also valid and older gc
already used them)

[minux1, 2013-12-17 07:37:33]

Hello alex.brainman@gmail.com, mattn.jp@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[minux1, 2013-12-17 07:40:16]

R: +rsc.

i think the latest patch set fixes the right problem (use / only in go
objfile), but I think there are definitely better ways. Suggestions welcome.

[brainman, 2013-12-18 01:56:42]

LGTM.

Let's fix the build. We could always change it later.

Alex

https://codereview.appspot.com/43150043/diff/140001/src/liblink/objfile.c
File src/liblink/objfile.c (right):

https://codereview.appspot.com/43150043/diff/140001/src/liblink/objfile.c#new...
src/liblink/objfile.c:399: } else {
Since your if ends with return, you might as well drop else here.

[rsc, 2013-12-18 02:44:23]

LGTM



On Tue, Dec 17, 2013 at 8:56 PM, <alex.brainman@gmail.com> wrote:

> LGTM.
>
> Let's fix the build. We could always change it later.
>
> Alex
>
>
> https://codereview.appspot.com/43150043/diff/140001/src/liblink/objfile.c
> File src/liblink/objfile.c (right):
>
> https://codereview.appspot.com/43150043/diff/140001/src/
> liblink/objfile.c#newcode399
> src/liblink/objfile.c:399: } else {
> Since your if ends with return, you might as well drop else here.
>
> https://codereview.appspot.com/43150043/
>

[brainman, 2013-12-18 03:13:20]

We also need https://codereview.appspot.com/43730043/ to fix windows builders.

Alex

[minux1, 2013-12-18 03:40:20]

*** Submitted as https://code.google.com/p/go/source/detail?r=676b14022aff ***

liblink: rewrite '\\' in paths to '/' on windows
At least three Go tests rely on that (log, runtime/{pprof,debug}).

Fixes issue 6972.
Fixes issue 6974.
Fixes issue 6975.

R=alex.brainman, mattn.jp, rsc
CC=golang-dev
https://codereview.appspot.com/43150043