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

ld: Fixes issue 1899 ("cannot create 8.out.exe")

http://code.google.com/p/go/issues/detail?id=1899

[jp, 2011-08-31 02:57:00]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-08-31 10:52:18]

This fixes two links in a row trying to use the same output file name.

What about three in a row?  Will the third link's rename fail because
the renamed file from the second link is really not yet gone?

Russ

[jp, 2011-08-31 21:14:57]

Three in a row work for me.

On 2011/08/31 10:52:18, rsc wrote:
> This fixes two links in a row trying to use the same output file name.
> 
> What about three in a row?  Will the third link's rename fail because
> the renamed file from the second link is really not yet gone?
> 
> Russ

[rsc, 2011-08-31 21:16:18]

Okay, sounds good.  Please remove the ifdef and use just "~".
We might as well do it everywhere.  It could help other
broken systems too.

[rsc, 2011-08-31 21:16:39]

temp = smprint("%s~", outfile)

[jp, 2011-08-31 21:40:41]

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

Please take another look.

[rsc, 2011-08-31 21:46:58]

With the changes below it would look fine except
that the rename has the arguments backward.
They were backward in your original CL too.
That makes me worry: why was this having any
effect when you tried it before?

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

http://codereview.appspot.com/4978047/diff/17001/src/cmd/ld/lib.c#newcode63
src/cmd/ld/lib.c:63: char* tempfile;
s/* / */

http://codereview.appspot.com/4978047/diff/17001/src/cmd/ld/lib.c#newcode75
src/cmd/ld/lib.c:75: // Windows 7 defers the actual remove
// Rename + remove to work around buggy remove on Windows 7.

[brainman, 2011-08-31 23:45:01]

Please, change CL description to something like:

"
ld: rename + remove to work around buggy remove on Windows 7

Fixes issue 1899.
"

Please, give me some time to verify your code. I need to find a PC to reproduce
this problem.

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

http://codereview.appspot.com/4978047/diff/17001/src/cmd/ld/lib.c#newcode76
src/cmd/ld/lib.c:76: tempfile = smprint("%s~", outfile);
As we have discovered, this situation is tricky. I am concerned that some of
your code might fail and we will not know what happened. Please, check results
of all of those operations you do and bail out if they fail with some
descriptive message.

[jp, 2011-09-01 04:26:40]

On 2011/08/31 21:46:58, rsc wrote:
> They were backward in your original CL too.

Oops.
That's funny, but it really does effect.
It gives a fresh look to the issue :)

> That makes me worry: why was this having any
> effect when you tried it before?

I have an idea why: the patch disables remove().
create() truncates existing file (if any) and it works.
Perhaps, a better fix would be just to delete line with remove().
What a buggy system we work around doing remove() before create() ?

PTAL.

[rsc, 2011-09-01 17:55:34]

yay portability

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

http://codereview.appspot.com/4978047/diff/17003/src/cmd/ld/lib.c#oldcode73
src/cmd/ld/lib.c:73: remove(outfile);
Skipping the remove is not okay either.
It is working around a Unix file system behavior
where a binary that is running is not writable,
so that if you run 6l x.6, 6.out, and then 6l x.6
again, if the 6.out hasn't finished, the second 6l
will fail to write a new file.  The remove is supposed
to take care of that.

Maybe Windows is keeping the outfile around
(delaying the remove) for the same reason?  
Do you know of any documentation explaining
why a Windows remove would be delayed?

It sounds like maybe the right code here is

// Unix doesn't like it when we write to a running
// (or, sometimes, recently run) binary, so remove
// the output file before writing it.  Windows postpones
// a remove of a running (or, sometimes, recently run)
// binary, so rename it before removing it.
p = smprint("%s~", outfile);
rename(outfile, p);
remove(p);
free(p);

[jp, 2011-09-01 23:54:47]

PTAL

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

http://codereview.appspot.com/4978047/diff/17003/src/cmd/ld/lib.c#oldcode73
src/cmd/ld/lib.c:73: remove(outfile);
On 2011/09/01 17:55:35, rsc wrote:
Hereinafter: 'Windows' refers to common things of all actual Windows (2000 to
Windows7), 'Windows_7_' refers to Windows7 specific.

> Skipping the remove is not okay either.
> It is working around a Unix file system behavior
> where a binary that is running is not writable,
> so that if you run 6l x.6, 6.out, and then 6l x.6
> again, if the 6.out hasn't finished, the second 6l
> will fail to write a new file.  The remove is supposed
> to take care of that.

Windows does not allow to write to a running file either.
Windows also does not allow to remove a running file.
But Windows allows to rename a running file (within the same filesystem, at
least).

So, this issue of rewriting a running file is the almost the same on Windows.

Solutions differ.
Adding remove() before create() does not solve it. 
Adding rename() does.
After rename(), the temporary file will have the executable code mmap'ed to
memory.
The temporary file becomes the file protected from being removed.
So in the chain "rename(out, tmp), remove(tmp), create(out)", remove() will fail
if the file is running.
And the temporary file will remain.

It also means that "%s~" is not enough.
Imagine 8.out.exe does not terminate long time, we create a new 8.out.exe, run,
the new one 8.out.exe, run, etc. 
Each running 8.out.exe with unique content must have its own file on disk with
unique name.
There will be "8.out.exe~1", "8.out.exe~2", etc. 
Who will take care of their deletion? (Well, on Windows it is possible to add a
filename into a list of files which will be deleted on next reboot when they
definetely are not running. But it looks too tricky to do so in a program like
the linker.)

Do the linker really must not fail on writing to running file? 
I think linker must fail if the output file is running.
If not-failing behavior is really needed, user should use 'rm' before in his
shell or makefile scripts (and see 'rm' fails).

For Unix case you wrote about it could be worth do handle create() failure doing
remove() then create() again.
But not remove() before create() which may succeed. 
On Windows_7_ remove() can break the subsequent create() .

> Maybe Windows is keeping the outfile around
> (delaying the remove) for the same reason?  
> Do you know of any documentation explaining
> why a Windows remove would be delayed?

What I said above on Windows comes from my pre-Windows_7_ experience.
The delayed remove() looks like a novel feature.
It looks more not like an API change but another process (microsoft antivirus?)
keeping the file's handle open.

"The DeleteFile function fails if an application attempts to delete a file that
is open for normal I/O or as a memory-mapped file.
The DeleteFile function marks a file for deletion on close. Therefore, the file
deletion does not occur until the last handle to the file is closed. Subsequent
calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED."
http://msdn.microsoft.com/en-us/library/aa363915(v=vs.85).aspx

> It sounds like maybe the right code here is
> 
> // Unix doesn't like it when we write to a running
> // (or, sometimes, recently run) binary, so remove
> // the output file before writing it.  Windows postpones
> // a remove of a running (or, sometimes, recently run)
> // binary, so rename it before removing it.
Not very correct: Windows does not allow neither to write to nor to remove of a
running file.
The problem is: on Windows_7_ calling remove(name) triggers the weird machinery
which put the filename into a strange state for a while. 
create(name) fails during this period and also the second remove(name) would
fail with ACCESS_DENIED error instead of expected FILE_NOT_FOUND.
The period can be quite long (10s and more).

> p = smprint("%s~", outfile);
> rename(outfile, p);
> remove(p);
> free(p);
> 

Maybe:
	cout = create(outfile, 1, 0775);
	if(cout < 0) {
+		remove(outfile);
+		cout = create(outfile, 1, 0775);
+		if(cout < 0) {
			diag("cannot create %s", outfile);
			errorexit();
+		}
	}

How do you think, does it solve the Unix issue?

[rsc, 2011-09-01 23:59:48]

I don't think you have to check the return values.
Just do a rename + remove and we're done.
If it works, it works.

Russ

[jp, 2011-09-02 00:15:59]

On 2011/09/01 23:59:48, rsc wrote:

Do you mean?

	cout = create(outfile, 1, 0775);
	if(cout < 0) {
-		remove(outfile);
+		rename(outfile, tmpfile);
+		remove(tmpfile);
		cout = create(outfile, 1, 0775);
		if(cout < 0) {
			diag("cannot create %s", outfile);
			errorexit();
		}
	}

It is essential on Windows to try create() prior to any rename/remove.
create() has more chances to success being called on the first place.

> I don't think you have to check the return values.
> Just do a rename + remove and we're done.
> If it works, it works.
> 
> Russ

[jp, 2011-09-02 00:33:40]

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

Please take another look.

[bsiegert, 2011-09-02 08:24:06]

On Thu, Sep 1, 2011 at 19:55,  <rsc@golang.org> wrote:
> Maybe Windows is keeping the outfile around
> (delaying the remove) for the same reason?
> Do you know of any documentation explaining
> why a Windows remove would be delayed?

Are you sure it is being delayed and does not simply fail with "file
busy" or some such error?

--Benny.

[rsc, 2011-09-02 17:21:22]

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

http://codereview.appspot.com/4978047/diff/11006/src/cmd/ld/lib.c#newcode75
src/cmd/ld/lib.c:75: // It is essencial to try create() first
No, I mean replace lines 75-90 in this version with


// Unix doesn't like it when we write to a running
// (or, sometimes, recently run) binary, so remove
// the output file before writing it.  Windows postpones
// a remove of a running (or, sometimes, recently run)
// binary, so rename it before removing it.
p = smprint("%s~", outfile);
rename(outfile, p);
remove(p);
free(p);

That's good enough to handle the back-to-back
case, and I don't mind if the OS bugs shine 
through in the general case.

[jp, 2011-09-05 16:25:04]

On 2011/09/02 17:21:22, rsc wrote:
> http://codereview.appspot.com/4978047/diff/11006/src/cmd/ld/lib.c
> File src/cmd/ld/lib.c (right):
> 
> http://codereview.appspot.com/4978047/diff/11006/src/cmd/ld/lib.c#newcode75
> src/cmd/ld/lib.c:75: // It is essencial to try create() first
> No, I mean replace lines 75-90 in this version with
> 
> 
> // Unix doesn't like it when we write to a running
> // (or, sometimes, recently run) binary, so remove
> // the output file before writing it.  Windows postpones
> // a remove of a running (or, sometimes, recently run)
> // binary, so rename it before removing it.
> p = smprint("%s~", outfile);
> rename(outfile, p);
> remove(p);
> free(p);
> 
> That's good enough to handle the back-to-back
> case, and I don't mind if the OS bugs shine 
> through in the general case.

If 8.out.exe still running, second ld's run will rename it 8.out.exe~,
then will fail to remove 8.out.exe~, then will create the new 8.out.exe.
So, the only problem of ld's run before the previous 8.out.exe is terminated is
the 8.out.exe~ file left on the disk.

Third ld's run will fail on rename(), then will not be able to create output
file.

I think, such a behavior will be a source of unclear bug reports.

[rsc, 2011-09-05 17:07:53]

Okay, then let's just go back to replacing

	remove(outfile);

with

#ifndef _WIN32
	remove(outfile);
#endif

[brainman, 2011-09-06 02:25:05]

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

http://codereview.appspot.com/4978047/diff/11006/src/cmd/ld/lib.c#newcode80
src/cmd/ld/lib.c:80: if(0 != remove(outfile)) {
I still get errors with your patch. I could see first create fails. Then this
remove succeeds, therefore, the program does not even attempts to try renaming
part. And, finally, second create fails.

[brainman, 2011-09-06 02:30:48]

On 2011/09/05 16:25:04, jp wrote:
> 
> Third ld's run will fail on rename(), then will not be able to create output
> file.
> 

I can not reproduce your assumption. Russ patch works for me all the time.
Perhaps my PC is not fast enough. Can we remove(8.out.exe~) before renaming
part?

I also concerned that we are clobbering 8.out.exe~. It is an unusable name, but
still.

Also, should this only do renaming part for 8.out.exe only, or for any output
file?

[brainman, 2011-09-06 02:31:32]

On 2011/09/05 17:07:53, rsc wrote:
> 
> #ifndef _WIN32
> 	remove(outfile);
> #endif

This fails in my tests.

Alex

[jp, 2011-09-06 04:24:55]

On 2011/09/06 02:30:48, brainman wrote:
> On 2011/09/05 16:25:04, jp wrote:
> > 
> > Third ld's run will fail on rename(), then will not be able to create output
> > file.
> > 
> 
> I can not reproduce your assumption. Russ patch works for me all the time.
> Perhaps my PC is not fast enough. Can we remove(8.out.exe~) before renaming
> part?

OK.

> I also concerned that we are clobbering 8.out.exe~. It is an unusable name,
but
> still.

Yes, that' why I used "%s~~~" first. Some programs (FTE text editor, for
example) use ~ suffix for storing previous versions of files they work with. I
have changed to more unusual "%s~^".

> Also, should this only do renaming part for 8.out.exe only, or for any output
file?

For any output file. 
8.out.exe and 6.out.exe are default names and thus we see them more often in bug
reports.

PTAL, tha last patch is Russ' one (which seems to work for all of us) with your
comments (delete the temp file before rename to it and weirder name of the temp
file).

[brainman, 2011-09-06 07:09:47]

On 2011/09/06 04:24:55, jp wrote:
> 
> PTAL, tha last patch is Russ' one (which seems to work for all of us) with
your
> comments (delete the temp file before rename to it and weirder name of the
temp
> file).

I do not like what we are doing here, but I see no alternative at this moment.
LGTM.

Russ?

[rsc, 2011-09-07 17:52:19]

I am confused.

I thought we had finished enumerating the
reasons that rename+remove was not okay,
so we were just going to replace the existing
1 line:

       remove(outfile);

with 3 lines:

#ifndef _WIN32
       remove(outfile);
#endif

Russ

[brainman, 2011-09-07 23:12:52]

On 2011/09/07 17:52:19, rsc wrote:
> 
> #ifndef _WIN32
>        remove(outfile);
> #endif
> 

I do not know who said what, but this change does not fixes the problem in my
environment (Windows 7).

I do not want to introduce complexity for others, but I don't see any better
solutions at this stage. Perhaps we could leave exiting code alone for
non-windows and change windows version to do what we need to do. Perhaps we will
find better solution in the future.

Alex

[rsc, 2011-09-08 00:53:40]

On Wed, Sep 7, 2011 at 19:12,  <alex.brainman@gmail.com> wrote:
> On 2011/09/07 17:52:19, rsc wrote:
>
>> #ifndef _WIN32
>>        remove(outfile);
>> #endif
>
>
> I do not know who said what, but this change does not fixes the problem
> in my environment (Windows 7).

Okay, that's interesting.  In the original CL that was sent out,
there was an incorrect (backwards) rename that fixed a problem
(maybe not yours) by mostly accidentally avoiding the remove.
So I thought that just not doing the remove on Windows would
solve it.  What is the problem that you are seeing?

It sounds like we need an #ifdef, despite trying not to.

On Unix, the correct behavior is remove(outfile).

What is the correct behavior on Windows?
If someone sends a CL that replaces

remove(outfile)

with

#ifdef _WIN32
    ... something
#else
    // On Unix, remove target before writing to it,
    // to avoid problems with caches that think the
    // target is still running.
    remove(outfile);
#endif

then I will be happy.  I will leave it to the Windows
experts to figure out the appropriate "... something".

Russ

[brainman, 2011-09-08 03:05:26]

On 2011/09/08 00:53:40, rsc wrote:
> ...  What is the problem that you are seeing?

$ pwd
/C/MinGW/go/doc/progs
$ hg diff
diff -r ef81a396f9ae doc/progs/run
--- a/doc/progs/run	Tue Sep 06 16:04:55 2011 -0700
+++ b/doc/progs/run	Thu Sep 08 11:11:07 2011 +1000
@@ -62,7 +62,7 @@
 
 
 testit helloworld "" "Hello, world; or Καλημέρα κόσμε; or こんにちは 世界"
-testit helloworld3 "" "hello, world can't open file; err=no such file or
directory"
+#testit helloworld3 "" "hello, world can't open file; err=no such file or
directory"
 testit echo "hello, world" "hello, world"
 testit sum "" "6"
 testit strings "" ""
diff -r ef81a396f9ae src/cmd/ld/lib.c
--- a/src/cmd/ld/lib.c	Tue Sep 06 16:04:55 2011 -0700
+++ b/src/cmd/ld/lib.c	Thu Sep 08 11:11:07 2011 +1000
@@ -70,7 +70,7 @@
 	// add goroot to the end of the libdir list.
 	libdir[nlibdir++] = smprint("%s/pkg/%s_%s", goroot, goos, goarch);
 
-	remove(outfile);
+//	remove(outfile);
 	cout = create(outfile, 1, 0775);
 	if(cout < 0) {
 		diag("cannot create %s", outfile);
$ for i in *; do ./run || break; done
cannot create 8.out.exe
$ 

I ran "process monitor" along and this is the error:

12:42:47.5140956
PM	8l.exe	308	CreateFile	C:\MinGW\go\doc\progs\8.out.exe	SHARING
VIOLATION	Desired Access: Generic Write, Read Attributes, Disposition:
OverwriteIf, Options: Synchronous IO Non-Alert, Non-Directory File, Attributes:
N, ShareMode: Read, Write, AllocationSize: 0

> 
> It sounds like we need an #ifdef, despite trying not to.
> 

Agreed.

Alex

[jp, 2011-09-08 07:44:49]

On 2011/09/08 03:05:26, brainman wrote:
> -	remove(outfile);
> +//	remove(outfile);

> $ for i in *; do ./run || break; done
> cannot create 8.out.exe

Works OK for me.
Will try on a slower PC.

[jp, 2011-09-08 09:50:11]

Reproduced with an external USB drive.
It seems to be a bug of MinGW's bash.exe
Not reproducible with the tests in test directory.
PTAL.

On 2011/09/08 07:44:49, jp wrote:
> On 2011/09/08 03:05:26, brainman wrote:
> > -	remove(outfile);
> > +//	remove(outfile);
> 
> > $ for i in *; do ./run || break; done
> > cannot create 8.out.exe
> 
> Works OK for me.
> Will try on a slower PC.

[rsc, 2011-09-12 17:00:24]

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

http://codereview.appspot.com/4978047/diff/15003/doc/progs/run#newcode45
doc/progs/run:45: TMPFILE="/tmp/gotest3-$$-$USER"
# Write to temporary file to avoid mingw bash bug.
TMPFILE="..."

[jp, 2011-09-12 18:10:05]

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

http://codereview.appspot.com/4978047/diff/15003/doc/progs/run#newcode45
doc/progs/run:45: TMPFILE="/tmp/gotest3-$$-$USER"
On 2011/09/12 17:00:24, rsc wrote:
> # Write to temporary file to avoid mingw bash bug.
> TMPFILE="..."
ok.
I added this comment into test/run as well.
It seems to be done there for the same reason, I just copied the solution from
there to doc/progs/run

[rsc, 2011-09-12 18:29:47]

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

http://codereview.appspot.com/4978047/diff/6004/doc/progs/run#newcode45
doc/progs/run:45: # Write to temporary file to avoid mingw bash bug.
I assume you were introducing $TMPFILE because

x=$(echo $(./$O.out $2 2>&1))

was breaking.  Is that true?

http://codereview.appspot.com/4978047/diff/6004/test/run
File test/run (right):

http://codereview.appspot.com/4978047/diff/6004/test/run#newcode37
test/run:37: # Write to temporary file to avoid mingw bash bug.
There's no bash bug here.
The temporary file is used because its contents get
referred to multiple times.  I think you can just revert
the changes to this file.

[jp, 2011-09-12 19:21:57]

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

http://codereview.appspot.com/4978047/diff/6004/doc/progs/run#newcode45
doc/progs/run:45: # Write to temporary file to avoid mingw bash bug.
On 2011/09/12 18:29:48, rsc wrote:
> I assume you were introducing $TMPFILE because
> 
> x=$(echo $(./$O.out $2 2>&1))
> 
> was breaking.  Is that true?

Yes.
Seems that bash does not close stdout of the spawned process on time thus
preventing the executable file to be deleted.

"rm $O.out.exe" fails just after "x=$(echo $(./$O.out $2 2>&1))"

http://codereview.appspot.com/4978047/diff/6004/test/run
File test/run (right):

http://codereview.appspot.com/4978047/diff/6004/test/run#newcode37
test/run:37: # Write to temporary file to avoid mingw bash bug.
On 2011/09/12 18:29:48, rsc wrote:
> There's no bash bug here.
> The temporary file is used because its contents get
> referred to multiple times.  I think you can just revert
> the changes to this file.

ok

[rsc, 2011-09-14 15:19:45]

LGTM

Thanks for seeing it through.

[brainman, 2011-09-15 07:38:12]

On 2011/09/14 15:19:45, rsc wrote:
> LGTM
> 

Can I have more time to test this, please?

Alex

[rsc, 2011-09-15 15:21:39]

On Thu, Sep 15, 2011 at 03:38,  <alex.brainman@gmail.com> wrote:
> Can I have more time to test this, please?

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

[brainman, 2011-09-16 02:27:50]

Thank you for sticking with this.

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?

[jp, 2011-09-16 02:38:29]

On 2011/09/16 02:27:50, brainman wrote:
 
> 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.

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

> fails if I run it like
> 
> test.exe 10000
> 
> I wrote this, so we can't blame mingw or anything.

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) ?

[brainman, 2011-09-16 02:51:56]

On 2011/09/16 02:38:29, jp wrote:
> 1. does test/run fail on your machine ?

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.

> 2. does this program fail if you do not provide pipe for stdout and stderr
(try
> nil for all 3 or handle ...) ?

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

Alex

[jp, 2011-09-16 03:01:12]

On 2011/09/16 02:51:56, brainman wrote:
> On 2011/09/16 02:38:29, jp wrote:
> > 1. does test/run fail on your machine ?
> 
> 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.
> 
> > 2. does this program fail if you do not provide pipe for stdout and stderr
> (try
> > nil for all 3 or handle ...) ?
> 
> Tried that. It still fails. Does my program fails for you?
 
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.

[brainman, 2011-09-19 03:49:15]

I see no way to improve on your change to ld.

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

[jp, 2011-09-23 23:56:05]

On 2011/09/19 03:49:15, brainman wrote:
PTAL
> I see no way to improve on your change to ld.
> 
> 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

[brainman, 2011-09-24 00:32:10]

On 2011/09/23 23:56:05, jp wrote:
> On 2011/09/19 03:49:15, brainman wrote:
> PTAL

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

[hector, 2011-09-24 12:18:57]

On Sep 24, 2:18 am, jp wrote:
> > > I wonder what happens if TEMP directory and our executable are on
> > > different drives.
> 
> > Good point.  In that case let's not bother renaming it to temp
> > directory, just rename to temp name in current directory.
> 
> We had this solution.
> 
> The problem is:
> 1. if we rename always to sprintf("%s~", oldname), we just postpone the
> problem with deletion of 8.out.exe to the same problem with deletion
> of 8.out.exe~
> 2. if we rename to a random name, eventually we will have garbage files in a
> working directory. Well, there is MoveFileEx
> and MOVEFILE_DELAY_UNTIL_REBOOT, but until reboot there will be files
> which the linker failed to delete: 8.out.exe~1, 8.out.exe~2, 8.out.exe~3

I think we need to clarify what we want to work and what we shouldn't support.

We would like the following to work:
- Overwrite an existing binary that is not running or has just finished running.

We shouldn't/can't support the following:
- Overwrite an existing binary that is currently running.
- Creating a new binary with a name that is pending deletion.

On that basis I think jp's changes LGTM.

NB. The issue with bash is probably because command substitution doesn't wait
for the command to complete, it just waits for the pipe to close.  The former
doesn't necessarily happen before the latter.

[brainman, 2011-09-24 13:08:13]

On 2011/09/24 12:18:57, hector wrote:
> 
> I think we need to clarify what we want to work and what we shouldn't support.
> 

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

[hector, 2011-09-24 14:26:03]

On 2011/09/24 13:08:13, brainman wrote:
> On 2011/09/24 12:18:57, hector wrote:
> > 
> > I think we need to clarify what we want to work and what we shouldn't
support.
> > 
> 
> 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

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.

[jp, 2011-09-24 19:12:38]

> 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.

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.

[brainman, 2011-10-03 06:39:48]

It seems to me, we have wasted enough time with this. As I said earlier, this
works most of the time. So, I propose that we submit it as is. I will look into
this again when I have more time. I have LGTM from rsc, hector and myself, so I
will submit unless I hear otherwise in a day or two.

Alex

[rsc, 2011-10-03 14:24:43]

LGTM

[brainman, 2011-10-04 05:32:40]

Tried to submit it, but build fails now: doc/progs/run fails, because "testit
helloworld3 ..." fails (it is expected to fail). We used to execute "testit
helloworld3 ..." in a subshell, so, perhaps, exit code didn't matter before.

Alex

[rsc, 2011-10-05 15:33:46]

On Tue, Oct 4, 2011 at 01:32,  <alex.brainman@gmail.com> wrote:
> Tried to submit it, but build fails now: doc/progs/run fails, because
> "testit helloworld3 ..." fails (it is expected to fail). We used to
> execute "testit helloworld3 ..." in a subshell, so, perhaps, exit code
> didn't matter before.

Testit prints an error on mismatch
but I don't think it exits the script with
a failed status.

What is the error you are seeing?

Russ

[brainman, 2011-10-06 00:21:54]

On 2011/10/05 15:33:46, rsc wrote:
> On Tue, Oct 4, 2011 at 01:32,  <mailto:alex.brainman@gmail.com> wrote:
> > Tried to submit it, but build fails now: doc/progs/run fails, because
> > "testit helloworld3 ..." fails (it is expected to fail). We used to
> > execute "testit helloworld3 ..." in a subshell, so, perhaps, exit code
> > didn't matter before.
> 
> Testit prints an error on mismatch
> but I don't think it exits the script with
> a failed status.
> 
> What is the error you are seeing?
> 

$ cd $GOROOT/doc/progs
$ ./run; echo $?
1
$ bash -x ./run
+ set -e
++ gomake --no-print-directory -f ../../src/Make.inc go-env
...
...
+ testit helloworld3 '' 'hello, world can'\''t open file; err=no such file or
directory'
+ 8l helloworld3.8
+ ./8.out
$

Running helloworld3 fails:

$ cat helloworld3.go
// Copyright 2009 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package main

import (
        "./file"
        "fmt"
        "os"
)

func main() {
        hello := []byte("hello, world\n")
        file.Stdout.Write(hello)
        f, err := file.Open("/does/not/exist")
        if f == nil {
                fmt.Printf("can't open file; err=%s\n", err.String())
                os.Exit(1)
        }
}

Alex

[rsc, 2011-10-06 17:38:03]

Aha!

Put the 8.out command in ( ) I guess.

Russ

[brainman, 2011-10-06 23:10:36]

On 2011/10/06 17:38:03, rsc wrote:
> 
> Put the 8.out command in ( ) I guess.
> 

I don't think we want to do that, running our process in a sub-process. I think
we were trying to move away from that.

Alex

[rsc, 2011-10-06 23:14:37]

On Thu, Oct 6, 2011 at 19:10,  <alex.brainman@gmail.com> wrote:
> I don't think we want to do that, running our process in a sub-process.
> I think we were trying to move away from that.

Add || true to the end of the command instead?


Russ

[brainman, 2011-10-06 23:17:05]

On 2011/10/06 23:14:37, rsc wrote:
> 
> Add || true to the end of the command instead?
> 

SGTM. I will wait for jp to change.

Alex

[jp, 2011-10-07 08:56:39]

I did not understand which change do you want to be done.

On 2011/10/06 23:17:05, brainman wrote:
> On 2011/10/06 23:14:37, rsc wrote:
> > 
> > Add || true to the end of the command instead?
> > 
> 
> SGTM. I will wait for jp to change.
> 
> Alex

[brainman, 2011-10-07 11:05:04]

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

http://codereview.appspot.com/4978047/diff/41001/doc/progs/run#newcode60
doc/progs/run:60: ./$O.out | $2 2>&1 >"$TMPFILE"
I think you could do:
./$O.out | $2 2>&1 >"$TMPFILE" || true
to ignore non-zero exit code returned by helloworld3.
Unless you have better idea.

[bradfitz, 2011-10-14 17:37:17]

Is this still unresolved?  I'm hitting this problem on windows-386 on
Windows 7.

Why are the buildbots cool but my machine isn't?


On Fri, Oct 7, 2011 at 4:05 AM, <alex.brainman@gmail.com> wrote:

>
>
http://codereview.appspot.com/**4978047/diff/41001/doc/progs/**run<http://cod...
> File doc/progs/run (right):
>
> http://codereview.appspot.com/**4978047/diff/41001/doc/progs/**
>
run#newcode60<http://codereview.appspot.com/4978047/diff/41001/doc/progs/run#newcode60>
> doc/progs/run:60: ./$O.out | $2 2>&1 >"$TMPFILE"
> I think you could do:
> ./$O.out | $2 2>&1 >"$TMPFILE" || true
> to ignore non-zero exit code returned by helloworld3.
> Unless you have better idea.
>
>
http://codereview.appspot.com/**4978047/<http://codereview.appspot.com/4978047/>
>

[hector, 2011-10-14 17:40:50]

The buildbots are slower than our PCs?
If jp would move this forward, we can all benefit.  Unfortunately this
fix is only a workaround, so there isn't much enthusiasm.

On 14 October 2011 18:37, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Is this still unresolved?  I'm hitting this problem on windows-386 on
> Windows 7.
> Why are the buildbots cool but my machine isn't?
>
> On Fri, Oct 7, 2011 at 4:05 AM, <alex.brainman@gmail.com> wrote:
>>
>> http://codereview.appspot.com/4978047/diff/41001/doc/progs/run
>> File doc/progs/run (right):
>>
>> http://codereview.appspot.com/4978047/diff/41001/doc/progs/run#newcode60
>> doc/progs/run:60: ./$O.out | $2 2>&1 >"$TMPFILE"
>> I think you could do:
>> ./$O.out | $2 2>&1 >"$TMPFILE" || true
>> to ignore non-zero exit code returned by helloworld3.
>> Unless you have better idea.
>>
>> http://codereview.appspot.com/4978047/
>
>

[jp, 2011-10-14 19:03:41]

Hello rsc@golang.org, alex.brainman@gmail.com, bsiegert@gmail.com,
hectorchu@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-10-14 19:22:17]

LGTM

[hector, 2011-10-14 19:24:19]

Don't submit yet - I'm looking at this CL now and I don't think it
runs all the tests.

On 14 October 2011 20:22, Russ Cox <rsc@golang.org> wrote:
> LGTM
>
>

[hector, 2011-10-14 19:29:55]

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

http://codereview.appspot.com/4978047/diff/67001/doc/progs/run#newcode50
doc/progs/run:50: ./$O.out $2 2>&1 >"$TMPFILE"
You need to add || true to this line as well.

[hector, 2011-10-14 19:35:46]

Since it's close enough, and I've made the necessary change locally,
I'll go ahead and submit this now.

On 14 October 2011 20:29,  <hectorchu@gmail.com> wrote:
>
> http://codereview.appspot.com/4978047/diff/67001/doc/progs/run
> File doc/progs/run (right):
>
> http://codereview.appspot.com/4978047/diff/67001/doc/progs/run#newcode50
> doc/progs/run:50: ./$O.out $2 2>&1 >"$TMPFILE"
> You need to add || true to this line as well.
>
> http://codereview.appspot.com/4978047/
>

[hector, 2011-10-14 19:37:32]

*** Submitted as http://code.google.com/p/go/source/detail?r=f650efd9ed8d ***

ld: Fixes issue 1899 ("cannot create 8.out.exe")

http://code.google.com/p/go/issues/detail?id=1899

R=rsc, alex.brainman, bsiegert, hectorchu, bradfitz
CC=golang-dev
http://codereview.appspot.com/4978047

Committer: Hector Chu <hectorchu@gmail.com>