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

Jump to bottom

cmd/vet: +build comment error is confusingly worded #31410

Open
navytux opened this issue Apr 11, 2019 · 5 comments
Open

cmd/vet: +build comment error is confusingly worded #31410

navytux opened this issue Apr 11, 2019 · 5 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@navytux
Copy link
Contributor

navytux commented Apr 11, 2019

Please answer these questions before submitting your issue. Thanks!

What did you do?

Hello up there. I was updating my tracing/xruntime package for Go1.12 and hit test error:

.../src/lab.nexedi.com/kirr/go123/tracing/internal/xruntime$ go test
# lab.nexedi.com/kirr/go123/tracing/internal/xruntime
./runtime_g_amd64.s:3:1: +build comment must appear before package clause and be followed by a blank line
FAIL    lab.nexedi.com/kirr/go123/tracing/internal/xruntime [build failed]

The error here complains about +build directive in assembly file:

---- 8< ---- (runtime_g_amd64.s)

#include "textflag.h"

// +build amd64 amd64p

// func getg() *g
TEXT ·getg(SB),NOSPLIT,$0-8
	MOVQ (TLS), R14
	MOVQ R14, ret+0(FP)
	RET

(https://lab.nexedi.com/kirr/go123/blob/7ee2de42/tracing/internal/xruntime/runtime_g_amd64.s)

It was working with Go1.11 and previous releases.

What did you expect to see?

Build and test succeed; test pass, as with e.g. Go1.11:

.../src/lab.nexedi.com/kirr/go123/tracing/internal/xruntime$ go1.11 test -v
=== RUN   TestStartStopTheWorld
--- PASS: TestStartStopTheWorld (1.00s)
PASS
ok      lab.nexedi.com/kirr/go123/tracing/internal/xruntime     1.003s

What did you see instead?

.../src/lab.nexedi.com/kirr/go123/tracing/internal/xruntime$ go1.12 test -v
# lab.nexedi.com/kirr/go123/tracing/internal/xruntime
./runtime_g_amd64.s:3:1: +build comment must appear before package clause and be followed by a blank line
FAIL    lab.nexedi.com/kirr/go123/tracing/internal/xruntime [build failed]

System details

go version go1.12.3 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/kirr/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/kirr/go"
GOPROXY=""
GORACE=""
GOROOT="/home/kirr/src/tools/go/go"
GOTMPDIR=""
GOTOOLDIR="/home/kirr/src/tools/go/go/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
GOROOT/bin/go version: go version go1.12.3 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.12.3
uname -sr: Linux 4.19.0-4-amd64
Distributor ID:	Debian
Description:	Debian GNU/Linux buster/sid
Release:	testing
Codename:	buster
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Debian GLIBC 2.28-8) stable release version 2.28.
gdb --version: GNU gdb (Debian 8.2.1-2) 8.2.1
navytux added a commit to navytux/go123 that referenced this issue Apr 11, 2019
@navytux
tracing/runtime: Try to add support for Go1.12
b46436b 
Generate g for Go 1.12; no changes here compared to Go 1.11, however
go1.12 build build fails because of +build in .s file:

	# lab.nexedi.com/kirr/go123/tracing/internal/xruntime
	./runtime_g_amd64.s:3:1: +build comment must appear before package clause and be followed by a blank line

Build problem reported upstream:

	golang/go#31410
@navytux
Copy link
Contributor Author

navytux commented Apr 11, 2019

Thanks beforehand for looking into this,
Kirill

@bcmills
Copy link
Contributor

bcmills commented Apr 11, 2019

CC @jayconrod

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 11, 2019
@bcmills bcmills added this to the Go1.13 milestone Apr 11, 2019
@rsc
Copy link
Contributor

rsc commented Apr 12, 2019

The error could be improved (obviously there is no package clause), but it is correct to be complaining. The // +build line in this file has no effect. You should move it up above the #include.

A more appropriate error would be

./runtime_g_amd64.s:3:1: +build comment must appear before first non-comment source line

and the "blank line" should be dropped in this instance. When the blank line is relevant the message should be

./runtime_g_amd64.s:3:1: blank line must separate +build comment from first non-comment source line

@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 12, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 12, 2019
@rsc rsc changed the title cmd/go: .s: +build comment must appear before package clause and be followed by a blank line (regression) cmd/vet: +build comment error is confusingly worded Apr 12, 2019
navytux added a commit to navytux/go123 that referenced this issue Apr 12, 2019
@navytux
tracing/runtime: Fix build for Go1.12
5a2cf11 
This continues b46436b (tracing/runtime: Try to add support for Go1.12):

Move `+build ...` constraint in assembly files to the top, as suggested
by Russ Cox:

	golang/go#31410 (comment)

Then go stops complaining about it.

The build constraint had no effect previously, but it was not caught
because getg is used only on race builds and I was testing on amd64
only, which is practically almost the only ISA supported by race
detector as of Go1.11 .
@navytux
Copy link
Contributor Author

navytux commented Apr 12, 2019

@rsc thanks for feedback. I confirm that moving +build ... to the top fixed my package for Go1.12. Good idea to improve the error message.

@bcmills
Copy link
Contributor

bcmills commented Apr 12, 2019

@ianthehat, how do you want us to triage cmd/vet issues?

(@alandonovan is still listed as the primary owner on dev.golang.org/owners, but I'm guessing there might be someone else on your team stepping up for vet maintenance?)

CC @josharian @mvdan (secondary owners)

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@bcmills bcmills modified the milestones: Go1.14, Unplanned Sep 18, 2019
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) 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
6 participants
@rsc @andybons @navytux @bcmills @adonovan @gopherbot