Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: racy writes to global var not observed #59088

Closed
huzhiwen93 opened this issue Mar 17, 2023 · 13 comments
Closed

cmd/compile: racy writes to global var not observed #59088

huzhiwen93 opened this issue Mar 17, 2023 · 13 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge

Comments

@huzhiwen93
Copy link

What version of Go are you using (go version)?

$ go version
1.13, 1.15, 1.17, 1.20

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

darwin, centos

go env Output
$ go env

What did you do?

Directly run the following code, you will see that the program keeps printing 0, which is not expected.

package main

import (
    "fmt"
    "time"
)

type X struct {
    X0 int
    X1 int
    X2 int
    X3 int
    X4 int
    X5 int
    X6 int
    X7 int
    X8 int
    X9 int
}

func (x *X) set(a int) {
    x.X0 = a
    x.X1 = a
    x.X2 = a
    x.X3 = a // [tag 2]
    //x.X4 = a // [tag A]
    //x.X5 = a
    //x.X6 = a
    //x.X7 = a
    //x.X8 = a
    //x.X9 = a
}

var global X

func main() {
    go change()
    go display()
    select {}
}

func change() {
    count := 0
    for {
        count += 1
        newX := X{} // [tag 1]
        newX.set(count)
        global = newX
        //time.Sleep(time.Microsecond) // [tag B]
    }
}

func display() {
    for {
        time.Sleep(time.Second)
        g := global
        fmt.Println(g.X0)
    }
}

But if we uncomment the line with [tag A], or uncomment the line with [tag B], or run the program with gcflag='-N -l', then the program can print results greater than 0, which is as expected.

Another experiment: If we change the line with [tag 1] as newX := X{X0:1}, then we have to further comment the line with [tag 2] to reproduce the 'all zero output' result. It seems to be related with the amount of instructions nearby.

By comparing the assembly codes generated by go tool compile (shown in the figure below), we find that the entire for loop in function change() is wiped out, probably because the optimizer infers that those lines of code do not make any influences.

Kim 2023-03-17 143013

The above test code was originally written to test the consistency during struct copy. And a real-world project might not be easy to trigger this bug. But it indeed reveals a defect in go compiler, which needs to be reviewed.

This issue has been testified by my colleagues.

@wdvxdr1123
Copy link
Contributor

I don't think this is a bug. There is no synchronization in your code. A program with a data race in it is not a valid Go program and it could do anything.
See #45826.

@merykitty
Copy link

@wdvxdr1123 It is not allowed to do "anything", it still must adhere to the Go memory model. But in this case, it is allowed to not observe the racy write to global and as a result, simply spin indefinitely.

@seankhliao seankhliao changed the title Go complier bug: removing necessary short codes during optimization cmd/compile: racy writes to global var not observed Mar 17, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 17, 2023
@seankhliao
Copy link
Member

I believe this is valid behavior according to the memory model

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Mar 17, 2023
@huzhiwen93
Copy link
Author

@seankhliao Please remind me how does the memory model make the output results different, between go run and go run -gcflags='-N -l'.

@GX666
Copy link

GX666 commented Mar 17, 2023

@seankhliao Please remind me how does the memory model make the output results different, between go run and go run -gcflags='-N -l'.

@seankhliao run the above code with go run -gcflags='-N -l' and go run , the results are different, it means the Go compiler makes unreasonable optimization

go_run

@seankhliao
Copy link
Member

In the presence of data races:

Otherwise, a read r of a memory location x that is not larger than a machine word must observe some write w such that r does not happen before w and there is no write w' such that w happens before w' and w' happens before r. That is, each read must observe a value written by a preceding or concurrent write.

In both instances, the program observes some valid value (initial write, write by other goroutine).
As it contains a data race, we make no guarantees about which value is observed.

@huzhiwen93
Copy link
Author

@seankhliao I am afraid we are talking about different things.

  1. As for the data race, I know there could be unpredictable outcome in one read. But since the program has been running for seconds, minutes or hours (with large number of none-zero write operations), how can the display function still keep reading the initial value, zero?
  2. From the assembly code, we can see that the write operation in for loop is completely removed. Therefore, there is actually no data race when the program is running. The program keeps printing 0-s simply because write operations are not included in the binary executable file.

@merykitty
Copy link

merykitty commented Mar 17, 2023

Let me clear up some things:

1, The memory model specifies a set of allowed behaviours, and a compiled program is deemed valid if its behaviour aligns with one of the allowed ones. As a result, 2 programs can express different behaviours and be both valid. In this case, the Go memory model says that the result of a racy load from a larger-than-word variable global is unspecified, which means that any value observed is legal, and both the unoptimised and the optimised compiled version is valid.

2, The memory model does not have a notion of wall clock, or real time, in the memory model, 1 action either happens before, at the same time or after another action. This happen-before relationship is a mathematical relationship defined and described in the memory model and has nothing to do with actual before-ness in terms of real time. In this program, the load g := global and the store global = newX do not establish a happen-before relationship. As a result, the load does not have to observe the store and it can return an arbitrary value which in this case it reads the original one.

3, Raciness, defined by the Go memory model is a property of a Go program, not the property of the compiled code. The raciness of which is defined by the memory model of the hardware architecture. As a result, multithreading reasoning must be performed on the Go source code, and in this case, analysis shows raciness in the global variable.

@huzhiwen93
Copy link
Author

@merykitty Am I right to restate your explanation as follows:
There is no happen-before relationship between racy write and read. So the compiler does not know whether write makes any observable differences. In this context, it is allowed to believe that read only relies on the initial value and write does not make actual influences. Therefore the compiler can choose to delete the 'useless' write without taking any responsibility, even if there is a possibility that write does have some influences to read in real-world.

(1) When the number of instructions near write increases, the complier tends to keep write, although the added instructions contribute none to determine the usefulness of write. Why? One should be concerned if a real-world code with deliberate none-synchronized variable (performance reason) works fine only because the number of instructions around it is above some threshold but not because the compiler has the responsibility to keep it.
(2) Assume that a mutex is added around read and write. The race is avoided, but the compiler still cannot determine wether read is before or after write. Can I explain that the compiler keeps write in this case just because lock and unlock add up many instructions around write, as it does in (1)?
(3) In C/C++ compiler, only O3+ is risky to change the logic of the code. But go compiler provides a risky default optimization. It is reasonable, from the view of a normal programmer?

In my opinion, the default compiler should be designed to translate the code and make it run as it looks like. Not in the other way around: the relation of write and read cannot be mathematically determined, so the compiler can freely delete the uncertain codes.

@merykitty
Copy link

@huzhiwen93 It seems to me that you are not used to working with concurrency at this low level, so I think that my attempt to explain the rationales behind the memory model would be futile. As a result, I will kindly refer to you the excellent blogs from Russ about this subject, which may clear up much of your confusion.

@ianlancetaylor
Copy link
Contributor

In my opinion, the default compiler should be designed to translate the code and make it run as it looks like.

History shows that that this really really hard to define. Different people have completely different intuitions about what it means to "run as it looks like."

That said, I am surprised that in this case the change function is compiled into an infinite loop that does nothing. @randall77 what compiler optimization is causing it to drop the assignment to the package scope variable?

@randall77
Copy link
Contributor

The memory state of that loop is never used. No breaks, no panics, nothing. The compiler elides memory modifications that are known not to lead to any synchronization point (or any read within the goroutine).

@ianlancetaylor
Copy link
Contributor

Thanks.

@golang golang locked and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

8 participants