[PATCH] D75849: [compiler-rt] Allow golang race detector to run on musl-c

[Tomas Volf via Phabricator via llvm-commits]

graywolf-at-work marked 4 inline comments as done.
graywolf-at-work added a comment.

In D75849#1913927 <https://reviews.llvm.org/D75849#1913927>, @dvyukov wrote:

> How did you test this?


Fired up Alpine (musl) docker container (golang:1.14.1-alpine3.11), build
compiler-rt with this patch, replaced golang's race_linux_amd64.syso with the
new one and tried running go test with -race flag and it worked.

Same on Archlinux (glibc) to make sure I did not break it.

Tested program was:

  ~/foo $ cat /etc/os-release
  NAME="Alpine Linux"
  ID=alpine
  VERSION_ID=3.11.3
  PRETTY_NAME="Alpine Linux v3.11"
  HOME_URL="https://alpinelinux.org/"
  BUG_REPORT_URL="https://bugs.alpinelinux.org/"
  ~/foo $ cat main.go
  package main
  
  import "time"
  import "fmt"
  import "math/rand"
  
  func main() {
          start := time.Now()
          var t *time.Timer
          t = time.AfterFunc(randomDuration(), func() {
                  fmt.Println(time.Now().Sub(start))
                  t.Reset(randomDuration())
          })
          time.Sleep(5 * time.Second)
  }
  
  func randomDuration() time.Duration {
          return time.Duration(rand.Int63n(1e9))
  }
  ~/foo $ go run -race .
  948.325306ms
  ==================
  WARNING: DATA RACE
  Read at 0x00c000010028 by goroutine 8:
    main.main.func1()
        /home/ci/foo/main.go:12 +0x121
  
  Previous write at 0x00c000010028 by main goroutine:
    main.main()
        /home/ci/foo/main.go:10 +0x18d
  
  Goroutine 8 (running) created at:
    time.goFunc()
        /usr/local/go/src/time/sleep.go:168 +0x51
  ==================
  1.033222843s
  1.699865735s
  1.935354245s
  2.222914073s
  2.772484274s
  3.405927055s
  3.738185435s
  3.921800399s
  4.40249537s
  Found 1 data race(s)
  exit status 66



> It would be good to add a test because it will be broken the very next
>  release again. People changing C++ mode don't have any good means to
>  foresee/test for any Go problems.

I agree, but short of running it on actual Alpine I do not see bullet-proof way
to do it.

> After running buildgo.sh I see:
> 
> go$ nm test | grep libc; ldd test
>  000000000002e9b0 T __libc_csu_fini
>  000000000002e950 T __libc_csu_init
> 
>                    U __libc_free@@GLIBC_2.2.5
>                    U __libc_malloc@@GLIBC_2.2.5
>                    U __libc_realloc@@GLIBC_2.2.5
>                    U __libc_stack_end@@GLIBC_2.2.5
>                    U __libc_start_main@@GLIBC_2.2.5
>   	linux-vdso.so.1 (0x00007ffd695dd000)
>   	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007ff66aa37000)
>   	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007ff66a877000)
>   	/lib64/ld-linux-x86-64.so.2 (0x00007ff66b44a000)
> 
> 
> We could grep nm and/or link something without libc in buildgo.sh.

I've tried putting something together. I did `nm` to `race_linux_amd64.syso`
instead, seemed like a better way. Do you think it is ok to do it that way?



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:1075
 uptr GetPageSize() {
-#if SANITIZER_LINUX && (defined(__x86_64__) || defined(__i386__))
+#if SANITIZER_LINUX && defined(EXEC_PAGESIZE)
   return EXEC_PAGESIZE;
----------------
dvyukov wrote:
> Looking at the comment below, this seems to be a hard-earned bit of logic:
> // EXEC_PAGESIZE may not be trustworthy.
> 
> Why do we need this change?
> 
Musl libc does not have that define:

```
# cat /etc/os-release
NAME="Alpine Linux"
ID=alpine
VERSION_ID=3.11.3
PRETTY_NAME="Alpine Linux v3.11"
HOME_URL="https://alpinelinux.org/"
BUG_REPORT_URL="https://bugs.alpinelinux.org/"
# grep -r EXEC_PAGESIZE /usr/include ; echo $?
1
```

I've returned the check for the architecture back in there.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp:65
 
-#if SANITIZER_FREEBSD
+#if SANITIZER_FREEBSD && !SANITIZER_GO
 extern "C" void *__libc_stack_end;
----------------
dvyukov wrote:
> Why is this needed? How does this affect alpine linux?
Musl libc does not have this symbol and it is not used in golang sanitizers so I did no see a reason to keep it. But at the same time no strong reason to remove it either. I've dropped this part of the patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75849/new/

https://reviews.llvm.org/D75849