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/go: ignore *.syso files in -msan mode #21884

Open
rsc opened this issue Sep 14, 2017 · 9 comments
Open

cmd/go: ignore *.syso files in -msan mode #21884

rsc opened this issue Sep 14, 2017 · 9 comments
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Sep 14, 2017

Packages with syso files currently fail in -msan mode because the syso file calls out to a routine like memcmp which then falsely reports uninitialized memory.

It's not possible to prepare a syso file that works regardless of memory-sanitizer setting, at least not if the code makes any memory writes, which most code does. Instead you'd have to prepare both a standard syso and an msan-enabled syso, and the go command conventions provide no way to tell those apart.

I suggest we simply ignore syso files in -msan mode. That will result in missing link symbols, but then package authors can revise their packages to avoid the syso in -msan mode. That gives package authors at least one way to write a package that has a syso and still works with -msan. Today there are zero ways.

A more complex change would be to introduce a new class of objects like *.syso.msan or *_msan.syso, but let's not do that until there's a compelling reason. In the case where this came up, the "ignore syso files" is good enough.

@rsc rsc added this to the Go1.10 milestone Sep 14, 2017
@gopherbot
Copy link

Change https://golang.org/cl/63917 mentions this issue: [dev.boringcrypto] cmd/go: exclude SysoFiles when using -msan

gopherbot pushed a commit that referenced this issue Sep 18, 2017
There's no way for a *.syso file to be compiled to work both in
normal mode and in msan mode. Assume they are compiled in
normal mode and drop them in msan mode.

Packages with syso files currently fail in -msan mode because
the syso file calls out to a routine like memcmp which then
falsely reports uninitialized memory. After this CL, they will fail
in -msan with link errors, because the syso will not be used.
But then it will at least be possible for package authors to write
fallback code in the package that avoids the syso in -msan mode,
so that the package with the syso can at least run in both modes.
Without this CL, that's not possible.

See #21884.

Change-Id: I77340614c4711325032484e65fa9c3f8332741d5
Reviewed-on: https://go-review.googlesource.com/63917
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/65488 mentions this issue: [dev.boringcrypto.go1.8] cmd/go: exclude SysoFiles when using -msan

gopherbot pushed a commit that referenced this issue Sep 22, 2017
There's no way for a *.syso file to be compiled to work both in
normal mode and in msan mode. Assume they are compiled in
normal mode and drop them in msan mode.

Packages with syso files currently fail in -msan mode because
the syso file calls out to a routine like memcmp which then
falsely reports uninitialized memory. After this CL, they will fail
in -msan with link errors, because the syso will not be used.
But then it will at least be possible for package authors to write
fallback code in the package that avoids the syso in -msan mode,
so that the package with the syso can at least run in both modes.
Without this CL, that's not possible.

See #21884.

Change-Id: I77340614c4711325032484e65fa9c3f8332741d5
Reviewed-on: https://go-review.googlesource.com/63917
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/65488
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@rsc rsc modified the milestones: Go1.10, Go1.11 Dec 1, 2017
@FiloSottile
Copy link
Contributor

@rsc any reason not to cherry-pick on master (with docs)?

@ManPython
Copy link

ManPython commented May 29, 2018

There is instruction how to add .icon based on .syso and not working
https://github.com/therecipe/qt/wiki/Setting-the-Application-Icon#setting-the-icon-on-windows

In any case of .syso looks not working
go1.10.2.windows-amd64
w7 x64

@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 6, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jul 6, 2018
@bcmills bcmills added the GoCommand cmd/go label Nov 14, 2018
@bcmills bcmills modified the milestones: Go1.12, Go1.13 Nov 14, 2018
@rsc
Copy link
Contributor Author

rsc commented Nov 20, 2018

This is OK to cherry pick to master.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@odeke-em
Copy link
Member

odeke-em commented Oct 7, 2019

@FiloSottile @andybons @bradfitz, in #21884 (comment) Russ gave the go ahead to cherry pick from dev.boringcrypto. I'd do it but I don't have the forge-author Gerrit permission.

@ianlancetaylor
Copy link
Contributor

I'm concerned that people might use build tags to make this work today, which would stop working if we unilaterally ignore .syso files in msan mode.

@FiloSottile
Copy link
Contributor

How would using build tags work?

@ianlancetaylor
Copy link
Contributor

file_linux.syso
file_darwin.syso

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

8 participants