Skip to content

cmd/go: go build -msan passes invalid option to GCC #25175

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.

Sign up for GitHub

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

Jump to bottom
Closed
daenney opened this issue Apr 30, 2018 · 6 comments
Closed

cmd/go: go build -msan passes invalid option to GCC #25175

daenney opened this issue Apr 30, 2018 · 6 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@daenney
Copy link

daenney commented Apr 30, 2018 •
edited
Loading

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

  • 1.9.5
  • 1.10.1

Does this issue reproduce with the latest release?

Yes

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

Linux, amd64

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"

What did you do?

  • Cloned a project that uses CGO, prometheus/node_exporter
  • Attempted to build with go build -msan

What did you expect to see?

One of:

  • The build proceeding as normal
  • The build outputting a user friendly error message notifying that -msan does not work with GCC (because GCC doesn't support -fsanitize=memory)

What did you see instead?

# runtime/cgo
gcc: error: unrecognized argument to -fsanitize= option: 'memory'

Though go env correctly detected CC=gcc and CXX=g++ it seems go build indiscriminately passes -fsantize=memory to the C compiler when calling go build with -msan. This should not be the case, as -fsanitize=memory is only available when using clang. Instead it should warn the invoker of go build about this with a friendly error message when go env CC is gcc.

The simple fix in this case was to env CC=clang env CXX=clang++ go build -msan.

$ gcc --version
gcc (GCC) 7.3.1 20180406
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ clang --version
clang version 6.0.0 (tags/RELEASE_600/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

MemorySaniziter is only supported on Linux with amd64, arm64 and mips64 according to the LLVM documentation, which might be worth checking for too (at least GOOS=linux seems like a pretty safe check)
@andybons andybons changed the title go build -msan passes invalid option to GCC cmd/go: go build -msan passes invalid option to GCC Apr 30, 2018
@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 30, 2018
@andybons andybons added this to the Unplanned milestone Apr 30, 2018
@andybons
Copy link
Member

@ianlancetaylor @bcmills

@ianlancetaylor
Copy link
Member

We should not ignore the -msan option with a compiler that does not support -fsanitize=memory, so it seems to me that all we are discussing is the possibility of a better error message.

What is your suggested fix? Although current versions of GCC does not support -fsanitize=memory, later versions may support it. Although current versions of clang only support -fsanitize=memory on amd64, arm64, mips64, and only on GNU/Linux, later versions of clang may support it on other targets.

@daenney
Copy link
Author

daenney commented May 1, 2018 •
edited
Loading

I'm not quite sure what a good solution is here. It's indeed possible that in newer releases these features will be supported. Do GCC/clang have any facilities that would allow to check whether a specific option is supported for an argument? So far all I've been able to get is gcc --help=common,joined but contrary to an option like ftls-model which als documents the possible options it doesn't do that for fsanitize.

Alternatively, is there a way we can wrap the error in this case with some more context? I don't think pattern matching on the error itself would be a very stable solution, but given that we know -msan can cause execution to fail, maybe it's enough to include that in the error message as a pointer to what might be the cause?

@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels May 1, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 1, 2018
@bcmills
Copy link
Contributor

bcmills commented May 1, 2018

@daenney, we do probe for a working sanitizer in some of the cgo tests today, but the check is somewhat involved and also adds latency to the test. (Also consider that it's possible for the compiler to accept -fsanitize=memory but fail to produce a working binary anyway, particularly if the result is linked against a different libc.)

go/misc/cgo/testsanitizers/cc_test.go

Lines 315 to 353 in 7f6105f

func (c *config) checkCSanitizer() (skip bool, err error) {
dir, err := ioutil.TempDir("", c.sanitizer)
if err != nil {
return false, fmt.Errorf("failed to create temp directory: %v", err)
}
defer os.RemoveAll(dir)
src := filepath.Join(dir, "return0.c")
if err := ioutil.WriteFile(src, cMain, 0600); err != nil {
return false, fmt.Errorf("failed to write C source file: %v", err)
}
dst := filepath.Join(dir, "return0")
cmd, err := cc(c.cFlags...)
if err != nil {
return false, err
}
cmd.Args = append(cmd.Args, c.ldFlags...)
cmd.Args = append(cmd.Args, "-o", dst, src)
out, err := cmd.CombinedOutput()
if err != nil {
if bytes.Contains(out, []byte("-fsanitize")) &&
(bytes.Contains(out, []byte("unrecognized")) ||
bytes.Contains(out, []byte("unsupported"))) {
return true, errors.New(string(out))
}
return true, fmt.Errorf("%#q failed: %v\n%s", strings.Join(cmd.Args, " "), err, out)
}
if out, err := exec.Command(dst).CombinedOutput(); err != nil {
if os.IsNotExist(err) {
return true, fmt.Errorf("%#q failed to produce executable: %v", strings.Join(cmd.Args, " "), err)
}
snippet := bytes.SplitN(out, []byte{'\n'}, 2)[0]
return true, fmt.Errorf("%#q generated broken executable: %v\n%s", strings.Join(cmd.Args, " "), err, snippet)
}
return false, nil
}

@daenney
Copy link
Author

daenney commented May 1, 2018 •
edited
Loading

Mmm, I suppose something along those lines would work, though if it slows things down by much that might be annoying. Though I'm not exactly clear on how the check on the error message there would have much of a perf impact/slow things down?

(Also consider that it's possible for the compiler to accept -fsanitize=memory but fail to produce a working binary anyway, particularly if the result is linked against a different libc.)

That's true, but I wouldn't expect that to throw the error this bug is about. That seems like a fairly esoteric condition to me, though I suppose anything is possible.

I'm also fine with leaving it as is. In theory this bug report would probably surface now if someone entered the specified error in a search engine paired with the go/golang keyword, hopefully providing the necessary context and workaround. I was hoping there was an easy way to improve the reporting around that error. It's not a very common flag to pass, in this case I was just attempting to debug a potentially cgo related issue which is hopefully not a common activity in gopher's daily lives.

@bcmills
Copy link
Contributor

bcmills commented Jan 18, 2019

I'm also fine with leaving it as is.

Since nobody has gotten to it since 1.10.1, I suspect that no one will in the near future; closing.

@bcmills bcmills closed this as completed Jan 18, 2019
@golang golang locked and limited conversation to collaborators Jan 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@andybons @daenney @ianlancetaylor @bcmills @gopherbot