Re: code review 4978047: ld: Fixes issue 1899 ("cannot create 8.out.exe") (issue 4978047)

Can I have more time to test this, please?

Alex

http://codereview.appspot.com/4978047/

Sure, it's all yours to submit or not.

http://codereview.appspot.com/4978047/diff/11011/doc/progs/run
File doc/progs/run (right):

http://codereview.appspot.com/4978047/diff/11011/doc/progs/run#newcode46
doc/progs/run:46: TMPFILE="/tmp/gotest3-$$-$USER"
I didn't look closely at this. Considering that main problem with 8l is
not resolved (see my other comment), I am not sure if this change helps
any or not.

The only thing that I have noticed is that sometimes this script will
leave files in /tmp, because of "set -e". If you proceed with this
change, perhaps it is OK just hard code file name. And it does not need
to be in /tmp. Everything in here runs in sequence.

http://codereview.appspot.com/4978047/diff/11011/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

http://codereview.appspot.com/4978047/diff/11011/src/cmd/ld/lib.c#newcode78
src/cmd/ld/lib.c:78: #endif
I wish it would work, but it doesn't. This program:

package main

import (
"bytes"
"exec"
"io"
"io/ioutil"
"log"
"os"
"strconv"
)

const prog = `
package main
func main() {
println("Hello")
}
`

/*
func runOne(args ...string) {
cmd := args[0]
args = args[1:]
b, err := exec.Command(cmd, args...).CombinedOutput()
if err != nil {
log.Fatalf("%s failed: %s: %s\n", cmd, err, b)
}
}
*/

func runOne(args ...string) string {
cmd := args[0]
fullcmd, err := exec.LookPath(cmd)
if err != nil {
log.Fatalf("LookPath(%s): %v", cmd, err)
}

r, w, err := os.Pipe()
if err != nil {
log.Fatalf("Pipe: %v", err)
}
attr := &os.ProcAttr{Files: []*os.File{nil, w, w}}
p, err := os.StartProcess(fullcmd, args, attr)
if err != nil {
log.Fatalf("StartProcess: %v", err)
}
defer p.Release()
w.Close()

var b bytes.Buffer
io.Copy(&b, r)
output := b.String()
msg, err := p.Wait(0)
if err != nil {
log.Fatalf("Wait: %v", err)
}
if !msg.Exited() || msg.ExitStatus() != 0 {
log.Fatalf("ExitStatus(%d): %v", msg.ExitStatus(), output)
}
return output
}

func run() {
// create source file
err := ioutil.WriteFile("hello.go", []byte(prog), 0666)
if err != nil {
log.Fatal(err)
}
defer os.Remove("hello.go")
// compile source file
runOne("8g", "-o", "hello.8", "hello.go")
defer os.Remove("hello.8")
// link executable
runOne("8l", "-o", "hello.exe", "hello.8")
// defer os.Remove("hello.exe")
// run executable
runOne("./hello.exe")
}

func main() {
if len(os.Args) != 2 {
log.Fatal("Invalid numberof args")
}
n, e := strconv.Atoi(os.Args[1])
if e != nil {
log.Fatalf("Must be a number (%s): %s\n", os.Args[1], e)
}
for i := 0; i < n; i++ {
run()
}
}

fails if I run it like

test.exe 10000

I wrote this, so we can't blame mingw or anything.

Your version improves things a bit (it doesn't fail as often as
original), but it is not 100%. Your previous attempts (where you were
moving file before deletion) do not work 100% either.

I tend to lean towards submitting this change - at least it improves on
our current situation. Maybe make a comment to say it is not 100%. What
do you think?

http://codereview.appspot.com/4978047/

I copied the tmp file code from test/run. Well, there was no set -e.

> test.exe 10000

Two questions:
1. does test/run fail on your machine ?
2. does this program fail if you do not provide pipe for stdout and
stderr (try nil for all 3 or handle of a disk file) ?

http://codereview.appspot.com/4978047/

I don't know. I didn't run it. I did see it fail in a similar fashion on
a different PC of mine couple month ago.

> nil for all 3 or handle ...) ?

Tried that. It still fails. Does my program fails for you?

Alex

http://codereview.appspot.com/4978047/

No, and neither bash.exe.
I could reproduce the bug with doc/progs/run putting go directory to
external usb drive (I do not have it right now to check your new prog
with it).
But I could not reproduce the bug with test/run tests.
So I started comparing doc/progs/run and test/run and after changing the
method of capturing output of test program all tests from doc/progs/run
passed many times even on that usb drive.

http://codereview.appspot.com/4978047/

Please fix problem with run script (where it would leave tmp file behind
on error), and I will submit. Perhaps, if you just use a fixed name tmp
file in $GOROOT/doc/progs directory, that will do the trick.

Thank you.

Alex

http://codereview.appspot.com/4978047/

Russ

Please, see this conversation http://goo.gl/RGFp6. Hector suggested to
rename file into a *temp* name, and it looks like it works for both him
and me. His suggestion is similar to one of your attempts, except the
temp file we choose is completely unique every time we use it.

Please, check if some programs I have posted work for you, then we could
implement it. I will be away for a week, so you could have a go, if you
like.

Alex

http://codereview.appspot.com/4978047/

There is one disadvantage: ...

Whatever you say, but current change (removeing "remove" before
"create") does not fixes the issue - I have a computer that still fails,
if you test it for long enough.

On the other hand, if we move file before deleting it, I could run test
for very long time and it would not fail. I was monitoring all
operations and none of them failed. It means we should not have garbage
files.

I am OK with whatever you decide to do. But I will keep with this myself
when I am back.

Alex

http://codereview.appspot.com/4978047/

> Alex

Yep, it looks like an impossible problem.

There are really 2 problems, which combined prevent us from fixing the
issue:
1. It is impossible to overwrite or delete a binary that is running.
2. It is impossible to know when the binary has been unmapped from
memory. Waiting on the process handle doesn't work because the handle
gets signalled as soon as ExitProcess() is called, and the binary will
still be mapped at that time.

As jp has said renaming isn't good enough as it's impossible to delete a
running binary, whether it has been renamed or not.

I think the only sane thing to do now is to revert the change to
ld/lib.c and change doc/progs/run so that the output file has the same
base name as the source file. Except that I see cat_rot13 is run twice,
so provisions will have to be made so that it doesn't get built twice.

http://codereview.appspot.com/4978047/

There are also tests in $GOROOT/test directory, not only in
$GOROOT/doc/progs/run.
If we revert changes in ld/lib.c they will fail.
There are a lot of them and some of them made such a way that one Go
program emit source for another one. And both of them are compiled to
the same binary 8.out.exe with no delay.
It would be a nice solution to give unique filename to each test (and
unique filename to each binary within the test), but it would be a much
bigger change than only can_rot13.

http://codereview.appspot.com/4978047/