text:
proposal: add a go:noinline directive

We're thinking about adding a <go:noinline> directive to help with testing.  Currently, if you don't want a function inlined, you add a <switch{}> or a defer or other not-obviously-a-nop construct to the function.  We'd like to introduce a new directive called <go:noinline> to mark such functions, as the old way depends on a consistent definition of nop-obviousness (which we probably don't want to commit to).
We particularly need this feature on the SSA branch because if a function is inlined, the code contained in that function might switch from being SSA-compiled to old-compiler-compiled.  Without some sort of noinline mark the SSA-specific tests might not be testing the SSA backend at all.
It is possible that once everything is running under SSA this directive will be less useful, but I think it will be generically useful going forward.  Thus I'd like to discuss this now in anticipation of it being merged into mainline sometime soon (if it is accepted).
Implementation is pretty easy, https://go-review.googlesource.com/13911 if you're interested.

Usually the implementation comes later :)
Seems reasonable but makes me nervous.
I have a general issue about the proliferation of such things, as I fear the compiler guys will, as always, infect the language with annotations.

I am worried about proliferation here too but I certainly see the need for it repeatedly. It's okay with me. It's also okay for compilers to completely ignore it, like any other comment. :-)

Could we limit the new annotations somehow?
I also think //go:notinlined is a better name to avoid the confusion
mentioned by Keith.

Restating the same: worried about proliferation.
That said, I'm ok with the mechanism for testing. I think it should not be advertised widely. And ideally, it should not be available (meaning: have no effect) unless in some form of test scenario. I don't want people to start relying on it.

@minux said on golang-dev (moving here to keep the discussion in one place):

Ok, how about add a runtime/debug func DoNotInline() that technically is a no-op, but if called, will inhibit inlining of the enclosing function.
For now, we just need to implement that in assembly. Future improved compiler might need to detect it.
It is as clear as //go:noinline, but doesn't require changes to the compiler.

There are two reasons this is not as good as an annotation:

It moves it from gc-only, which is fairly hidden and to which the Go 1 compatibility promise doesn't apply, to the stdlib, where it is visible and plausibly creates expectations of cross-compiler support.
This annotation is mainly helpful for testing the runtime and compiler. Having to insert extra code into a function in order to achieve this is not ideal, because it means that the part of the runtime or compiler that we are interested has to be unimpacted by the insertion of such code. That's why dev.ssa currently uses the obviously-inlinable-but-not-yet-inlined switch {}: It compiles away to nothing, and the compiler doesn't yet know how to compile panics, defers, etc.

I think having this feature be test-only would be nice.  We don't see any clean way to do that, however.  The compiler doesn't know what is test code and what isn't.  We'd have to add a flag to the compiler and have the go tool set that flag for test builds.  That seems like a lot of ugly to keep people from using this feature where it doesn't belong.

We could use the toolchain experiment support (see cmd/internal/obj/go.go)
and add a noinline experiment which controls the new directive.
And then we can enable the experiment at build time by setting
GOEXPERIMENT=noinline.

@minux hmmm. How would work with using go:noinline in the runtime or test directory tests, which are run as part of all.bash? Would we set GOEXPERIMENT for the duration of all.bash? Would it cause confusion when someone manually ran 'go test runtime' and the tests failed? To echo Keith, it seems like that mechanism would cause more trouble than it is worth.

I assumed that after the completion of SSA branch, there won't
be a need for //go:noinline.
GOEXPERIMENT is being embedded into the compiler though.
so you only need to set it before running make.bash.

I think it'll be useful outside of the SSA branch.
A very cursory glance at the master branch suggests it'd be useful in pprof test's allocateTransient2M, runtime test's testIfaceEqual, the runtime's atomic* functions, fixedbugs/issue10441.go, fixedbugs/issue12133.go, and fixedbugs/issue8036.go. There are probably more. And it'd be a shame to lose the SSA tests when it gets merged into master and GOEXPERIMENT can no longer be set.

"Useful" is always true for a feature request. The question is, does the usefulness justify the cost? The cost here is continued proliferation of magic comments, which are becoming too numerous already.

My concern here is that a name like go:noinline sounds like something a developer could depend on. Maybe all that's needed is a different name or a qualification that tells the average developer to stay away or at least that the directive cannot be relied on, something like one of these:
//go:test noinline
//go:internal noinline

We already have:
//go:noescape
//go:nosplit
//go:nowritebarrier
//go:systemstack

Does the same argument not apply to all those? Especially to e.g. "noescape"? We going to rename those to //go:internal_no_seriously_i_mean_in nowritebarrier?
It's a comment. Anybody is free to ignore it. And it's an inlining option... it should not change semantics of the program. If it did, we'd have a bigger problem. So nobody should ever want to use it anyway, as it can only make things slower.
I think there's way too much discussion here (including this comment) for something trivial and internal-only which exists only to clean up some tests.

Only //go:noescape and //go:nosplit are publicly documented.
//go:nowritebarrier is internal and only allowed in runtime,
I proposed in #12454 to limit //go:systemstack to runtime
as well.
The other //go: directive is //go:linkname, which is used by
runtime and cgo, and it's also publicly documented.

Let's figure out why do we need to mark a function not
inlinable first. (I'm not talking about the dev.ssa branch, the
problem there is too specific to that branch.)
Right now, I think the only reason is because inlined function
won't appear in stack traces, and that is a bug of our back
tracer, and if we want to be able to inline more, we have to
solve the problem.
Is there any other reason I missed?

@minux another reason is because you need code to take a particular form to trigger a compiler bug, and preventing inlining allows a more minimal/reasonable form. See e.g. this from fixedbugs/issue10441.go:
func bar() {
	f := func() {}
	foo(&f)
}

func foo(f *func()) func() {
	defer func() {}() // prevent inlining of foo
	return *f
}

And with that, I'm giving up arguing for this. I thought it was a simple, unobtrusive, low visibility, low impact fix to an annoyance felt by people working on the compiler. It has ballooned to the point that it no longer feels worth the words.

Is that better achieved with the -l option?
Another compromise is to introduce a compiler option to enable
the new directive. It just shouldn't be available by default.
Actually, having //go:noinline will benefit some of my code, but
my analysis of those code shows all its use are to workaround
the stack trace problem I mentioned earlier. I'd rather have that
fixed than introducing an easier way for people to mask that
problem. As I stressed, we definitely need to fix the issue if we
want to enable more inlining.

I agree with Josh, and I am astonished at the amount of discussion generated for a flag that I actually expect most users to avoid except when it really is necessary for real live workarounds.  Imagine instead if it were //go:inline , and trying to stop people from using that inappropriately.
As a general rule, I will always favor an explicit comment-expressed directive over attempting to control the compiler by subtext.  We have "noinline" now.  We spell it "switch {}".  Is that documented?  Is that guaranteed to work in the future?  Is a random Go programmer going to know what that means, and leave it alone?

Introducing a new directive for testing the compiler is one thing,
introducing a new directive for general use is another.
I don't think we should add more generally available directives
lightly. (I just removed one, //go:systemstack)
People might not use //go:noinline very often, but outside of
compiler testing, I see all its use as workaround of a particular
issue which we should solve instead.
Right now there is no convincing argument that explains why
exposing //go:noinline without limitation is necessary.

As an example where compiler improvements have changed the intent of a test with respect to inlining, see race/testdata/regression_test.go:
func NewImage() Image {
          var pleaseDoNotInlineMe stack
          pleaseDoNotInlineMe.push(1)
          _ = pleaseDoNotInlineMe.pop()
          return Image{}
  }

I tested with go1.2 and the stack push/pop prevented inline.  However, I just built that code and it no longer does.  The test is no longer testing what it was intended to test due to the implicit prevention of inlining.

Minux, I understand that you disagree, and for good ideal reasons, but pragmatically I think we should move forward with //go:noinline. Again, compilers in other toolchains will always be free to ignore it, just as they are free to ignore any comments.
As for the specific name, "noinline" matches the other directives. I don't think we should introduce a second naming convention at this point.

I think there's sufficient support in favor of this proposal that it should be accepted.

authors: adg bradfitz dr2chase griesemer josharian minux randall77 robpike rsc tzneal

repo: golang/go


text:
cmd/compile: BLAS regressions at tip

Comparing go1.8 with go version devel +b9f6b22 Fri Mar 17 13:59:31 2017 +0000 darwin/amd64
For all the benchmarks, the package is The github.com/gonum/blas/native. Below, the noasm tag makes the code pure go, rather than the ASM implementations of ddot and daxpy (which use SIMD and thus make the code significantly faster).
The first benchmark is Daxpy. Command is go test -bench Daxpy -tags noasm -count 5
brendan:~/Documents/mygo/src/github.com/gonum$ benchstat daxpy18.txt dgaxpytip.txt
name                      old time/op  new time/op   delta
DaxpySmallBothUnitary-8   21.8ns ± 2%   23.2ns ± 6%   +6.80%  (p=0.008 n=5+5)
DaxpySmallIncUni-8        25.6ns ± 1%   29.7ns ± 1%  +16.08%  (p=0.008 n=5+5)
DaxpySmallUniInc-8        24.8ns ± 1%   29.0ns ± 4%  +17.00%  (p=0.008 n=5+5)
DaxpySmallBothInc-8       25.3ns ± 1%   28.6ns ± 1%  +13.04%  (p=0.008 n=5+5)
DaxpyMediumBothUnitary-8   872ns ± 1%   1153ns ± 0%  +32.14%  (p=0.008 n=5+5)
DaxpyMediumIncUni-8       1.07µs ± 1%   1.17µs ± 1%   +9.10%  (p=0.008 n=5+5)
DaxpyMediumUniInc-8        928ns ± 1%   1162ns ± 1%  +25.12%  (p=0.008 n=5+5)
DaxpyMediumBothInc-8      1.15µs ± 1%   1.22µs ± 1%   +6.68%  (p=0.008 n=5+5)
DaxpyLargeBothUnitary-8   88.6µs ± 1%  116.5µs ± 4%  +31.57%  (p=0.008 n=5+5)
DaxpyLargeIncUni-8         181µs ± 1%    185µs ± 2%   +2.35%  (p=0.016 n=5+5)
DaxpyLargeUniInc-8         140µs ± 1%    146µs ± 2%   +4.20%  (p=0.008 n=5+5)
DaxpyLargeBothInc-8        294µs ± 1%    296µs ± 1%     ~     (p=0.310 n=5+5)
DaxpyHugeBothUnitary-8    13.2ms ± 1%   14.2ms ± 2%   +8.00%  (p=0.008 n=5+5)
DaxpyHugeIncUni-8         30.5ms ± 3%   30.2ms ± 1%     ~     (p=0.310 n=5+5)
DaxpyHugeUniInc-8         29.3ms ± 1%   29.1ms ± 1%     ~     (p=0.310 n=5+5)
DaxpyHugeBothInc-8        46.6ms ± 2%   46.3ms ± 1%     ~     (p=0.222 n=5+5)

There are significant slowdowns at the smaller sizes, though they disappear at the larger sizes. I'm not sure these explain the regressions we're seeing in Dgemv, so it may be a separate issue.
The command is go test -bench Dgemv -tags noasm -count 5.
brendan:~/Documents/mygo/src/github.com/gonum$ benchstat dgemv18.txt dgemvtip.txt
name                      old time/op  new time/op   delta
DgemvSmSmNoTransInc1-8     171ns ± 0%    179ns ± 1%   +4.81%  (p=0.008 n=5+5)
DgemvSmSmNoTransIncN-8     206ns ± 1%    228ns ± 0%  +10.66%  (p=0.008 n=5+5)
DgemvSmSmTransInc1-8       209ns ± 0%    220ns ± 1%   +5.07%  (p=0.016 n=4+5)
DgemvSmSmTransIncN-8       221ns ± 1%    257ns ± 1%  +16.17%  (p=0.008 n=5+5)
DgemvMedMedNoTransInc1-8  9.93µs ± 3%   9.90µs ± 4%     ~     (p=0.841 n=5+5)
DgemvMedMedNoTransIncN-8  10.1µs ± 1%   12.9µs ± 1%  +27.62%  (p=0.008 n=5+5)
DgemvMedMedTransInc1-8    10.7µs ± 1%   12.8µs ± 1%  +19.38%  (p=0.008 n=5+5)
DgemvMedMedTransIncN-8    11.4µs ± 1%   13.9µs ± 1%  +22.11%  (p=0.008 n=5+5)
DgemvLgLgNoTransInc1-8     919µs ± 1%    922µs ± 2%     ~     (p=0.841 n=5+5)
DgemvLgLgNoTransIncN-8     937µs ± 2%   1219µs ± 2%  +30.12%  (p=0.008 n=5+5)
DgemvLgLgTransInc1-8       948µs ± 1%   1236µs ± 1%  +30.29%  (p=0.008 n=5+5)
DgemvLgLgTransIncN-8       988µs ± 1%   1223µs ± 1%  +23.85%  (p=0.008 n=5+5)
DgemvLgSmNoTransInc1-8    14.4µs ± 1%   14.5µs ± 1%     ~     (p=0.421 n=5+5)
DgemvLgSmNoTransIncN-8    17.4µs ± 1%   20.2µs ± 1%  +15.57%  (p=0.008 n=5+5)
DgemvLgSmTransInc1-8      17.6µs ± 2%   18.3µs ± 2%   +4.18%  (p=0.008 n=5+5)
DgemvLgSmTransIncN-8      18.0µs ± 1%   22.4µs ± 2%  +24.35%  (p=0.008 n=5+5)
DgemvSmLgNoTransInc1-8    8.65µs ± 1%   8.76µs ± 1%   +1.21%  (p=0.032 n=5+5)
DgemvSmLgNoTransIncN-8    8.74µs ± 1%  11.64µs ± 1%  +33.15%  (p=0.008 n=5+5)
DgemvSmLgTransInc1-8      9.36µs ± 1%  12.15µs ± 1%  +29.83%  (p=0.008 n=5+5)
DgemvSmLgTransIncN-8      10.1µs ± 0%   12.5µs ± 1%  +24.03%  (p=0.008 n=5+5)

As we see, here some of the largest regressions are at the larger sizes.  These additionally work to about a 15% regression on Dgemm for almost all sizes, and about a 15% regression for the yet higher-level Cholesky decomposition.

Looks like there are some extra reg->reg moves in tip compared to 1.8.
I strongly suspect the introduction of MOVSDmem ops.  Previously we had
    m = some constant
    for ... {
        x = load ...
        x = m * x
    }

which works fine with no reg moves.  After introducing MOVSDmem ops, we have
    m = some constant
    for ... {
        x = m * load ...
    }

The op for x = m * load ... requires its output register (for x) to be the same as the input register (m).  So there are 2 additional register moves, one to copy m to another register and one to move it back.  The copy back then requires an additional branch instruction.
@TocarIP
source code:
func f(dst []float64, alpha float64, x, y []float64) {
	for i, v := range x {
		dst[i] = alpha*v + y[i]
	}
}

1.8:
	0x0038 00056 (/Users/khr/go/tmp1.go:4)	MOVSD	(DI), X1
	0x003c 00060 (/Users/khr/go/tmp1.go:5)	MULSD	X0, X1
	0x0040 00064 (/Users/khr/go/tmp1.go:5)	CMPQ	R8, BX
	0x0043 00067 (/Users/khr/go/tmp1.go:5)	JCC	$0, 111
	0x0045 00069 (/Users/khr/go/tmp1.go:5)	MOVSD	(SI)(R8*8), X2
	0x004b 00075 (/Users/khr/go/tmp1.go:5)	ADDSD	X2, X1
	0x004f 00079 (/Users/khr/go/tmp1.go:5)	CMPQ	R8, DX
	0x0052 00082 (/Users/khr/go/tmp1.go:5)	JCC	$0, 111
	0x0054 00084 (/Users/khr/go/tmp1.go:5)	MOVSD	X1, (CX)(R8*8)
	0x005a 00090 (/Users/khr/go/tmp1.go:4)	ADDQ	$8, DI
	0x005e 00094 (/Users/khr/go/tmp1.go:4)	INCQ	R8
	0x0061 00097 (/Users/khr/go/tmp1.go:4)	CMPQ	R8, AX
	0x0064 00100 (/Users/khr/go/tmp1.go:4)	JLT	$0, 56

tip:
	0x0038 00056 (/Users/khr/go/tmp1.go:5)	MOVUPS	X0, X1
	0x003b 00059 (/Users/khr/go/tmp1.go:5)	MULSD	(DI), X0
	0x003f 00063 (/Users/khr/go/tmp1.go:5)	CMPQ	R8, SI
	0x0042 00066 (/Users/khr/go/tmp1.go:5)	JCC	$0, 122
	0x0044 00068 (/Users/khr/go/tmp1.go:5)	MOVSD	(CX)(R8*8), X2
	0x004a 00074 (/Users/khr/go/tmp1.go:5)	ADDSD	X2, X0
	0x004e 00078 (/Users/khr/go/tmp1.go:5)	CMPQ	R8, BX
	0x0051 00081 (/Users/khr/go/tmp1.go:5)	JCC	$0, 115
	0x0053 00083 (/Users/khr/go/tmp1.go:5)	MOVSD	X0, (DX)(R8*8)
	0x0059 00089 (/Users/khr/go/tmp1.go:4)	ADDQ	$8, DI
	0x005d 00093 (/Users/khr/go/tmp1.go:4)	INCQ	R8
	0x0060 00096 (/Users/khr/go/tmp1.go:4)	CMPQ	R8, AX
	0x0063 00099 (/Users/khr/go/tmp1.go:4)	JGE	$0, 106
	0x0065 00101 (/Users/khr/go/tmp1.go:5)	MOVUPS	X1, X0
	0x0068 00104 (/Users/khr/go/tmp1.go:4)	JMP	56

@btracey Please try the patch above and see if it fixes everything for you.

It seems to have partially helped, but not fixed the problem. I'm not sure I applied it correctly though (see #19619)
DaxpySmallBothUnitary-8   21.6ns ± 1%   22.3ns ± 1%   +3.15%  (p=0.008 n=5+5)
DaxpySmallIncUni-8        25.6ns ± 0%   27.8ns ± 1%   +8.75%  (p=0.016 n=4+5)
DaxpySmallUniInc-8        24.9ns ± 1%   27.4ns ± 2%  +10.13%  (p=0.008 n=5+5)
DaxpySmallBothInc-8       25.4ns ± 0%   27.8ns ± 1%   +9.61%  (p=0.008 n=5+5)
DaxpyMediumBothUnitary-8   880ns ± 1%    884ns ± 1%     ~     (p=0.341 n=5+5)
DaxpyMediumIncUni-8       1.07µs ± 1%   1.18µs ± 1%  +10.12%  (p=0.008 n=5+5)
DaxpyMediumUniInc-8        927ns ± 0%   1168ns ± 1%  +26.07%  (p=0.008 n=5+5)
DaxpyMediumBothInc-8      1.15µs ± 2%   1.24µs ± 4%   +8.47%  (p=0.008 n=5+5)
DaxpyLargeBothUnitary-8   88.2µs ± 1%   89.2µs ± 0%   +1.18%  (p=0.008 n=5+5)
DaxpyLargeIncUni-8         185µs ± 1%    189µs ± 1%   +2.45%  (p=0.008 n=5+5)
DaxpyLargeUniInc-8         142µs ± 1%    146µs ± 1%   +3.11%  (p=0.008 n=5+5)
DaxpyLargeBothInc-8        292µs ± 0%    304µs ± 1%   +4.25%  (p=0.008 n=5+5)
DaxpyHugeBothUnitary-8    13.3ms ± 1%   13.1ms ± 1%   -1.43%  (p=0.016 n=5+5)
DaxpyHugeIncUni-8         30.4ms ± 2%   30.5ms ± 1%     ~     (p=0.690 n=5+5)
DaxpyHugeUniInc-8         29.1ms ± 0%   29.3ms ± 1%     ~     (p=0.286 n=4+5)
DaxpyHugeBothInc-8        47.4ms ± 1%   46.6ms ± 1%   -1.60%  (p=0.032 n=5+5)
DgemvSmSmNoTransInc1-8     171ns ± 1%    203ns ± 0%  +19.09%  (p=0.008 n=5+5)
DgemvSmSmNoTransIncN-8     206ns ± 1%    228ns ± 1%  +10.37%  (p=0.008 n=5+5)
DgemvSmSmTransInc1-8       210ns ± 1%    212ns ± 2%     ~     (p=0.333 n=5+5)
DgemvSmSmTransIncN-8       222ns ± 1%    245ns ± 1%  +10.08%  (p=0.008 n=5+5)
DgemvMedMedNoTransInc1-8  9.68µs ± 1%  10.59µs ± 1%   +9.41%  (p=0.008 n=5+5)
DgemvMedMedNoTransIncN-8  10.1µs ± 1%   13.0µs ± 1%  +28.10%  (p=0.008 n=5+5)
DgemvMedMedTransInc1-8    10.8µs ± 1%   10.9µs ± 3%     ~     (p=0.151 n=5+5)
DgemvMedMedTransIncN-8    11.3µs ± 0%   13.9µs ± 1%  +22.81%  (p=0.008 n=5+5)
DgemvLgLgNoTransInc1-8     924µs ± 2%    934µs ± 1%     ~     (p=0.310 n=5+5)
DgemvLgLgNoTransIncN-8     929µs ± 1%   1212µs ± 1%  +30.49%  (p=0.008 n=5+5)
DgemvLgLgTransInc1-8       952µs ± 2%    957µs ± 2%     ~     (p=0.690 n=5+5)
DgemvLgLgTransIncN-8       974µs ± 0%   1212µs ± 1%  +24.38%  (p=0.008 n=5+5)
DgemvLgSmNoTransInc1-8    14.4µs ± 1%   17.5µs ± 1%  +21.47%  (p=0.008 n=5+5)
DgemvLgSmNoTransIncN-8    17.9µs ± 1%   19.9µs ± 1%  +11.48%  (p=0.008 n=5+5)
DgemvLgSmTransInc1-8      17.5µs ± 1%   17.5µs ± 0%     ~     (p=0.690 n=5+5)
DgemvLgSmTransIncN-8      18.0µs ± 0%   20.5µs ± 1%  +14.09%  (p=0.008 n=5+5)
DgemvSmLgNoTransInc1-8    8.70µs ± 1%   8.85µs ± 2%   +1.75%  (p=0.016 n=5+5)
DgemvSmLgNoTransIncN-8    8.72µs ± 1%  11.65µs ± 2%  +33.57%  (p=0.008 n=5+5)
DgemvSmLgTransInc1-8      9.41µs ± 0%   9.39µs ± 1%     ~     (p=0.421 n=5+5)
DgemvSmLgTransIncN-8      10.2µs ± 2%   12.7µs ± 1%  +24.70%  (p=0.008 n=5+5)

It looks like all of the Daxpy benchmarks got faster with this patch, even though there seems to still be some regression overall. The Dgemv benchmarks have a less clear story.

I've taken a closer look at DgemvMedMedNoTransIncN.  Its inner loop is gonum/internal/asm/f64/axpy.go:AxpyInc
There's no difference in the inner loop with CL38337.
The difference between 1.8 and tip is that we're no longer loop peeling the branch at the start of the loop.  See CL 36205 and issues #18977 #15837.
So tip looks like:
loop:
    CMPQ ...
    JGE exit
    ...
    JMP loop
exit:

whereas 1.8 is:
    CMPQ ...
    JGE exit
loop:
    ...
    CMPQ ...
    JLT loop
exit:

I'm pretty surprised this matters at the ~30% slowdown level.  There might be something I'm missing.
In any case, we plan to fix this one for 1.9.

We might be able to get the effect of the loop peel just by changing the weights on block ordering -- for loops where the header block has a loop exit edge, make the branch to the header be lower than the backedge to the header.

@dr2chase I feel like that would interact badly with the register allocator.  If you put the loop header block last, then the 2nd block of the loop appears first in the schedule.  But none of its predecessors have been processed yet, so the register allocator won't know what register state to start with.  It could be fixed, I suppose, computing a regalloc schedule instead of just doing blocks in order.
It would generate:
    JMP entry
loop:
    ...
entry:
    CMPQ ...
    JLT loop
exit:

Which isn't quite the same as 1.8 either.  Better or worse?  I'm not sure.
It definitely has the appeal of doing less CFG surgery.

When I did the explicit loop peel to fix i,e:=range-of-slice loops in the presence of backedge rescheduling checks, PPC did get a little faster.

@btracey would you mind re-measuring with the latest tip, to assess whether this still might need fixes for 1.9? Thanks.

These are my new numbers, doing as best I can to control for noise
brendan:~/Documents/mygo/src/github.com/gonum$ benchstat blasbench181.txt blasbenchtip.txt
name                      old time/op  new time/op   delta
DaxpySmallBothUnitary-8   24.0ns ± 2%   25.2ns ± 1%   +5.09%  (p=0.008 n=5+5)
DaxpySmallIncUni-8        28.0ns ± 2%   32.5ns ± 1%  +16.06%  (p=0.008 n=5+5)
DaxpySmallUniInc-8        26.9ns ± 2%   31.1ns ± 2%  +15.93%  (p=0.008 n=5+5)
DaxpySmallBothInc-8       27.8ns ± 3%   31.5ns ± 1%  +13.37%  (p=0.008 n=5+5)
DaxpyMediumBothUnitary-8   974ns ± 1%   1275ns ± 2%  +30.95%  (p=0.008 n=5+5)
DaxpyMediumIncUni-8       1.21µs ± 1%   1.30µs ± 2%   +7.45%  (p=0.008 n=5+5)
DaxpyMediumUniInc-8       1.04µs ± 1%   1.28µs ± 4%  +23.19%  (p=0.008 n=5+5)
DaxpyMediumBothInc-8      1.31µs ± 1%   1.39µs ± 2%   +6.44%  (p=0.008 n=5+5)
DaxpyLargeBothUnitary-8   97.0µs ± 1%  125.1µs ± 1%  +28.92%  (p=0.008 n=5+5)
DaxpyLargeIncUni-8         213µs ± 2%    216µs ± 2%     ~     (p=0.310 n=5+5)
DaxpyLargeUniInc-8         156µs ± 1%    164µs ± 1%   +4.91%  (p=0.008 n=5+5)
DaxpyLargeBothInc-8        342µs ± 2%    345µs ± 1%     ~     (p=0.151 n=5+5)
DaxpyHugeBothUnitary-8    14.4ms ± 0%   15.8ms ± 2%  +10.06%  (p=0.008 n=5+5)
DaxpyHugeIncUni-8         32.9ms ± 2%   33.1ms ± 1%     ~     (p=0.310 n=5+5)
DaxpyHugeUniInc-8         31.5ms ± 2%   31.4ms ± 0%     ~     (p=0.690 n=5+5)
DaxpyHugeBothInc-8        50.3ms ± 1%   50.1ms ± 1%     ~     (p=0.841 n=5+5)
DgemvSmSmNoTransInc1-8     189ns ± 1%    199ns ± 1%   +5.62%  (p=0.008 n=5+5)
DgemvSmSmNoTransIncN-8     230ns ± 2%    251ns ± 1%   +9.22%  (p=0.008 n=5+5)
DgemvSmSmTransInc1-8       208ns ± 1%    243ns ± 3%  +16.70%  (p=0.008 n=5+5)
DgemvSmSmTransIncN-8       243ns ± 1%    281ns ± 2%  +15.62%  (p=0.008 n=5+5)
DgemvMedMedNoTransInc1-8  10.5µs ± 2%   10.7µs ± 2%   +1.91%  (p=0.032 n=5+5)
DgemvMedMedNoTransIncN-8  11.7µs ± 1%   14.2µs ± 1%  +21.31%  (p=0.008 n=5+5)
DgemvMedMedTransInc1-8    11.1µs ± 1%   13.9µs ± 3%  +25.43%  (p=0.008 n=5+5)
DgemvMedMedTransIncN-8    12.5µs ± 2%   15.3µs ± 1%  +21.95%  (p=0.008 n=5+5)
DgemvLgLgNoTransInc1-8    1.01ms ± 1%   1.02ms ± 2%     ~     (p=0.222 n=5+5)
DgemvLgLgNoTransIncN-8    1.03ms ± 1%   1.33ms ± 1%  +28.85%  (p=0.008 n=5+5)
DgemvLgLgTransInc1-8      1.02ms ± 2%   1.33ms ± 1%  +31.13%  (p=0.008 n=5+5)
DgemvLgLgTransIncN-8      1.08ms ± 1%   1.32ms ± 2%  +23.03%  (p=0.008 n=5+5)
DgemvLgSmNoTransInc1-8    15.2µs ± 2%   15.8µs ± 2%   +4.34%  (p=0.008 n=5+5)
DgemvLgSmNoTransIncN-8    20.0µs ± 1%   21.8µs ± 1%   +8.88%  (p=0.008 n=5+5)
DgemvLgSmTransInc1-8      17.0µs ± 2%   19.7µs ± 1%  +15.70%  (p=0.008 n=5+5)
DgemvLgSmTransIncN-8      20.2µs ± 1%   23.7µs ± 1%  +17.41%  (p=0.008 n=5+5)
DgemvSmLgNoTransInc1-8    9.56µs ± 2%   9.56µs ± 1%     ~     (p=1.000 n=5+5)
DgemvSmLgNoTransIncN-8    9.63µs ± 1%  12.88µs ± 2%  +33.75%  (p=0.008 n=5+5)
DgemvSmLgTransInc1-8      10.2µs ± 1%   13.5µs ± 1%  +32.64%  (p=0.008 n=5+5)
DgemvSmLgTransIncN-8      11.1µs ± 1%   13.8µs ± 2%  +24.56%  (p=0.008 n=5+5)
DgemmSmSmSm-8             1.90µs ± 2%   2.27µs ± 1%  +19.78%  (p=0.008 n=5+5)
DgemmMedMedMed-8           521µs ± 4%    625µs ± 2%  +19.87%  (p=0.008 n=5+5)
DgemmMedLgMed-8           3.67ms ± 0%   4.39ms ± 1%  +19.68%  (p=0.008 n=5+5)
DgemmLgLgLg-8              350ms ± 2%    422ms ± 1%  +20.73%  (p=0.008 n=5+5)
DgemmLgSmLg-8             5.53ms ± 4%   6.61ms ± 1%  +19.56%  (p=0.008 n=5+5)
DgemmLgLgSm-8             3.71ms ± 2%   4.60ms ± 4%  +23.90%  (p=0.008 n=5+5)
DgemmHgHgSm-8              330ms ± 4%    401ms ± 2%  +21.51%  (p=0.008 n=5+5)
DgemmMedMedMedTNT-8        526µs ± 1%    655µs ± 2%  +24.55%  (p=0.008 n=5+5)
DgemmMedMedMedNTT-8        482µs ± 3%    513µs ± 1%   +6.35%  (p=0.008 n=5+5)
DgemmMedMedMedTT-8         629µs ± 5%    721µs ± 1%  +14.70%  (p=0.008 n=5+5)

Here are the current stats. Mostly better
DaxpySmallBothUnitary-8   19.5ns ± 1%  21.5ns ± 1%  +10.27%  (p=0.008 n=5+5)
DaxpySmallIncUni-8        25.9ns ± 1%  28.4ns ± 2%   +9.58%  (p=0.008 n=5+5)
DaxpySmallUniInc-8        25.5ns ± 1%  27.7ns ± 2%   +8.62%  (p=0.008 n=5+5)
DaxpySmallBothInc-8       25.1ns ± 1%  27.4ns ± 4%   +8.92%  (p=0.008 n=5+5)
DaxpyMediumBothUnitary-8   865ns ± 1%   861ns ± 1%     ~     (p=0.190 n=5+5)
DaxpyMediumIncUni-8       1.07µs ± 1%  1.08µs ± 2%     ~     (p=0.690 n=5+5)
DaxpyMediumUniInc-8        922ns ± 1%   915ns ± 1%     ~     (p=0.103 n=5+5)
DaxpyMediumBothInc-8      1.14µs ± 1%  1.15µs ± 1%     ~     (p=0.841 n=5+5)
DaxpyLargeBothUnitary-8   87.9µs ± 2%  87.3µs ± 3%     ~     (p=0.151 n=5+5)
DaxpyLargeIncUni-8         173µs ± 2%   174µs ± 2%     ~     (p=1.000 n=5+5)
DaxpyLargeUniInc-8         137µs ± 1%   138µs ± 1%     ~     (p=0.056 n=5+5)
DaxpyLargeBothInc-8        299µs ± 1%   287µs ± 2%   -4.03%  (p=0.008 n=5+5)
DaxpyHugeBothUnitary-8    12.9ms ± 1%  12.9ms ± 0%     ~     (p=0.841 n=5+5)
DaxpyHugeIncUni-8         29.9ms ± 1%  29.9ms ± 3%     ~     (p=0.841 n=5+5)
DaxpyHugeUniInc-8         28.8ms ± 1%  28.7ms ± 0%     ~     (p=0.111 n=5+4)
DaxpyHugeBothInc-8        46.1ms ± 2%  45.8ms ± 1%     ~     (p=0.151 n=5+5)
DgemmSmSmSm-8             1.73µs ± 1%  1.73µs ± 2%     ~     (p=0.690 n=5+5)
DgemmMedMedMed-8           469µs ± 2%   469µs ± 1%     ~     (p=0.841 n=5+5)
DgemmMedLgMed-8           3.20ms ± 2%  3.39ms ±14%     ~     (p=0.310 n=5+5)
DgemmLgLgLg-8              313ms ± 1%   317ms ± 1%   +1.58%  (p=0.008 n=5+5)
DgemmLgSmLg-8             4.85ms ± 2%  4.88ms ± 1%     ~     (p=0.690 n=5+5)
DgemmLgLgSm-8             3.38ms ± 3%  3.39ms ± 2%     ~     (p=0.841 n=5+5)
DgemmHgHgSm-8              299ms ± 2%   297ms ± 4%     ~     (p=0.690 n=5+5)
DgemmMedMedMedTNT-8        486µs ± 1%   483µs ± 4%     ~     (p=0.310 n=5+5)
DgemmMedMedMedNTT-8        448µs ± 1%   453µs ± 4%     ~     (p=0.690 n=5+5)
DgemmMedMedMedTT-8         556µs ± 1%   576µs ± 2%   +3.72%  (p=0.008 n=5+5)
DgemvSmSmNoTransInc1-8     168ns ± 1%   181ns ± 1%   +7.98%  (p=0.008 n=5+5)
DgemvSmSmNoTransIncN-8     207ns ± 1%   213ns ± 1%   +2.89%  (p=0.008 n=5+5)
DgemvSmSmTransInc1-8       188ns ± 1%   204ns ± 1%   +8.61%  (p=0.008 n=5+5)
DgemvSmSmTransIncN-8       222ns ± 1%   244ns ± 0%   +9.81%  (p=0.008 n=5+5)
DgemvMedMedNoTransInc1-8  9.43µs ± 1%  9.59µs ± 1%   +1.75%  (p=0.008 n=5+5)
DgemvMedMedNoTransIncN-8  10.6µs ± 2%  11.0µs ± 3%   +3.23%  (p=0.008 n=5+5)
DgemvMedMedTransInc1-8    10.0µs ± 2%   9.9µs ± 0%   -1.24%  (p=0.032 n=5+5)
DgemvMedMedTransIncN-8    11.2µs ± 0%  11.5µs ± 1%   +2.45%  (p=0.008 n=5+5)
DgemvLgLgNoTransInc1-8     909µs ± 1%   908µs ± 1%     ~     (p=0.690 n=5+5)
DgemvLgLgNoTransIncN-8     929µs ± 2%   929µs ± 1%     ~     (p=0.548 n=5+5)
DgemvLgLgTransInc1-8       920µs ± 1%   920µs ± 1%     ~     (p=1.000 n=5+5)
DgemvLgLgTransIncN-8       966µs ± 1%   977µs ± 1%   +1.19%  (p=0.016 n=5+5)
DgemvLgSmNoTransInc1-8    13.7µs ± 1%  14.7µs ± 1%   +6.91%  (p=0.008 n=5+5)
DgemvLgSmNoTransIncN-8    18.0µs ± 1%  18.0µs ± 1%     ~     (p=0.841 n=5+5)
DgemvLgSmTransInc1-8      15.2µs ± 1%  16.3µs ± 1%   +7.52%  (p=0.008 n=5+5)
DgemvLgSmTransIncN-8      18.4µs ± 1%  19.7µs ± 1%   +7.27%  (p=0.008 n=5+5)
DgemvSmLgNoTransInc1-8    8.93µs ± 1%  8.71µs ± 3%     ~     (p=0.056 n=5+5)
DgemvSmLgNoTransIncN-8    9.07µs ± 1%  9.03µs ± 1%     ~     (p=0.548 n=5+5)
DgemvSmLgTransInc1-8      9.59µs ± 1%  9.55µs ± 0%     ~     (p=0.341 n=5+5)
DgemvSmLgTransIncN-8      10.4µs ± 1%  10.3µs ± 1%     ~     (p=0.421 n=5+5)

But a few non-trivial regressions still. If you're up for it, would you mind patching in CLs 43293 and 43491 and see whether they help any? (No particular reason to think they will, other than that they have a significant effect on code layout in general.)

With both applied:
name                      old time/op  new time/op   delta
DaxpySmallBothUnitary-8   19.1ns ± 1%   19.7ns ± 1%   +3.03%  (p=0.008 n=5+5)
DaxpySmallIncUni-8        25.3ns ± 1%   28.8ns ± 1%  +13.65%  (p=0.008 n=5+5)
DaxpySmallUniInc-8        25.1ns ± 1%   25.7ns ± 1%   +2.47%  (p=0.008 n=5+5)
DaxpySmallBothInc-8       24.7ns ± 1%   26.5ns ± 1%   +7.19%  (p=0.008 n=5+5)
DaxpyMediumBothUnitary-8   846ns ± 1%    858ns ± 3%     ~     (p=0.222 n=5+5)
DaxpyMediumIncUni-8       1.05µs ± 1%   1.22µs ±11%  +16.51%  (p=0.008 n=5+5)
DaxpyMediumUniInc-8        902ns ± 1%   1134ns ± 1%  +25.75%  (p=0.008 n=5+5)
DaxpyMediumBothInc-8      1.13µs ± 1%   1.19µs ± 3%   +5.80%  (p=0.008 n=5+5)
DaxpyLargeBothUnitary-8   85.8µs ± 1%   93.2µs ± 4%   +8.69%  (p=0.016 n=4+5)
DaxpyLargeIncUni-8         164µs ± 1%    179µs ± 2%   +8.89%  (p=0.008 n=5+5)
DaxpyLargeUniInc-8         134µs ± 0%    141µs ± 1%   +4.79%  (p=0.008 n=5+5)
DaxpyLargeBothInc-8        288µs ± 2%    287µs ± 2%     ~     (p=1.000 n=5+5)
DaxpyHugeBothUnitary-8    12.8ms ± 1%   12.8ms ± 1%     ~     (p=0.421 n=5+5)
DaxpyHugeIncUni-8         29.4ms ± 1%   29.3ms ± 1%     ~     (p=0.310 n=5+5)
DaxpyHugeUniInc-8         28.5ms ± 1%   28.4ms ± 1%     ~     (p=0.421 n=5+5)
DaxpyHugeBothInc-8        45.5ms ± 2%   45.5ms ± 1%     ~     (p=0.841 n=5+5)
DgemmSmSmSm-8             1.70µs ± 2%   1.66µs ± 1%   -2.55%  (p=0.008 n=5+5)
DgemmMedMedMed-8           467µs ± 3%    471µs ± 3%     ~     (p=0.421 n=5+5)
DgemmMedLgMed-8           3.16ms ± 3%   3.20ms ± 3%     ~     (p=0.151 n=5+5)
DgemmLgLgLg-8              315ms ± 2%    314ms ± 0%     ~     (p=0.730 n=5+4)
DgemmLgSmLg-8             4.87ms ± 5%   4.81ms ± 1%     ~     (p=0.690 n=5+5)
DgemmLgLgSm-8             3.37ms ± 3%   3.33ms ± 1%     ~     (p=0.222 n=5+5)
DgemmHgHgSm-8              298ms ± 3%    299ms ± 5%     ~     (p=1.000 n=5+5)
DgemmMedMedMedTNT-8        485µs ± 1%    492µs ± 2%     ~     (p=0.095 n=5+5)
DgemmMedMedMedNTT-8        452µs ± 1%    448µs ± 2%     ~     (p=0.310 n=5+5)
DgemmMedMedMedTT-8         561µs ± 3%    603µs ± 2%   +7.56%  (p=0.008 n=5+5)
DgemvSmSmNoTransInc1-8     166ns ± 1%    168ns ± 1%   +1.69%  (p=0.032 n=5+5)
DgemvSmSmNoTransIncN-8     205ns ± 1%    202ns ± 1%   -1.27%  (p=0.032 n=5+5)
DgemvSmSmTransInc1-8       186ns ± 1%    182ns ± 1%   -2.15%  (p=0.008 n=5+5)
DgemvSmSmTransIncN-8       219ns ± 1%    220ns ± 0%     ~     (p=0.175 n=5+4)
DgemvMedMedNoTransInc1-8  9.35µs ± 2%   9.36µs ± 0%     ~     (p=0.690 n=5+5)
DgemvMedMedNoTransIncN-8  10.4µs ± 0%   10.0µs ± 1%   -4.68%  (p=0.008 n=5+5)
DgemvMedMedTransInc1-8    9.82µs ± 0%   9.77µs ± 1%   -0.52%  (p=0.032 n=5+5)
DgemvMedMedTransIncN-8    11.0µs ± 1%   12.4µs ± 1%  +12.07%  (p=0.008 n=5+5)
DgemvLgLgNoTransInc1-8     893µs ± 1%    888µs ± 1%     ~     (p=0.421 n=5+5)
DgemvLgLgNoTransIncN-8     908µs ± 1%    904µs ± 0%     ~     (p=0.151 n=5+5)
DgemvLgLgTransInc1-8       905µs ± 1%    898µs ± 1%     ~     (p=0.056 n=5+5)
DgemvLgLgTransIncN-8       951µs ± 0%   1158µs ± 1%  +21.80%  (p=0.008 n=5+5)
DgemvLgSmNoTransInc1-8    13.5µs ± 2%   13.4µs ± 1%     ~     (p=0.690 n=5+5)
DgemvLgSmNoTransIncN-8    17.8µs ± 2%   17.1µs ± 1%   -3.83%  (p=0.008 n=5+5)
DgemvLgSmTransInc1-8      15.0µs ± 0%   14.6µs ± 1%   -2.41%  (p=0.008 n=5+5)
DgemvLgSmTransIncN-8      18.0µs ± 1%   18.0µs ± 0%     ~     (p=0.738 n=5+5)
DgemvSmLgNoTransInc1-8    8.49µs ± 1%   8.44µs ± 1%     ~     (p=0.381 n=5+5)
DgemvSmLgNoTransIncN-8    8.56µs ± 1%   8.56µs ± 3%     ~     (p=0.690 n=5+5)
DgemvSmLgTransInc1-8      9.02µs ± 0%   9.11µs ± 2%   +0.91%  (p=0.016 n=5+5)
DgemvSmLgTransIncN-8      9.84µs ± 1%  12.11µs ± 2%  +23.10%  (p=0.008 n=5+5)
```

Thanks. Looks like those CLs are a mixed bag, maybe a bit worse on balance. To confirm, are these latest from 1.8 to tip+2 CLs? I ask because the before numbers e.g. for the last benchmark don't line up—10.4us two comments back vs 9.84us in the last comment. Iff you have all the benchmark files around, could you post the benchstat going from tip to tip+2CLs, maybe running with -geomean? Ta.

It was 1.8.1 to the most recent. I'm running the benchmarks on my laptop (which I've been using all day), so the benchmarks could shift a bit. This is why I re-ran the 1.8.1 results (and why they don't exactly match up).
-geomean doesn't do what I expected. It prints the exact same output as before, except with a [Geo mean]                41.0µs        42.2µs        +2.94% at the bottom (I ran benchstat -geomean blasbench181.txt blasbenchtip.txt , using the latest from golang.org/x/perf/cmd/benchstat)

Punting to 1.10.

source: gharchive/issue

license_type: permissive

license_source: github-api


text:
cmd/internal/obj/arm64: golang assembler handles arm64 instruction hlt/hvc/smc/brk/clrex incorrectly.

Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (go version)?
go version devel +59051da Fri Jun 16 06:05:57 2017 +0000 linux/arm64
What operating system and processor architecture are you using (go env)?
GOARCH="arm64"
GOBIN=""
GOEXE=""
GOHOSTARCH="arm64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/fanzha02/go"
GORACE=""
GOROOT="/home/fanzha02/work/asm_test/golang_us/golang_1/golang"
GOTOOLDIR="/home/fanzha02/work/asm_test/golang_us/golang_1/golang/pkg/tool/linux_arm64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build468264647=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
What did you do?
cd src/cmd/asm/internal/asm
add arm64enc.s test file
enable the ARM64Encoder test command
go test -run "ARM64Encoder"
arm64enc.s test file and endtoend_test.go you can load from this patch:
https://go-review.googlesource.com/c/46438/1
If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.
What did you expect to see?
go test successfully
What did you see instead?
the hlt/hvc/smc/brk/clrex test cases are failed

cd src/cmd/asm/internal/asm
add arm64enc.s test file
enable the ARM64Encoder test command
go test -run "ARM64Encoder"

Instead of throwing a big test in a pending CL, could you describe what the correct encoding should be, and what wrong encoding it currently generates? Thanks!

@cherrymui  The test result of current assembly is as below.
--- FAIL: TestARM64Encoder (0.01s)
endtoend_test.go:186: illegal combination 00172 (testdata/arm64enc.s:60)        BRK     $35943 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00268 (testdata/arm64enc.s:84)        CLREX   $4 ABCON0 NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00432 (testdata/arm64enc.s:130)       DCPS1   $11378 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00436 (testdata/arm64enc.s:131)       DCPS2   $10699 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00440 (testdata/arm64enc.s:132)       DCPS3   $24415 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00500 (testdata/arm64enc.s:147)       HLT     $65509 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00504 (testdata/arm64enc.s:148)       HVC     $61428 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00940 (testdata/arm64enc.s:306)       SMC     $37977 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00172 (testdata/arm64enc.s:60)        BRK     $35943 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00268 (testdata/arm64enc.s:84)        CLREX   $4 ABCON0 NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00432 (testdata/arm64enc.s:130)       DCPS1   $11378 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00436 (testdata/arm64enc.s:131)       DCPS2   $10699 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00440 (testdata/arm64enc.s:132)       DCPS3   $24415 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00500 (testdata/arm64enc.s:147)       HLT     $65509 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00504 (testdata/arm64enc.s:148)       HVC     $61428 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00940 (testdata/arm64enc.s:306)       SMC     $37977 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00172 (testdata/arm64enc.s:60)        BRK     $35943 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00268 (testdata/arm64enc.s:84)        CLREX   $4 ABCON0 NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00432 (testdata/arm64enc.s:130)       DCPS1   $11378 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00436 (testdata/arm64enc.s:131)       DCPS2   $10699 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00440 (testdata/arm64enc.s:132)       DCPS3   $24415 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00500 (testdata/arm64enc.s:147)       HLT     $65509 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00504 (testdata/arm64enc.s:148)       HVC     $61428 MOVCON NONE NONE, 4 0
endtoend_test.go:186: illegal combination 00940 (testdata/arm64enc.s:306)       SMC     $37977 MOVCON NONE NONE, 4 0
endtoend_test.go:228: 00172 (testdata/arm64enc.s:60)    BRK     $35943: have encoding 000020d4, want e08c31d4
endtoend_test.go:228: 00268 (testdata/arm64enc.s:84)    CLREX   $4: have encoding 5f3f03d5, want 5f3403d5
endtoend_test.go:228: 00432 (testdata/arm64enc.s:130)   DCPS1   $11378: have encoding 0100a0d4, want 418ea5d4
endtoend_test.go:228: 00436 (testdata/arm64enc.s:131)   DCPS2   $10699: have encoding 0200a0d4, want 6239a5d4
endtoend_test.go:228: 00440 (testdata/arm64enc.s:132)   DCPS3   $24415: have encoding 0300a0d4, want e3ebabd4
endtoend_test.go:228: 00500 (testdata/arm64enc.s:147)   HLT     $65509: have encoding 000040d4, want a0fc5fd4
endtoend_test.go:228: 00504 (testdata/arm64enc.s:148)   HVC     $61428: have encoding 020000d4, want 82fe1dd4
endtoend_test.go:228: 00940 (testdata/arm64enc.s:306)   SMC     $37977: have encoding 030000d4, want 238b12d4

authors: cherrymui zhangfannie


text:
gollvm: build fail on Ubuntu 16.04 in driver/Command.cpp

$ lsb_release -d
Description:	Ubuntu 16.04.4 LTS
$ gcc --version
gcc (Ubuntu 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609
$ cd llvm && $ git rev-parse HEAD
483861bc32381cb649c6d9be416bd23798586ad6
$ cd tools/gollvm && git rev-parse HEAD
7352f685d0a4c1b9ae1baeab2480416e8353b757

What did you do?
#!/usr/bin/env sh

set -e

git_clone() {
	git clone --depth 1 "$@";
}

rm -rf llvm build.opt

git_clone http://llvm.org/git/llvm.git llvm
git_clone https://go.googlesource.com/gollvm llvm/tools/gollvm
git_clone https://go.googlesource.com/gofrontend llvm/tools/gollvm/gofrontend
git_clone https://github.com/libffi/libffi.git llvm/tools/gollvm/libgo/libffi
git_clone https://github.com/ianlancetaylor/libbacktrace.git llvm/tools/gollvm/libgo/libbacktrace

mkdir build.opt
cd build.opt
cmake -DCMAKE_BUILD_TYPE=Debug -DLLVM_USE_LINKER=gold -G Ninja ../llvm
ninja -j 2

What did you expect to see?
Successful build.
What did you see instead?
[2363/3548] Building CXX object tools/gollvm/driver/CMakeFiles/LLVMDriverUtils.dir/Command.cpp.o
FAILED: /usr/bin/c++   -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/gollvm/driver -I/home/ainar/dev/gollvm/llvm/tools/gollvm/driver -I/usr/include/libxml2 -Iinclude -I/home/ainar/dev/gollvm/llvm/include -I/home/ainar/dev/gollvm/llvm/tools/gollvm/gofrontend/go -I/home/ainar/dev/gollvm/llvm/tools/gollvm/bridge -Itools/gollvm/external/install/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -g    -fno-exceptions -fno-rtti -MD -MT tools/gollvm/driver/CMakeFiles/LLVMDriverUtils.dir/Command.cpp.o -MF tools/gollvm/driver/CMakeFiles/LLVMDriverUtils.dir/Command.cpp.o.d -o tools/gollvm/driver/CMakeFiles/LLVMDriverUtils.dir/Command.cpp.o -c /home/ainar/dev/gollvm/llvm/tools/gollvm/driver/Command.cpp
/home/ainar/dev/gollvm/llvm/tools/gollvm/driver/Command.cpp: In member function ‘int gollvm::driver::Command::execute(std::__cxx11::string*)’:
/home/ainar/dev/gollvm/llvm/tools/gollvm/driver/Command.cpp:35:51: error: could not convert ‘((gollvm::driver::Command*)this)->gollvm::driver::Command::arguments_.llvm::SmallVector<const char*, 16u>::<anonymous>.llvm::SmallVectorImpl<const char*>::<anonymous>.llvm::SmallVectorTemplateBase<const char*, true>::<anonymous>.llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::data<const char*, void>()’ from ‘llvm::SmallVectorTemplateCommon<const char*, void>::pointer {aka const char**}’ to ‘llvm::ArrayRef<llvm::StringRef>’
                                    arguments_.data(),
                                                   ^
[2363/3548] Building CXX object tools/gollvm/driver/CMakeFiles/LLVMDriverUtils.dir/CompileGo.cpp.o
ninja: build stopped: subcommand failed.

Not sure if it's the right repo to report an issue on gollvm, but the readme there doesn't have a "Reporting Issues" doc.

I'll take a look

CC @thanm @cherrymui

This may be that there is some LLVM API change that gollvm hasn't caught up. We'll update it.
Meanwhile, building with an order version of LLVM may work.

Can confirm, the build is successful.

Thanks for verifying, and thanks for sending the report.

authors: ainar-g cherrymui ianlancetaylor thanm


text:
encoding/xml: handling of anonymous pointer fields is broken

This happens on both Go 1.10.4 and go version devel +c21ba224ec Sat Aug 25 23:59:43 2018 +0000 linux/amd64.
This playground link shows what encoding/json does when it encounters anonymous pointer fields which are nil: https://play.golang.org/p/FUox06lbRVn
As one can see, they're simply skipped. encoding/xml does something slightly different: https://play.golang.org/p/KWYc_IQR7p5
The XML package tries to print the zero values instead. Which seems fine in theory, but here's the first weird behavior - as you can see via the print statements, the encoded value has been modified in-place.
But here's an even worse problem - if the encoded value isn't addressable, encoding panics via reflect: https://play.golang.org/p/7ORK8Vw8o6Q
I think these two problems need to be fixed. I see two possibilities:

Behave like encoding/json, ignoring anonymous pointer fields which are nil.
Keep the existing behavior, encoding zero-values without modifying the anonymous fields in-place.

I personally strongly prefer option 1, as that would make encoding/xml consistent with encoding/json. Both are documented in the exact same way when it comes to anonymous fields, so I believe consistency should be important. Here are the relevant docs for JSON and XML respectively:

Anonymous struct fields are usually marshaled as if their inner exported fields were fields in the outer struct, subject to the usual Go visibility rules amended as described in the next paragraph.
an anonymous struct field is handled as if the fields of its value were part of the outer struct.

The only problem with option 1 is that this may break some existing programs, in which case option 2 may be better in Go 1.x. Although it would still be possible to achieve the existing behavior, if encoding/xml users changed their code to ensure that there aren't any nil anonymous pointer fields.
Whatever we settle for, the fix should be pretty simple - happy to put in the work.
cc @rsc @niemeyer @SamWhited @rogpeppe

Please also note that handling of these is broken in encoding/json at tip at the moment, which is how I ended up digging this bug. A fix for JSON is already up: https://go-review.googlesource.com/c/go/+/131376

Behave like encoding/json, ignoring anonymous pointer fields which are nil.

This is how I read the docs anyways and seems sane to me. It also makes the package more internally consistent: when the children of an anonymous field are promoted they're still behind a pointer so *T.A string is effectively promoted to A *string in my mind (which would be ignored if the pointer is nil).

Agreed. Moreover, trying to access the embedded field in plain Go would never give you an empty value - it would panic - so I think the current behavior is unexpected.

I ended up dropping the ball on this a bit. The good news is that I had a mostly working fix last year, which I'm going to rebase, clean up a bit, and send.
I still think that option 1 is the correct solution here, which is the fix I implemented. I'm not sure if this bug is worth fixing, or if it's the correct fix, but it's not really doing anything sitting in a local branch for a year :)

authors: SamWhited mvdan


text:
cmd/compile: unnecessary hash/eq functions for slice literals

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

$ go version
go version go1.12rc1 linux/amd64

Consider this package:
package bloat
var x []interface{}

$ go build -o bloat.a .
$ ls -l bloat.a
-rw-r--r-- 1 dneil primarygroup 778 Feb 27 15:23 bloat.a

Now consider the case where we initialize the var x:
package bloat
var x = []interface{}{0, 1, 2}

$ go build -o bloat.a .
$ ls -l bloat.a
-rw-r--r-- 1 dneil primarygroup 4030 Feb 27 15:28 bloat.a

The object file for the latter package contains eq and hash functions for [3]interface{}, which appear to account for much of the size increase:
$ go tool pack x bloat.a
$ go tool objdump -S bloat.a | grep ^TEXT
TEXT type..hash.[3]interface {}(SB) gofile..<autogenerated>
TEXT type..eq.[3]interface {}(SB) gofile..<autogenerated>

There is no need for these functions, however; the only use of [3]interface{} is to initialize a []interface{}, and there is no way to recover the array type from that slice.

I have a CL related to this: https://go-review.googlesource.com/c/go/+/151497/
It's buggy somehow - I haven't dug back in to what the issue was.
Presumably the whole [3]interface{} type, including algorithms, is dead-code eliminated when linking.

I was also noticing this and was looking into this for go1.13 and had made an issue related to this as the hash and eq is on the backing array #28639.
Also for performance which also improves the size for the non deadcode eliminated algs:
https://go-review.googlesource.com/c/go/+/148177

I just filed #30528 as a more generalized version of this problem. I imagine that its harder to resolve than this one here since it requires a wider-scope static analysis to prove certain facts.

authors: dsnet martisch neild randall77


text:
path/filepath: Join documentation inconsistencies with path.Join

This is a follow-up to #29875, which is closed.
Problems:

The fix was only applied to path.Join docs, but filepath.Join has the same problem and should be fixed too.
The new wording drops the sentence "all empty strings are ignored", leaving no clue that path.Join("", "foo") results in "foo" rather than "/foo".

New:

Join joins the argument's path elements into a single path, separating them with slashes. The result is Cleaned. However, if the argument list is empty or all its elements are empty, Join returns an empty string.

Old:

Join joins any number of path elements into a single path, adding a separating slash if necessary. The result is Cleaned; in particular, all empty strings are ignored.

I think it should be clear that Join can return the empty string, but it should be also clear that empty strings at the beginning don't make the path absolute.
While I'm at it, I'm also confused by path.Dir:

Dir returns all but the last element of path, typically the path's directory. After dropping the final element using Split, the path is Cleaned and trailing slashes are removed. If the path is empty, Dir returns ".". If the path consists entirely of slashes followed by non-slash bytes, Dir returns a single slash. In any other case, the returned path does not end in a slash.

It should probably read "If the path consists entirely of slashes possibly followed by non-slash bytes" or "If the path consists entirely of slashes followed by zero or more non-slash bytes", otherwise the last sentence ("In any other case ...") is false.

/cc @erutherford who worked on the updated documentation and @robpike, who owns path.

Sorry, I meant to respond to this over the weekend, but ended up being sick instead :)

I think it should be clear that Join can return the empty string, but it should be also clear that empty strings at the beginning don't make the path absolute.
You're right this could have been clearer. Any leading empty strings are ignored in path.Join. I can take a look at updating this later today and I can take a look at updating filepath.Join as well.

While I'm at it, I'm also confused by path.Dir:
I can take a look at updating this too.

@erutherford, thanks! No hurry, I'm never sure if people see my replies on closed issues...

Ok @toothrot and @pam4, I've got a CL up for the path.Join and filepath.Join changes.

@erutherford, "are" repetition typo in "Any leading empty elements are are ignored".
Your wording SGTM, but consider even just "Empty elements are ignored", since non leading ones don't make any difference after Clean.

@pam4 sorry, I thought it was code like a pirate day :)
I've corrected the the error and adopted your suggestion.

authors: erutherford pam4 toothrot


text:
crypto/x509: build tag "ios" broken at tip on darwin/amd64

In previous Go releases, the "ios" build tag with darwin/amd64 produced working binaries:
barloga5k:x509 $ go1.14.2 download
...
Downloaded 100.0% (125040726 / 125040726 bytes)
Unpacking /Users/bradfitz/sdk/go1.14.2/go1.14.2.darwin-amd64.tar.gz ...
Success. You may now run 'go1.14.2'
barloga5k:x509 $ GOARCH=amd64 go1.14.2 install --tags=ios crypto/x509
barloga5k:x509 $

(Such a configuration is used when running under the iOS simulator in Xcode)
But at Go tip:
barloga5k:x509 $ go version
go version devel +a7e9e84716 Mon Apr 27 20:20:53 2020 +0000 darwin/amd64
barloga5k:x509 $ go install --tags=ios crypto/x509
# crypto/x509
./cert_pool.go:65:9: undefined: loadSystemRoots
./root.go:21:32: undefined: loadSystemRoots

Looks like https://go-review.googlesource.com/c/go/+/227197 (da8591b61c141ca58e4b3eae382d2827006344fd, @aclements) dropped the // +build arm arm64 ios disjunction.
Labeling release-blocker in that it's a regression.

The ios build tag was only used in crypto/x509, and not documented anywhere I could find. Is "like iOS but on amd64" a supported configuration? Is crypto/x509 really the only thing that needs to support it?

I have no clue about the history other than it used to work and I stumbled into a project that was depending on it.

(fwiw, I was also surprised it ever worked)

Probably we just need to drop the !ios build constraint from root_cgo_darwin.go? I guess previously that was meant to be exclusive with root_darwin_armx.go, but I only dropped the positive build tag. That would just make it ignore the ios build tag.

@aclements, that might not work because cgo might be disabled in such an environment.
What I ended up doing for our build was renaming those files from "arm" to "embed" (in https://github.com/tailscale/go/commit/1f0b2d1000d3b2998054cf69c6ccded48601a8a0) and making the build tags be:
// +build !x509omitbundledroots
// +build darwin
// +build arm64 ios

@FiloSottile , what do you think is the right fix here? It should be easy to fix, I'm just not sure what the right way to do it is. :)

@FiloSottile Friendly ping as this is a Go 1.15 release blocking issue.

I have no clue about the history other than it used to work and I stumbled into a project that was depending on it.

@bradfitz Can you elaborate on how the project is depending on it? What was the expected behavior? If we can make it more clear what the mode represents and is supposed to do, it will make it easier to make progress here. I'll update the issue state to NeedsInvestigation because I don't think we know what the fix needs to be yet (as confirmed from discussion above).
While it's true this is a regression, if we take the potential plan for 1.15 described in https://github.com/golang/go/issues/38485#issuecomment-622504803 into account, we may want to deliberately make a change here anyway. This would be in scope of Go 1 compatibility promise since the mobile support is considered experimental and the ios tag was not documented. I don't know yet if we'll want or need to do that, just pointing out that issue is relevant here.
I understand the current (Go 1.14) modes of operation and the tags that are satisfied are:

Mode
Satisfied Build Tags

macOS, x86 64-bit
darwin, amd64, !ios

iOS mobile, ARM 64-bit
darwin, arm64, !ios

Issue #38710 mode
darwin, amd64, ios

We would execute the plan in https://github.com/golang/go/issues/38485#issuecomment-622504803 only if we know that darwin/arm64 needs to be reclaimed for macOS, which would be if there is an announcement of a future ARM-based macOS configuration (we may learn more from WWDC 2020 keynote in 4~ days). So the forward-looking table may look like this:

Mode
Satisfied Build Tags

macOS, x86 64-bit
darwin, amd64, !ios

macOS, ARM 64-bit
darwin, arm64, !ios

iOS mobile, ARM 64-bit
darwin, arm64, ios

Issue #38710 mode
darwin, amd64, ios

At that point, it would only be possible to preserve "Issue #38710 mode" behavior of Go 1.14 on an incomplete subset of macOS configurations. If it's important for this mode to be available to all macOS users, some change will be needed.
@bradfitz From the commit message of https://github.com/tailscale/go/commit/1f0b2d1000d3b2998054cf69c6ccded48601a8a0, I understand "Issue #38710 mode" is related to the iOS simulator. Do you know if it needs to be different from normal iOS mobile (other than the architecture difference)?

I've looked into the history and understand this better now.

The ios build tag was only used in crypto/x509, and not documented anywhere I could find. Is "like iOS but on amd64" a supported configuration?

I found some documentation for it in Go 1.5 release notes:

On Darwin, the use of the system X.509 certificate interface can be disabled
with the ios build tag.

There's more background in CL 12301 that added it and issue #11736 that it fixed (/cc @crawshaw).
So as I understand it now, "Issue #38710 mode" (in the tables of my previous comment) is indistinguishable from "iOS mobile" mode: it just means to disable the code to fetch system rooms via API calls and use the embedded ones instead.
So, for 1.15 specifically, this shouldn't intersect with #38485, because the described behavior can be applied for both amd64 and arm64 architectures.
I see there has been a new x509omitbundledroots build tag added in CL 229762 (/cc @bradfitz) for 1.15, but it should work the same for darwin,amd64,ios and darwin,arm64,ios, so I'm not seeing a conflict.

Is crypto/x509 really the only thing that needs to support it?

@hyangah's comment in https://github.com/golang/go/issues/38485#issuecomment-622526879 also suggested that the ios tag is only used in crypto/x509 package.

The build failure can't be reproduced on tip anymore.
It turns out it was fixed during Cthulhu's great awakening in CL 227037 (/cc @FiloSottile), sadly missing out on a chance to add another "Fixes" line:
x509 $ git co 6f52790a20a2432ae61e0ec9852a3df823a16d40^
HEAD is now at 6ea19bb668 crypto/tls: rotate session keys in older TLS versions
x509 $ go build -tags=ios
# crypto/x509
./cert_pool.go:65:9: undefined: loadSystemRoots
./root.go:21:32: undefined: loadSystemRoots
x509 $ git co 6f52790a20a2432ae61e0ec9852a3df823a16d40
Previous HEAD position was 6ea19bb668 crypto/tls: rotate session keys in older TLS versions
HEAD is now at 6f52790a20 crypto/x509: use Security.framework without cgo for roots on macOS
x509 $ go build -tags=ios
x509 $ echo $?
0

I'll look into if the right files are selected, and maybe send a test that checks that for all relevant build configurations, so we have more confidence and this doesn't regress.
Filippo has offered to look at this issue this week, so I'm counting on him to look things over afterwards.

I'll look into if the right files are selected, and maybe send a test that checks that for all relevant build configurations, so we have more confidence and this doesn't regress.

I'm not spotting an issue with the selected files from a first look.
@FiloSottile, does the aforementioned test that checks various build configurations and the .go files that are selected sound worth including? A draft looks like this:

// Test supported build modes. Primarily to provide coverage for
// mobile platforms (Android, iOS) and special build tags.
func TestBuildModes(t *testing.T) {
	if testing.Short() {
		t.Skip("skipping in -short mode")
	}

	tests := []struct {
		name         string
		goos, goarch string
		cgo          bool
		tags         []string
		wantGoFiles  string // Space-separated list of all .go files selected in the build.
	}{
		{
			"Linux (linux/arm64), Cgo on",
			"linux", "ppc64", true, nil,
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_linux.go root_unix.go sec1.go verify.go x509.go",
		},
		{
			"Android (android/arm64), Cgo on",
			"android", "amd64", true, nil,
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_linux.go root_unix.go sec1.go verify.go x509.go",
		},

		{
			"macOS Intel-based 64-bit (darwin/amd64), Cgo off",
			"darwin", "amd64", false, nil,
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_amd64.go sec1.go verify.go x509.go",
		},
		{
			"macOS Intel-based 64-bit (darwin/amd64), Cgo on",
			"darwin", "amd64", true, nil,
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_amd64.go sec1.go verify.go x509.go root_cgo_darwin_amd64.go",
		},
		{
			"iOS 64-bit (darwin/arm64, ios build tag), Cgo on",
			"darwin", "arm64", true, []string{"ios"},
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_arm64.go sec1.go verify.go x509.go",
		},
		{
			"iOS 64-bit (darwin/arm64, ios build tag), Cgo on, x509omitbundledroots build tag",
			"darwin", "arm64", true, []string{"ios", "x509omitbundledroots"},
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_omit.go sec1.go verify.go x509.go",
		},
	}

	for _, test := range tests {
		bctx := build.Default
		bctx.GOOS, bctx.GOARCH = test.goos, test.goarch
		bctx.CgoEnabled = test.cgo
		bctx.BuildTags = test.tags

		p, err := bctx.Import("crypto/x509", "", 0)
		if err != nil {
			t.Fatal(err)
		}
		if len(p.InvalidGoFiles) != 0 {
			t.Errorf("%s: InvalidGoFiles = %q, want none", test.name, p.InvalidGoFiles)
		}
		if got, want := strings.Join(append(p.GoFiles, p.CgoFiles...), " "), test.wantGoFiles; got != want {
			t.Errorf("%s: source files = %q, want %q", test.name, got, want)
		}
	}
}

(It doesn't include all build configurations, only the ones that seem at risk because they're in x/mobile and might not get easily caught.)

I am not sure this is fixed, partially because I am still not sure what this is supposed to do, besides "compile".
In Go 1.14 the ios tag makes the package use the bundled roots even if on amd64. CL 227197 broke the build because it dropped the ios tag without dropping the !ios tag. CL 227037 fixed the build because it also dropped the !ios tag.
The behavior though is now not the same as Go 1.14: with the (now meaningless) ios tag, the package will now still use Security.framework rather than the embedded roots. The problem is that I don't know if Security.framework is acceptable in the iOS simulator (while maybe neither cgo nor shelling out were), or if we need to reintroduce the tag. There's also a chance we'll have to patch the runtime because I think it now unconditionally links Security.framework.
A test that just hardcodes the selected files seems brittle. At least, we should check that the package builds, like for x509omitbundledroots, but even better would be to figure out if we can run the iOS simulator, if that's something we want to support.
@bradfitz is there a way to run the simulator from the command line?

I'm not spotting an issue with the selected files from a first look.

I am spotting an issue now. As you mentioned, it's not using embedded roots in iOS mode. Sorry for a misleading comment.

The problem is that I don't know if Security.framework is acceptable in the iOS simulator (while maybe neither cgo nor shelling out were), or if we need to reintroduce the tag.

I think keeping the same behavior for iOS as it was in 1.14 would be the safest path forward, which means it should continue to use embedded roots (optionally compressed/lazy loaded if x509omitbundledroots build tag is also specified).
Upgrading iOS from using embedded roots to fetching them from via APIs, if it happens to be possible now via Security.framework, can be a feature request to be investigated for 1.16 or later.

I am still not sure what this is supposed to do, besides "compile".

Is the fourth paragraph in https://github.com/golang/go/issues/38710#issuecomment-647483240 helpful? In iOS mode (regardless if the GOARCH used is arm64 or amd64), embedded roots should be used.

is there a way to run the simulator from the command line?

As far as I know, the only officially supported way will need to use x/mobile's gomobile tool, and it will need an Apple developer identity to be available in the environment. I don't think that a test that can be added to crypto/x509. Adding a new builder is a reasonable request, but seems out of scope for this issue.

I am spotting an issue now. As you mentioned, it's not using embedded roots in iOS mode (only when GOARCH is amd64; not when it's arm64).

The following test cases, which I forgot to include earlier, catch this:
{
	"iOS 64-bit in iOS simulator on Intel-based macOS (darwin/amd64, ios build tag), Cgo on",
	"darwin", "amd64", true, []string{"ios"},
	"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_arm64.go sec1.go verify.go x509.go",
},
{
	"iOS 64-bit in iOS simulator on Intel-based macOS (darwin/amd64, ios build tag), Cgo on, x509omitbundledroots build tag",
	"darwin", "amd64", true, []string{"ios", "x509omitbundledroots"},
	"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_omit.go sec1.go verify.go x509.go",
},

$ go test -v -run=TestBuildModes crypto/x509
=== RUN   TestBuildModes
    x509_test.go:1464: iOS 64-bit in iOS simulator on Intel-based macOS (darwin/amd64, ios build tag), Cgo on: source files = "cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_amd64.go sec1.go verify.go x509.go root_cgo_darwin_amd64.go", want "cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_arm64.go sec1.go verify.go x509.go"
    x509_test.go:1464: iOS 64-bit in iOS simulator on Intel-based macOS (darwin/amd64, ios build tag), Cgo on, x509omitbundledroots build tag: source files = "cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_amd64.go sec1.go verify.go x509.go root_cgo_darwin_amd64.go", want "cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_omit.go sec1.go verify.go x509.go"
--- FAIL: TestBuildModes (0.01s)
FAIL
FAIL	crypto/x509	0.394s
FAIL

authors: FiloSottile aclements bradfitz dmitshur toothrot


text:
x/build/cmd/releasebot: improve -dry-run -mode=prepare to support advance testing for more release types

Right now, it's possible to use releasebot -dry-run -mode=prepare (notably, with -dry-run flag) to run a release locally that doesn't publish any public artifacts, but only for the beta release type (e.g., go1.15beta2).
For most other release types, it fails "too early" to produce useful information. For example, it doesn't take into account that the Gerrit CL updating the VERSION file isn't actually mailed, so trying to fetch its commit hash from  Gerrit will give 404, and release won't be able to proceed.
This is the tracking issue about improving that, so fewer ad-hoc changes need to be made to releasebot each time this ability is needed. It's broken out of https://github.com/golang/go/issues/29252#issuecomment-632352576.
/cc @toothrot @cagedmantis @andybons

releasebot has been superseded by relui and this issue doesn't apply directly. relui has a "dry-run (test and build only): ..." workflows for testing, and #47407 is for more. Closing.


text:
cmd/compile: R12 can be clobbered for write barrier call on PPC64

Most Go function calls potentially clobber all registers, except a small set of special ones, including the write barrier call. Currently, on PPC64, the write barrier call is marked as preserving R0, R3-R15 (except R13), and R20, R11. In external linking mode, for large binaries, the external linker may insert a trampoline for the write barrier call, which looks like
0000000005a98cc8 <__long_branch_runtime.gcWriteBarrier>:
 5a98cc8:       86 01 82 3d     addis   r12,r2,390
 5a98ccc:       d8 bd 8c e9     ld      r12,-16936(r12)
 5a98cd0:       a6 03 89 7d     mtctr   r12
 5a98cd4:       20 04 80 4e     bctr

It clobbers R12 (and CTR, which is never live across a call).
As at compile time we don't know whether the binary is big and what link mode will be used, I think we need to mark R12 as clobbered for write barrier call. I'll send a CL.
(Internal linking mode is okay, as the trampoline emitted by Go linker uses R31, the dedicated temp register, instead of R12.)
It has been like that several releases ago. I think we may want to back port, as there could cause runtime crash or data corruption without workaround.

@ianlancetaylor pointed out that for future-proof, the external linker could clobber any caller-saved register that cannot be used for function arguments, that also includes R11. We could consider marking R11 as clobbered as well.

@gopherbot please consider backporting this to Go 1.15. Thanks.

@cherrymui , does this also need a 1.14 backport? I'm pretty sure it does.

Yeah, I think it also has this problem in Go 1.14.

@gopherbot please consider backporting this to Go 1.14. Thanks.

GopherBot is lazy (#25574), so I'm afraid you'll have to open the 1.14 backport issue by hand.

authors: aclements cherrymui


text:
encoding/xml: encoding a start token with a namespace set in Name and Attr results in invalid XML

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

$ go version
go version devel +6965b01ea248 20201124070706 linux/amd64

Does this issue reproduce with the latest release?
Yes
What did you do?
https://play.golang.org/p/ZK8yhFoVwW9
	e := xml.NewEncoder(os.Stdout)
	e.EncodeToken(xml.StartElement{
		Name: xml.Name{Space: "space", Local: "local"},
		Attr: []xml.Attr{{Name: xml.Name{Local: "xmlns"}, Value: "spaceAttr"}},
	})
	e.Flush()
	// <local xmlns="space" xmlns="spaceAttr">

What did you expect to see?
One "xmlns" attribute.
What did you see instead?
Two "xmlns" attributes.
While the provided example is obviously very contrived this can happen in the real world (without a user doing something silly like manually adding a second xmlns attribute) in the situation where the user decodes some XML, makes modifications to the tokens, then encodes those tokens somewhere else (probably a fairly common use case). The decoder will add the "xmlns" attribute and fill out Name, then the encoder will duplicate it. The user would have to manually remove xmlns before encoding, or use RawToken (but this is not an option if the decoder is an arbitrary xml.TokenReader so this isn't always possible). Skipping xmlns attributes on marshal if start.Name is set is a simple patch that would fix this issue.

/cc @rsc

authors: SamWhited cagedmantis


text:
cmd/cgo: exporting functions breaks if CGO_FLAGS enables LTO

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

$ go version
go version devel +23ffb5b9ae Sun Mar 28 03:27:04 2021 +0000 linux/amd64

(but also everything else I've tried)
Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (go env)?
Ubuntu 20.04 amd64 -- but fine details are not important.
go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/mwhudson/.cache/go-build"
GOENV="/home/mwhudson/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/mwhudson/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/mwhudson/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/snap/go/7221"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/snap/go/7221/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.2"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/mwhudson/src/go/src/go.mod"
CGO_CFLAGS="-flto"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2567708190=/tmp/go-build -gno-record-gcc-switches"

What did you do?
(master)mwhudson@anduril:~/src/go/src$ cat ~/src/go-test-cases/cgocallback.go
package main

//
import "C"

//export MyFunction
func MyFunction(arg1, arg2 int) int {
	return arg1 + arg2
}

func main() {

}
(master)mwhudson@anduril:~/src/go/src$ CGO_CFLAGS=-flto ~/src/go/bin/go build ~/src/go-test-cases/cgocallback.go

What did you expect to see?
No output and a binary built.
What did you see instead?
# command-line-arguments
_cgo_export.c:21:13: error: variable ‘_cgoexp_604c78453144_MyFunction’ redeclared as function
cgo-generated-wrappers:1:5: note: previously declared here
lto1: fatal error: errors during merging of translation units
compilation terminated.
lto-wrapper: fatal error: /usr/bin/gcc returned 1 exit status
compilation terminated.
/usr/bin/ld: error: lto-wrapper failed
collect2: error: ld returned 1 exit status

Dup of #43830?  Does https://golang.org/cl/293290 fix the problem?

Yes, and that fixes my problem and one I'd missed and has tests so I'd say definitely prefer that one over mine :)

authors: ianlancetaylor mwhudson


text:
internal/poll: TestSplicePipePool failures with "at least one pipe is still open" on linux-arm

$ greplogs --dashboard -md -l -e (?m)FAIL: TestSplicePipePool.*\n.*at least one pipe is still open --since=2021-04-27

2021-08-26T22:37:54-967a801/android-arm-corellium
2021-08-26T20:39:56-af80af2/android-arm-corellium
2021-08-24T22:23:12-54cdef1/android-arm-corellium
2021-08-22T13:11:50-96d816c/android-arm-corellium
2021-08-17T16:22:15-cf12b0d/android-arm-corellium
2021-08-11T22:07:50-dea23e9/android-arm-corellium
2021-05-26T22:41:55-0fbecec/linux-arm-scaleway
2021-04-29T13:24:00-eb3fe28/linux-arm-scaleway
See previously #45782, #45059 (@panjf2000 @ianlancetaylor @bradfitz).

This test was added for Go 1.17 (March 9, in CL 271537 for #42740), so these failures represent at least a regression in test flakiness in the current release (CC @golang/release). A rollback of the flaky test in CL 308089 was abandoned in favor of an attempt to fix the test in CL 308329.
The failure rate seems to have been partially obscured by the low rate of changes during the Go 1.17 code freeze.
It is not clear to me whether the test failures represent a defect in the test itself or an incomplete fix for #42740. Since this flakiness is new as of Go 1.17, I think a fix for it should at least block the next release.

Given that at least one pipe was not closed in the test, it could only be either that the finalizers for each pipe didn't work properly on some platforms, or that the CloseFunc in destroyPipe() failed when finalizers were executed.
If it is the latter mentioned above, then this issue has been there since the day splice was implemented, otherwise, the problematic issue will goes to the former: runtime.SetFinalizer() and we have to figure out why it was not working as expected.

Per https://pkg.go.dev/runtime#SetFinalizer (emphasis mine):

The finalizer is scheduled to run at some arbitrary time after the program can no longer reach the object to which obj points. There is no guarantee that finalizers will run before a program exits, so typically they are useful only for releasing non-memory resources associated with an object during a long-running program.

If the test depends on a finalizer actually executing, it is depending on a property of the runtime that is not guaranteed to hold — it should probably have extra scrutiny from the runtime team.

It seems to have been working well on https://github.com/golang/go/blob/go1.17/src/sync/pool_test.go#L100-L135 with runtime.SetFinalizer. Is there any chance that some poll.splicePipe's were allocated on stack instead of heap, so the garbage collector did not take effect in this case.

Is it possible that F_GETPIPE_SZ somehow succeeds on a file descriptor for a pipe that was actually destroyed? (Perhaps the FD is somehow reused for another, unrelated file for which F_GETPIPE_SZ succeeds, or some bug in the kernel affecting ARM processors causes a spurious 0-return?)

Is it possible that the destroyPipe call in the finalizer is actually failing to destroy the pipe? I notice that it makes two calls to CloseFunc, which returns an error, but does not check whether those errors are non-nil.

Is it possible that the finalizer is failing to run entirely? (Would it make sense to modify the test to count or examine the finalizer calls independently from the status of the FDs, perhaps by allowing the test to inject an atomic-counter-update into the finalizer?)

Just a note on the original issue: the scaleway builders no longer exist, as Scaleway dropped ARM support.
@panjf2000 We aren't granting new access to our builders at this time.
It looks like it also failed today on linux-386-jessie: https://build.golang.org/log/e4fcae576822945ee58df39050d0db1b2a8a7b8d, for which we have a public image: gcr.io/symbolic-datum-552/linux-x86-jessie:latest
The only other special thing about that builder is the following environment variables, which can also be seen in the linked logs:
			"GOARCH=386",
			"GOHOSTARCH=386",
			"GO_DISABLE_OUTBOUND_NETWORK=1",

It appears that all finalizers occasionally failed to execute on the android platform for some unknown reasons, take a look at this test log, where none of the finalizers executed.

I think you are misreading the log.  Of the various descriptors to check, only descriptor 8 is still open.
This makes me wonder whether descriptor 8 has been opened by some other thread, as in #25628.  This could in fact be the same issue as that one.  According to the internal/testenv package, android-arm always uses external linking.  That doesn't explain the linux-arm failure, though.

I think you are misreading the log. Of the various descriptors to check, only descriptor 8 is still open.
This makes me wonder whether descriptor 8 has been opened by some other thread, as in #25628. This could in fact be the same issue as that one. According to the internal/testenv package, android-arm always uses external linking. That doesn't explain the linux-arm failure, though.

I did misread the log and sorry about that, it was the minimum fd that was still alive, which makes it stands a good chance of being reused inside the process, I will submit a CL to improve the splice_linux_test.go, it would be rational for this test to ignore those fds reopened by other threads.

authors: bcmills ianlancetaylor panjf2000 toothrot


text:
all: test failures on netbsd blocked on waiting for subprocesses

#!watchflakes
post <- builder ~ `netbsd-.*` && (log ~ `^\s*os\.wait6` || log ~ `^\s*syscall.wait4` || `DETECTED A HANGING GO COMMAND` || `test timed out while running command` || `script_test\.go:\d+: .*: signal: killed`)

(attn @golang/netbsd)
See previously:

#56180
#50138
#56215
#48789

@golang/netbsd, is anyone looking into this? These failures are quite frequent.

@golang/netbsd, this failure rate is still quite high compared to the other ports. Is anyone looking into it?

@golang/netbsd, the Go porting policy requires that each port must have at least two developers “making required updates in a timely manner”.
Since no one has even responded to this issue since it was reported in January, and given the frequency of these failures, I believe that this port is no longer meeting that requirement. (If I am mistaken and someone is actually looking into this problem, please comment here with some details about when we can expect progress on a fix.)

I didn't check every log but each log that I checked is consistent with a hang in the wait6 system call.  I would vote for rolling back CL 431855 (again).  CC @tklauser

Would it help if we updated all the NetBSD 9.x test machines to the latest NetBSD 9_STABLE? That should have the rtld fix.
(Relatedly, I have a bug assigned to me to provide NetBSD 10 build VMs.)

The Go bug behind #50138 was fixed in https://go.dev/cl/442478. It is possible that related bugs remain, but the cause of the hands must be different from that cause.
The libc bug behind #56810 was supposed to have been fixed by upgrading the builders in #54773.

I don't think that history is quite right.  We don't have to implement blockUntilWaitable at all.  For NetBSD our first attempt at implementing blockUntilWaitable led to #48789.  We fixed that issue and tried again.  I am suggesting that that exposed this issue.  I think we should now try not implementing blockUntilWaitable at all on NetBSD.

To be clear, I am suggesting that this is a NetBSD kernel bug in wait6.

Just to be sure, can you share the output of ident /usr/lib/libpthread.so (or whatever is the pthread library that golang links against) on the affected builders?

Note that Go by default does not link against libpthread.so at all.  It calls the system call lwp_create directly.  That said, there are cases where it calls pthread_create, and I haven't checked which case applies for these failures.  The output of the ident is:

/usr/lib/libpthread.so:
     $NetBSD: crti.S,v 1.1 2010/08/07 18:01:35 joerg Exp $
     $NetBSD: crtbegin.S,v 1.2 2010/11/30 18:37:59 joerg Exp $
     $NetBSD: tss.c,v 1.1 2019/04/24 11:43:19 kamil Exp $
     $NetBSD: thrd.c,v 1.3.4.1 2019/09/13 06:54:09 martin Exp $
     $NetBSD: mtx.c,v 1.1 2019/04/24 11:43:19 kamil Exp $
     $NetBSD: cnd.c,v 1.1 2019/04/24 11:43:19 kamil Exp $
     $NetBSD: call_once.c,v 1.2 2019/04/24 21:41:15 kamil Exp $
     $NetBSD: sem.c,v 1.9 2019/02/21 21:33:34 christos Exp $
     $NetBSD: pthread_spin.c,v 1.6 2012/08/16 04:49:47 matt Exp $
     $NetBSD: pthread_once.c,v 1.3 2013/03/21 16:49:12 christos Exp $
     $NetBSD: pthread_cancelstub.c,v 1.38.32.1 2021/10/05 11:02:18 martin Exp $
     $NetBSD: pthread_tsd.c,v 1.17 2019/03/05 01:35:52 christos Exp $
     $NetBSD: pthread_specific.c,v 1.26 2013/03/21 16:49:12 christos Exp $
     $NetBSD: pthread_rwlock.c,v 1.34.18.1 2020/01/26 10:55:16 martin Exp $
     $NetBSD: pthread_mutex.c,v 1.65.2.1 2020/01/26 10:55:16 martin Exp $
     $NetBSD: pthread_misc.c,v 1.15.32.1 2020/01/26 10:55:16 martin Exp $
     $NetBSD: pthread_lock.c,v 1.34 2008/04/28 20:23:01 martin Exp $
     $NetBSD: pthread_cond.c,v 1.65.6.1 2020/01/26 10:55:16 martin Exp $
     $NetBSD: pthread_barrier.c,v 1.20 2016/07/03 14:24:58 christos Exp $
     $NetBSD: pthread_attr.c,v 1.18 2017/08/01 12:31:45 martin Exp $
     $NetBSD: pthread.c,v 1.153.2.2 2022/08/08 17:16:53 martin Exp $
     $NetBSD: pthread_makelwp_netbsd.c,v 1.2 2014/12/17 01:49:08 pooka Exp $
     $NetBSD: crtend.S,v 1.1 2010/08/07 18:01:34 joerg Exp $
     $NetBSD: crtn.S,v 1.1 2010/08/07 18:01:35 joerg Exp $

Is there a summary of how golang uses the wait family of system calls and handles SIGCHLD?  I see there's a lot of past discussion and changes, so it would be helpful if I had a clear idea of the current picture.

Go will normally install a signal handler for SIGCHLD, but for most programs, including the ones that are failing, that signal handler won't do anything.
When Go is waiting for a process, it first calls wait6(P_PID, pidOfProcess, WEXITED|WNOWAIT).  When that call returns, Go will call wait4(pidOfProcess, &status, 0, &rusage).  (This two step process is to avoid a race in sending a signal to a process and accidentally sending it to a PID of a process that has been waited for and then recycled.)

golang doesn't by any chance use SA_NOCLDWAIT, or set SIGCHLD to SIG_IGN, does it?

It does not.

Does golang have any mechanism to guarantee that it only calls waitid/wait6 without WNOWAIT at most once -- or, at most once with a successful return if WNOHANG -- for each child?  (I suppose the answer is probably yes but I want to rule out easy problems before going into a wild goose chase of harder ones!)

Go does not have any such mechanism.  If a program calls os.Process.Wait from two different goroutines simultaneously, then Go will call wait4 from two different threads.  That said, that is not happening in these test cases.

Is there any ptrace involved?

No.

Is there any SIGTSTP/SIGSTOP/SIGCONT/WSTOPPED/WUNTRACED involved?

No.

Can you get any kind of core dump or stack trace of the child process when it hangs like this?

As far as I can tell, the child process has exited.

Are there any reports that consistently happen running the same test (after the rtld/pthread fix and the idtype numbering fix), so I could try running that one in a loop to more reliably reproduce it?

I haven't been able to recreate the problem.  It's fairly rare.  But I also haven't tried very hard.

Is there any chance you could start ktrace in the test harness, and save the ktrace file when it fails in this manner?  Something like ktrace("/tmp/ktrace.out", KTROP_SET|KTRFLAG_DESCEND|KTRFACv2, KTRFAC_SYSCALL|KTRFAC_SYSRET|KTRFAC_NAMEI|KTRFAC_PSIG|KTRFAC_EMUL|KTRFAC_MIB, getpid()) when each test starts, and ktrace(NULL, KTROP_CLEAR|KTRFLAG_DESCEND|KTRFACv2, KTRFAC_MASK, 1) when it ends, and, if the test fails, either save /tmp/ktrace.out or include the output of kdump -T -f /tmp/ktrace.out in the report?  (@bsiegert, maybe you could rig this up?)

I'll leave that for @bsiegert.

I took a look at a few other logs and they all appear to be blocking in wait6 under blockUntilWaitable, which is consistent with the hypothesis that the subprocess just hasn't exited yet. If that is the case, we need to know why the subprocess hasn't exited, not why wait6 isn't returning.

That is a fair point.  My only real reason for thinking that the problem is with wait6 is that we aren't seeing the builder hang.  We're only seeing the builder time out while waiting for child processes.

I took a look at a few other logs and they all appear to be blocking in wait6 under blockUntilWaitable, which is consistent with the hypothesis that the subprocess just hasn't exited yet. If that is the case, we need to know why the subprocess hasn't exited, not why wait6 isn't returning.

Several of the failures in this log are for subtests of os/exec.TestCancelErrors — namely, subtests beginning with killed_.
In normal operation, those subtests terminate the child process by sending it SIGKILL.
For example, in https://build.golang.org/log/0889975bf55f90c65f04d4535e58591d7c30fa4f we see:
goroutine 172 [chan send, 29 minutes]:
runtime.gopark(0x40001d9df8?, 0xaf9f8?, 0x0?, 0x0?, 0x4000024000?)
	/var/gobuilder/buildlet/go/src/runtime/proc.go:398 +0xc8 fp=0x40001d9dd0 sp=0x40001d9db0 pc=0x4adf8
runtime.chansend(0x4000185560, 0x40001d9f90, 0x1, 0x40001d9e50?)
	/var/gobuilder/buildlet/go/src/runtime/chan.go:259 +0x3c4 fp=0x40001d9e40 sp=0x40001d9dd0 pc=0x166e4
runtime.chansend1(0x40001d9f10?, 0x3d4608?)
	/var/gobuilder/buildlet/go/src/runtime/chan.go:145 +0x18 fp=0x40001d9e70 sp=0x40001d9e40 pc=0x16308
os/exec.(*Cmd).watchCtx(0x4000235080, 0x4000185560)
	/var/gobuilder/buildlet/go/src/os/exec/exec.go:813 +0x328 fp=0x40001d9fb0 sp=0x40001d9e70 pc=0x10c368
os/exec.(*Cmd).Start.func4()
	/var/gobuilder/buildlet/go/src/os/exec/exec.go:715 +0x2c fp=0x40001d9fd0 sp=0x40001d9fb0 pc=0x10bdbc
runtime.goexit()
	/var/gobuilder/buildlet/go/src/runtime/asm_arm64.s:1197 +0x4 fp=0x40001d9fd0 sp=0x40001d9fd0 pc=0x7da24
created by os/exec.(*Cmd).Start in goroutine 120
	/var/gobuilder/buildlet/go/src/os/exec/exec.go:715 +0x788

That goroutine is blocked on this line:
https://cs.opensource.google/go/go/+/master:src/os/exec/exec.go;l=813;drc=29253f4d513f1d3fe549db39a8daa311492138b3
which is dominated by an unconditional call to Kill on the command's *os.Process:
https://cs.opensource.google/go/go/+/master:src/os/exec/exec.go;l=768;drc=29253f4d513f1d3fe549db39a8daa311492138b3
The implementation of *os.Process.Kill is pretty straightforward, so I would be pretty shocked if that doesn't end up making the correct syscalls on NetBSD. 😅
os/exec.TestCancelErrors and os.TestStdPipe might be good cases to focus on, because they involve only single subprocesses. (Many of the other failures here end up stuck on go build, but in those cases more often than not go build is itself blocked on a stuck invocation of cmd/compile, which makes the goroutine dump for the test itself look — correctly! — like it is blocked on a go subprocess that is still running.)

Is there a document anywhere that lays out how this works on each OS, or roughly how things like the syscalls and cgo dynamic symbol calls are put together?

I'm not aware of any particular document. (I would expect to find it in https://cs.opensource.google/go/go/+/master:src/runtime/HACKING.md, but it's not there.)
A short summary of my understanding:

On darwin (including ios), aix, solaris (including illumos), openbsd (#61546 notwithstanding), and windows, the runtime always makes its system calls through a dynamically-linked libc or kernel32.dll.

On linux (including android), freebsd, netbsd, dragonfly, plan9, and wasm, the runtime makes direct system calls using assembly routines.

However, if cgo is enabled and the program ends up linking against a package that uses it (often the net package), the runtime calls through libc for functions specific to thread management (declared in src/runtime/cgo.go).
That behavior is toggled at link time using the runtime.iscgo variable.

It looks like the os test doesn't have any dependencies that would involve dynamic linking. (Notably, it doesn't end up importing net.) I'm not entirely sure whether it is purely statically linked in practice, but you could check by building the test binary with go test -c os.
One of these hangs was reported in os.TestStdPipe in https://github.com/golang/go/issues/57999#issuecomment-1684522180.

I examined a go1.20 executable on netbsd-9 with readelf --dynamic and it certainly looks like it's linked against libpthread:

As @bcmills says, this is going to vary depending on whether the Go program imports any packages that call into C code.  I've verified that on NetBSD the os package tests are running a program that is not dynamically linked.  This is the test used for TestStdPipe that @bcmills mentioned.

If you're calling the _lwp_create syscall directly instead of using the pthread_create symbol, but you're calling any other functions via dynamic symbols, that will get you in trouble and having a newer libpthread.so with the rtld/pthread fix won't help. There is probably lots of other trouble you can get into if you bypass the pthread symbols -- I would have to think more about it.

When dynamically linked, Go programs will call pthread_create to create new threads.
That said, there are cases where dynamically linked programs will call the LWP functions.  The functions I see that may be called are lwp_park, lwp_unpark, lwp_self, and lwp_kill.

Can you be more specific about what the difference is?

What we are seeing is that a Go program starts a subprocess, waits for it to complete, and the wait6(P_PID, pid, WEXITED|WNOWAIT) call never returns.  If there were a problem such that programs fail to exit, I would expect to also see cases where the build and test run does not complete without reporting any error about waiting for a subprocess.  As far as I know we aren't seeing such a problem.

Hmm. I guess it would be possible to use runtime.Stack to detect when the test has been stuck in wait6 for too long — but then wouldn't we have to start some other new process to prevent the test from deadlocking when it sends itself SIGSTOP?
(You're welcome to send a CL to implement that, and I'll review it, but it seems like it would add a lot of extra complexity to an already-complex situation. 😅)

The flake above is on 10_BETA. So much for my hope that this was fixed in a newer NetBSD version :(

@golang/netbsd, is it possible that this is (somehow) a kqueue-related race condition?
#63937 on darwin is plausibly kqueue-related, and I wonder if they might share a similar root cause.

@golang/netbsd, is it possible that this is (somehow) a kqueue-related race condition?
#63937 on darwin is plausibly kqueue-related, and I wonder if they might share a similar root cause.

Are you thinking of a race in the way Go uses kqueue, or a race in kqueue itself?

I suspect a race in the way Go uses kqueue.

authors: bcmills bsiegert ianlancetaylor


text:
PureGo Plugins?

I was playing around with the plugins package of the standard library.
The idea of plugins itself is great.
However, I did not appreciate the fact that I was forced to switch compiler. (plugins are enabled by C language & toolchain)
Searching online I have failed to find a mention of why was it implemented in such a way.
Also, it is my desire to contribute code to the plugins package.
Please refer me to any resources of programming Go.
(apart from the Go source-code, that I'm already reading)

Unlike many projects, the Go project does not use GitHub Issues for general discussion or asking questions. GitHub Issues are used for tracking bugs and proposals only.
For questions please refer to https://github.com/golang/go/wiki/Questions

authors: seankhliao zarkones