A missing operand to mergePoint caused lower to place values
in the wrong blocks.
Includes test, belt+suspenders to do both ssa check and verify
the output (was is how the bug was originally observed).
The fixed bug here is very likely present in Go versions
1.9-1.12 on amd64 and s390x
Updates #32680.
Fixes#32712.
Change-Id: I63e702c4c40602cb795ef71b1691eb704d38ccc7
Reviewed-on: https://go-review.googlesource.com/c/go/+/183059
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
(cherry picked from commit 769fda2d51)
Reviewed-on: https://go-review.googlesource.com/c/go/+/183241
For int8, int16, and int32, comparing their unsigned value to MaxInt64
to determine non-negativity doesn't make sense, because they have
negative values whose unsigned representation is smaller than that.
Fix is simply to compare with the appropriate upper bound based on the
value type's size.
Fixes#32583.
Change-Id: Ie7afad7a56af92bd890ba5ff33c86d1df06cfd9a
Reviewed-on: https://go-review.googlesource.com/c/go/+/181797
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit f44404ebbf)
Reviewed-on: https://go-review.googlesource.com/c/go/+/181978
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
The logic for detecting deferreturn calls is wrong.
We used to look for a relocation whose symbol is runtime.deferreturn
and has an offset of 0. But on some architectures, the relocation
offset is not zero. These include arm (the offset is 0xebfffffe) and
s390x (the offset is 6).
This ends up setting the deferreturn offset at 0, so we end up using
the entry point live map instead of the deferreturn live map in a
frame which defers and then segfaults.
Instead, use the IsDirectJump helper to find calls.
Fixes#32484
Change-Id: Iecb530a7cf6eabd7233be7d0731ffa78873f3a54
Reviewed-on: https://go-review.googlesource.com/c/go/+/181258
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
(cherry picked from commit 9eb403159d)
Reviewed-on: https://go-review.googlesource.com/c/go/+/181262
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
If a slice's entries are sparse, we decide to initialize it dynamically
instead of statically. That's CL 151319.
But if we do initialize it dynamically, we still need to initialize
the static entries. Typically we do that, but the bug fixed here is
that we don't if the entry's value is itself an array or struct.
To fix, use initKindLocalCode to ensure that both static and
dynamic entries are initialized via code.
Fixes#32013
Change-Id: I1192ffdbfb5cd50445c1206c4a3d8253295201dd
Reviewed-on: https://go-review.googlesource.com/c/go/+/176904
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
(cherry picked from commit a9e107c85c)
Reviewed-on: https://go-review.googlesource.com/c/go/+/177040
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Stack object generation code was always using the local package name
for its symbol. Normally that doesn't matter, as we usually only
compile functions in the local package. But for wrappers, the compiler
generates functions which live in other packages. When there are two
other packages with identical functions to wrap, the same name appears
twice, and the compiler goes boom.
Fixes#31396
Change-Id: I7026eebabe562cb159b8b6046cf656afd336ba25
Reviewed-on: https://go-review.googlesource.com/c/go/+/171464
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
(cherry picked from commit 43001a0dc9)
Reviewed-on: https://go-review.googlesource.com/c/go/+/173317
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
CL 135377 introduces pass strings and slices to convT2{E,I} by value.
Before that CL, all types, except interface will be allocated temporary
address. The CL changes the logic that only constant and type which
needs address (determine by convFuncName) will be allocated.
It fails to cover the case where type is static composite literal.
Adding condition to check that case fixes the issue.
Also, static composite literal node implies constant type, so consttype
checking can be removed.
Fixes#31209
Change-Id: Ifc750a029fb4889c2d06e73e44bf85e6ef4ce881
Reviewed-on: https://go-review.googlesource.com/c/go/+/168858
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
(cherry picked from commit d47db6dc0c)
Reviewed-on: https://go-review.googlesource.com/c/go/+/170437
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
It is possible that a "volatile" value (one that can be clobbered
by preparing args of a call) to be used in multiple write barrier
calls. We used to copy the volatile value right before each call.
But this doesn't work if the value is used the second time, after
the first call where it is already clobbered. Copy it before
emitting any call.
Updates #30977.
Fixes#30996.
Change-Id: Iedcc91ad848d5ded547bf37a8359c125d32e994c
Reviewed-on: https://go-review.googlesource.com/c/go/+/168677
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
(cherry picked from commit f23c601bf9)
Reviewed-on: https://go-review.googlesource.com/c/go/+/168817
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Currently, runtime.KeepAlive applied on a stack object doesn't
actually keeps the stack object alive, and the heap object
referenced from it could be collected. This is because the
address of the stack object is rematerializeable, and we just
ignored KeepAlive on rematerializeable values. This CL fixes it.
Updates #30476.
Fixes#30478.
Change-Id: Ic1f75ee54ed94ea79bd46a8ddcd9e81d01556d1d
Reviewed-on: https://go-review.googlesource.com/c/164537
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
(cherry picked from commit 40df9cc606)
Reviewed-on: https://go-review.googlesource.com/c/go/+/164627
Make sure the side effects inside short-circuited operations (&& and ||)
happen correctly.
Before this CL, we attached the side effects to the node itself using
exprInPlace. That caused other side effects in sibling expressions
to get reordered with respect to the short circuit side effect.
Instead, rewrite a && b like:
r := a
if r {
r = b
}
That code we can keep correctly ordered with respect to other
side-effects extracted from part of a big expression.
exprInPlace seems generally unsafe. But this was the only case where
exprInPlace is called not at the top level of an expression, so I
don't think the other uses can actually trigger an issue (there can't
be a sibling expression). TODO: maybe those cases don't need "in
place", and we can retire that function generally.
This CL needed a small tweak to the SSA generation of OIF so that the
short circuit optimization still triggers. The short circuit optimization
looks for triangle but not diamonds, so don't bother allocating a block
if it will be empty.
Go 1 benchmarks are in the noise.
Fixes#30567
Change-Id: I19c04296bea63cbd6ad05f87a63b005029123610
Reviewed-on: https://go-review.googlesource.com/c/go/+/165617
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit 4a9064ef41)
Reviewed-on: https://go-review.googlesource.com/c/go/+/165858
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
CL 154057 adds guards agaist out-of-bound reads from readonly
constants. It turns out that in dead code, the offset can also
be negative. Guard against negative offset as well.
Fixes#30257.
Change-Id: I47c2a2e434dd466c08ae6f50f213999a358c796e
Reviewed-on: https://go-review.googlesource.com/c/162819
Reviewed-by: Keith Randall <khr@golang.org>
(cherry picked from commit dca707b2a0)
Reviewed-on: https://go-review.googlesource.com/c/162827
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Consider the following code:
func f(x []*T) interface{} {
return x
}
It returns an interface that holds a heap copy of x (by calling
convT2I or friend), therefore x escape to heap. The current
escape analysis only recognizes that x flows to the result. This
is not sufficient, since if the result does not escape, x's
content may be stack allocated and this will result a
heap-to-stack pointer, which is bad.
Fix this by realizing that if a CONVIFACE escapes and we're
converting from a non-direct interface type, the data needs to
escape to heap.
Running "toolstash -cmp" on std & cmd, the generated machine code
are identical for all packages. However, the export data (escape
tags) differ in the following packages. It looks to me that all
are similar to the "f" above, where the parameter should escape
to heap.
io/ioutil/ioutil.go:118
old: leaking param: r to result ~r1 level=0
new: leaking param: r
image/image.go:943
old: leaking param: p to result ~r0 level=1
new: leaking param content: p
net/url/url.go:200
old: leaking param: s to result ~r2 level=0
new: leaking param: s
(as a consequence)
net/url/url.go:183
old: leaking param: s to result ~r1 level=0
new: leaking param: s
net/url/url.go:194
old: leaking param: s to result ~r1 level=0
new: leaking param: s
net/url/url.go:699
old: leaking param: u to result ~r0 level=1
new: leaking param: u
net/url/url.go:775
old: (*URL).String u does not escape
new: leaking param content: u
net/url/url.go:1038
old: leaking param: u to result ~r0 level=1
new: leaking param: u
net/url/url.go:1099
old: (*URL).MarshalBinary u does not escape
new: leaking param content: u
flag/flag.go:235
old: leaking param: s to result ~r0 level=1
new: leaking param content: s
go/scanner/errors.go:105
old: leaking param: p to result ~r0 level=0
new: leaking param: p
database/sql/sql.go:204
old: leaking param: ns to result ~r0 level=0
new: leaking param: ns
go/constant/value.go:303
old: leaking param: re to result ~r2 level=0, leaking param: im to result ~r2 level=0
new: leaking param: re, leaking param: im
go/constant/value.go:846
old: leaking param: x to result ~r1 level=0
new: leaking param: x
encoding/xml/xml.go:518
old: leaking param: d to result ~r1 level=2
new: leaking param content: d
encoding/xml/xml.go:122
old: leaking param: leaking param: t to result ~r1 level=0
new: leaking param: t
crypto/x509/verify.go:506
old: leaking param: c to result ~r8 level=0
new: leaking param: c
crypto/x509/verify.go:563
old: leaking param: c to result ~r3 level=0, leaking param content: c
new: leaking param: c
crypto/x509/verify.go:615
old: (nothing)
new: leaking closure reference c
crypto/x509/verify.go:996
old: leaking param: c to result ~r1 level=0, leaking param content: c
new: leaking param: c
net/http/filetransport.go:30
old: leaking param: fs to result ~r1 level=0
new: leaking param: fs
net/http/h2_bundle.go:2684
old: leaking param: mh to result ~r0 level=2
new: leaking param content: mh
net/http/h2_bundle.go:7352
old: http2checkConnHeaders req does not escape
new: leaking param content: req
net/http/pprof/pprof.go:221
old: leaking param: name to result ~r1 level=0
new: leaking param: name
cmd/internal/bio/must.go:21
old: leaking param: w to result ~r1 level=0
new: leaking param: w
Fixes#29353.
Change-Id: I7e7798ae773728028b0dcae5bccb3ada51189c68
Reviewed-on: https://go-review.googlesource.com/c/162829
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit 0349f29a55)
Reviewed-on: https://go-review.googlesource.com/c/163203
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Change-Id: Idee94a1d93555d53442098dd7479982e3f5afbba
Reviewed-on: https://go-review.googlesource.com/c/161339
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Make sure the argument to memmove is of pointer type before we try to
get the element type.
This has been noticed for code that uses unsafe+linkname so it can
call runtime.memmove. Probably not the best thing to allow, but the
code is out there and we'd rather not break it unnecessarily.
Fixes#30061
Change-Id: I334a8453f2e293959fd742044c43fbe93f0b3d31
Reviewed-on: https://go-review.googlesource.com/c/160826
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
We are copying the results to uninitialized stack space. Write
barrier is not needed.
Fixes#30041.
Change-Id: Ia91d74dbafd96dc2bd92de0cb479808991dda03e
Reviewed-on: https://go-review.googlesource.com/c/160737
Run-TryBot: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Treat compiler-generated init functions as wrappers, so they will not
be shown in tracebacks.
The exception to this rule is that we'd like to show the line number
of initializers for global variables in tracebacks. In order to
preserve line numbers for those cases, separate out the code for those
initializers into a separate function (which is not marked as
autogenerated).
This CL makes the go binary 0.2% bigger.
Fixes#29919
Change-Id: I0f1fbfc03d10d764ce3a8ddb48fb387ca8453386
Reviewed-on: https://go-review.googlesource.com/c/159717
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Whether a truncation should become a MOVWreg or a MOVWZreg doesn't
depend on the type of the operand, it depends on the type of the final
result. If the final result is unsigned, we can use MOVWZreg. If the
final result is signed, we can use MOVWreg. Checking the type of the
operand does the wrong thing if truncating an unsigned value to a
signed value, or vice-versa.
Fixes#29943
Change-Id: Ia6fc7d006486fa02cffd0bec4d910bdd5b6365f8
Reviewed-on: https://go-review.googlesource.com/c/159760
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
They can't be used, so we don't need code generated for them. We just
need to report errors in their bodies.
This is the minimal CL for 1.12. For 1.13, CL 158845 will remove
a bunch of special cases sprinkled about the compiler to handle "_"
functions, which should (after this CL) be unnecessary.
Update #29870
Change-Id: Iaa1c194bd0017dffdce86589fe2d36726ee83c13
Reviewed-on: https://go-review.googlesource.com/c/158820
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reuse the strict mechanism from FileLine for FuncForPC, so we don't
crash when asking the pcln table about bad pcs.
Fixes#29735
Change-Id: Iaffb32498b8586ecf4eae03823e8aecef841aa68
Reviewed-on: https://go-review.googlesource.com/c/157799
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Normally this happens when combining a sign extension and a load. We
want the resulting combo-instruction to get the line number of the
load, not the line number of the sign extension.
For each rule, compute where we should get its line number by finding
a value on the match side that can fault. Use that line number for
all the new values created on the right-hand side.
Fixes#27201
Change-Id: I19b3c6f468fff1a3c0bfbce2d6581828557064a3
Reviewed-on: https://go-review.googlesource.com/c/156937
Reviewed-by: David Chase <drchase@google.com>
Currently, obj.Ctxt's symbol table does not distinguish between ABI0
and ABIInternal symbols. This is *almost* okay, since a given symbol
name in the final object file is only going to belong to one ABI or
the other, but it requires that the compiler mark a Sym as being a
function symbol before it retrieves its LSym. If it retrieves the LSym
first, that LSym will be created as ABI0, and later marking the Sym as
a function symbol won't change the LSym's ABI.
Marking a Sym as a function symbol before looking up its LSym sounds
easy, except Syms have a dual purpose: they are used just as interned
strings (every function, variable, parameter, etc with the same
textual name shares a Sym), and *also* to store state for whatever
package global has that name. As a result, it's easy to slip up and
look up an LSym when a Sym is serving as the name of a local variable,
and then later mark it as a function when it's serving as the global
with the name.
In general, we were careful to avoid this, but #29610 demonstrates one
case where we messed up. Because of on-demand importing from indexed
export data, it's possible to compile a method wrapper for a type
imported from another package before importing an init function from
that package. If the argument of the method is named "init", the
"init" LSym will be created as a data symbol when compiling the
wrapper, before it gets marked as a function symbol.
To fix this, we separate obj.Ctxt's symbol tables for ABI0 and
ABIInternal symbols. This way, the compiler will simply get a
different LSym once the Sym takes on its package-global meaning as a
function.
This fixes the above ordering issue, and means we no longer need to go
out of our way to create the "init" function early and mark it as a
function symbol.
Fixes#29610.
Updates #27539.
Change-Id: Id9458b40017893d46ef9e4a3f9b47fc49e1ce8df
Reviewed-on: https://go-review.googlesource.com/c/157017
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Returning the innermost frame instead of the outermost
makes code that walks the results of runtime.Caller{,s}
still work correctly in the presence of mid-stack inlining.
Fixes#29582
Change-Id: I2392e3dd5636eb8c6f58620a61cef2194fe660a7
Reviewed-on: https://go-review.googlesource.com/c/156364
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
The compiler appears to contain several squirrelly corner
cases where nodes are double walked, some where new nodes
are created from walked parts. Rather than trust that we
had searched hard enough for the last one, change
exprSwitch.walk() to return immediately if it has already
been walked. This appears to be the only case where
double-walking a node is actually harmful.
Fixes#29562.
Change-Id: I0667e8769aba4c3236666cd836a934e256c0bfc5
Reviewed-on: https://go-review.googlesource.com/c/156317
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
As a followon to CL 152537, modify the panic-printing traceback
to also handle mid-stack inlining correctly.
Also declare -fm functions (aka method functions) as wrappers, so that
they get elided during traceback. This fixes part 2 of #26839.
Fixes#28640Fixes#24488
Update #26839
Change-Id: I1c535a9b87a9a1ea699621be1e6526877b696c21
Reviewed-on: https://go-review.googlesource.com/c/153477
Reviewed-by: David Chase <drchase@google.com>
Use the length of the bitmap to decide how much to pass to the
write barrier, not the total length of the arguments.
The test needs enough arguments so that two distinct bitmaps
get interpreted as a single longer bitmap.
Update #29362
Change-Id: I78f3f7f9ec89c2ad4678f0c52d3d3def9cac8e72
Reviewed-on: https://go-review.googlesource.com/c/156123
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
In the case of x+d >= w, where d and w are constants, we are
deriving x is within the bound of min=w-d and max=maxInt-d. When
there is an overflow (min >= max), we know only one of x >= min
or x <= max is true, and we derive this by excluding the other.
When excluding x >= min, we did not consider the equal case, so
we could incorrectly derive x <= max when x == min.
Fixes#29502.
Change-Id: Ia9f7d814264b1a3ddf78f52e2ce23377450e6e8a
Reviewed-on: https://go-review.googlesource.com/c/156019
Reviewed-by: David Chase <drchase@google.com>
CL 155917 added a -race test that shouldn't be run when cgo is not
enabled. Enforce this in the test file, with a buildflag.
Fixes the nocgo builder.
Change-Id: I9fe0d8f21da4d6e2de3f8fe9395e1fa7e9664b02
Reviewed-on: https://go-review.googlesource.com/c/155957
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reorg map flags a bit so we don't need any extra space for the extra flag.
Fixes#23734
Change-Id: I436812156240ae90de53d0943fe1aabf3ea37417
Reviewed-on: https://go-review.googlesource.com/c/155918
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
We can't remove race instrumentation unless there are no calls,
not just no static calls. Closure and interface calls also count.
The problem in issue 29329 is that there was a racefuncenter, an
InterCall, and a racefuncexit. The racefuncenter was removed, then
the InterCall was rewritten to a StaticCall. That prevented the
racefuncexit from being removed. That caused an imbalance in
racefuncenter/racefuncexit calls, which made the race detector barf.
Bug introduced at CL 121235
Fixes#29329
Change-Id: I2c94ac6cf918dd910b74b2a0de5dc2480d236f16
Reviewed-on: https://go-review.googlesource.com/c/155917
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Work involved in getting a stack trace is divided between
runtime.Callers and runtime.CallersFrames.
Before this CL, runtime.Callers returns a pc per runtime frame.
runtime.CallersFrames is responsible for expanding a runtime frame
into potentially multiple user frames.
After this CL, runtime.Callers returns a pc per user frame.
runtime.CallersFrames just maps those to user frame info.
Entries in the result of runtime.Callers are now pcs
of the calls (or of the inline marks), not of the instruction
just after the call.
Fixes#29007Fixes#28640
Update #26320
Change-Id: I1c9567596ff73dc73271311005097a9188c3406f
Reviewed-on: https://go-review.googlesource.com/c/152537
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(SGTconst [c] (SRLconst _ [d])) && 0 <= int32(c) && uint32(d) <= 31 && 1<<(32-uint32(d)) <= int32(c) -> (MOVWconst [1])
This rule is problematic. 1<<(32-uint32(d)) <= int32(c) meant to
say that it is true if c is greater than the largest possible
value of the right shift. But when d==1, 1<<(32-1) is negative
and results in the wrong comparison.
Rewrite the rules in a more direct way.
Fixes#29402.
Change-Id: I5940fc9538d9bc3a4bcae8aa34672867540dc60e
Reviewed-on: https://go-review.googlesource.com/c/155798
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
If someone takes a pointer to a zero-sized stack variable, it can
be incorrectly interpreted as a pointer to the next object in the
stack frame. To avoid this, add some padding after zero-sized variables.
We only need to pad if the next variable in memory (which is the
previous variable in the order in which we allocate variables to the
stack frame) has pointers. If the next variable has no pointers, it
won't hurt to have a pointer to it.
Because we allocate all pointer-containing variables before all
non-pointer-containing variables, we should only have to pad once per
frame.
Fixes#24993
Change-Id: Ife561cdfdf964fdbf69af03ae6ba97d004e6193c
Reviewed-on: https://go-review.googlesource.com/c/155698
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Method expressions where the method is implicitly declared have no
line number. The Error method of the built-in error type is one such
method. We leave the line number at the use of the method expression
in this case.
Fixes#29389
Change-Id: I29c64bb47b1a704576abf086599eb5af7b78df53
Reviewed-on: https://go-review.googlesource.com/c/155639
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
It was possible that
var X interface{} = 'x'
could cause a compilation failure due to having not calculated rune's
width yet. typecheck.go normally calculates the width of things, but
it doesn't for implicit conversions to default type. We already
compute the width of all of the standard numeric types in universe.go,
but we failed to calculate it for the rune alias type. So we could
later crash if the code never otherwise explicitly mentioned 'rune'.
While here, explicitly compute widths for 'byte' and 'error' for
consistency.
Fixes#29350.
Change-Id: Ifedd4899527c983ee5258dcf75aaf635b6f812f8
Reviewed-on: https://go-review.googlesource.com/c/155380
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
Out-of-bounds reads of globals can happen in dead code. For code
like this:
s := "a"
if len(s) == 3 {
load s[0], s[1], and s[2]
}
The out-of-bounds loads are dead code, but aren't removed yet
when lowering. We need to not panic when compile-time evaluating
those loads. This can only happen for dead code, so the result
doesn't matter.
Fixes#29215
Change-Id: I7fb765766328b9524c6f2a1e6ab8d8edd9875097
Reviewed-on: https://go-review.googlesource.com/c/154057
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alberto Donizetti <alb.donizetti@gmail.com>
The formatting routines for types use a depth limit as primitive
mechanism to detect cycles. For now, increase the limit from 100
to 250 and file #29312 so we don't drop this on the floor.
Also, adjust some fatal error messages elsewhere to use
better formatting.
Fixes#29264.
Updates #29312.
Change-Id: Idd529f6682d478e0dcd2d469cb802192190602f6
Reviewed-on: https://go-review.googlesource.com/c/154583
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
When a println arg contains a call to an inlineable function
that itself contains a switch, that switch statement will be
walked twice, once by the walkexprlist formerly in the
OPRINT/OPRINTN case, then by walkexprlistcheap in walkprint.
Remove the first walkexprlist, it is not necessary.
walkexprlist =
s[i] = walkexpr(s[i], init)
walkexprlistcheap = {
s[i] = cheapexpr(n, init)
s[i] = walkexpr(s[i], init)
}
Seems like this might be possible in other places, i.e.,
calls to inlineable switch-containing functions.
See also #25776.
Fixes#29220.
Change-Id: I3781e86aad6688711597b8bee9bc7ebd3af93601
Reviewed-on: https://go-review.googlesource.com/c/154497
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
A prior optimization (https://golang.org/cl/106175) removed the
generation of unnecessary method expression wrappers, but also
eliminated the generation of the wrapper for error.Error which
was still required.
Special-case error type in the optimization.
Fixes#29304.
Change-Id: I54c8afc88a2c6d1906afa2d09c68a0a3f3e2f1e3
Reviewed-on: https://go-review.googlesource.com/c/154578
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Instead of testing len(slice)+numNewElements > cap(slice) use
uint(len(slice)+numNewElements) > uint(cap(slice)) to test
if a slice needs to be grown in an append operation.
This prevents a possible overflow when len(slice) is near the maximum
int value and the addition of a constant number of new elements
makes it overflow and wrap around to a negative number which is
smaller than the capacity of the slice.
Appending a slice to a slice with append(s1, s2...) already used
a uint comparison to test slice capacity and therefore was not
vulnerable to the same overflow issue.
Fixes: #29190
Change-Id: I41733895838b4f80a44f827bf900ce931d8be5ca
Reviewed-on: https://go-review.googlesource.com/c/154037
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
By combining the load+op, we may force the op to happen earlier in
the store chain. That might force the SymAddr operation earlier, and
in particular earlier than its corresponding VarDef. That leads to
an invalid schedule, so avoid that.
This is kind of a hack to work around the issue presented. I think
the underlying problem, that LEAQ is not directly ordered with respect
to its vardef, is the real problem. The benefit of this CL is that
it fixes the immediate issue, is small, and obviously won't break
anything. A real fix for this issue is much more invasive.
The go binary is unchanged in size.
This situation just doesn't occur very often.
Fixes#28445
Change-Id: I13a765e13f075d5b6808a355ef3c43cdd7cd47b6
Reviewed-on: https://go-review.googlesource.com/c/153641
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
When functions are inlined, for instructions in the inlined body, does
-S print the location of the call, or the location of the body? Right
now, we do the former. I'd like to do the latter by default, it makes
much more sense when reading disassembly. With mid-stack inlining
enabled in more cases, this quandry will come up more often.
The original behavior is still available with -S=2. Some tests
use this mode (so they can find assembly generated by a particular
source line).
This helped me with understanding what the compiler was doing
while fixing #29007.
Change-Id: Id14a3a41e1b18901e7c5e460aa4caf6d940ed064
Reviewed-on: https://go-review.googlesource.com/c/153241
Reviewed-by: David Chase <drchase@google.com>
IsSliceInBounds(x, y) asserts that y is not negative, but
there were cases where this is not true. Change code
generation to ensure that this is true when it's not obviously
true. Prove phase cleans a few of these out.
With this change the compiler text section is 0.06% larger,
that is, not very much. Benchmarking still TBD, may need
to wait for access to a benchmarking box (next week).
Also corrected run.go to handle '?' in -update_errors output.
Fixes#28797.
Change-Id: Ia8af90bc50a91ae6e934ef973def8d3f398fac7b
Reviewed-on: https://go-review.googlesource.com/c/152477
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Currently,
for i := range a {
a[i] = nil
}
will compile to have write barriers even if a is a slice of pointers
to go:notinheap types. This happens because the optimization that
transforms this into a memclr only asks it a's element type has
pointers, and not if it specifically has heap pointers.
Fix this by changing arrayClear to use HasHeapPointer instead of
types.Haspointers. We probably shouldn't have both of these functions,
since a pointer to a notinheap type is effectively a uintptr, but
that's not going to change in this CL.
Change-Id: I284b85bdec6ae1e641f894e8f577989facdb0cf1
Reviewed-on: https://go-review.googlesource.com/c/152723
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Previously, when a function signature had defined a non-final variadic
parameter, the error message always referred to the type associated with that
parameter. However, if the offending parameter's name was part of an identifier
list with a variadic type, one could misinterpret the message, thinking the
problem had been with one of the other names in the identifer list.
func bar(a, b ...int) {}
clear ~~~~~~~^ ^~~~~~~~ confusing
This change updates the error message and sets the column position to that of
the offending parameter's name, if it exists.
Fixes#28450.
Change-Id: I076f560925598ed90e218c25d70f9449ffd9b3ea
Reviewed-on: https://go-review.googlesource.com/c/152417
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
For recursive functions, the parameters were iterated using
fn.Name.Defn.Func.Dcl, which does not include unnamed/blank
parameters. This results in a mismatch in formal-actual
assignments, for example,
func f(_ T, x T)
f(a, b) should result in { _=a, x=b }, but the escape analysis
currently sees only { x=a } and drops b on the floor. This may
cause b to not escape when it should (or a escape when it should
not).
Fix this by using fntype.Params().FieldSlice() instead, which
does include unnamed parameters.
Also add a sanity check that ensures all the actual parameters
are consumed.
Fixes#29000
Change-Id: Icd86f2b5d71e7ebbab76e375b7702f62efcf59ae
Reviewed-on: https://go-review.googlesource.com/c/152617
Reviewed-by: Keith Randall <khr@golang.org>
staticcopy of a struct or array should recursively call itself, not
staticassign.
This fixes an issue where a struct with a slice in it is copied during
static initialization. In this case, the backing array for the slice
is duplicated, and each copy of the slice refers to a different
backing array, which is incorrect. That issue has existed since at
least Go 1.2.
I'm not sure why this was never noticed. It seems like a pretty
obvious bug if anyone modifies the resulting slice.
In any case, we started to notice when the optimization in CL 140301
landed. Here is basically what happens in issue29013b.go:
1) The error above happens, so we get two backing stores for what
should be the same slice.
2) The code for initializing those backing stores is reused.
But not duplicated: they are the same Node structure.
3) The order pass allocates temporaries for the map operations.
For the first instance, things work fine and two temporaries are
allocated and stored in the OKEY nodes. For the second instance,
the order pass decides new temporaries aren't needed, because
the OKEY nodes already have temporaries in them.
But the order pass also puts a VARKILL of the temporaries between
the two instance initializations.
4) In this state, the code is technically incorrect. But before
CL 140301 it happens to work because the temporaries are still
correctly initialized when they are used for the second time. But then...
5) The new CL 140301 sees the VARKILLs and decides to reuse the
temporary for instance 1 map 2 to initialize the instance 2 map 1
map. Because the keys aren't re-initialized, instance 2 map 1
gets the wrong key inserted into it.
Fixes#29013
Change-Id: I840ce1b297d119caa706acd90e1517a5e47e9848
Reviewed-on: https://go-review.googlesource.com/c/152081
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
A Go user made a well-documented request for a slightly
lower threshold. I tested against a selection of other
people's benchmarks, and saw a tiny benefit (possibly noise)
at equally tiny cost, and no unpleasant surprises observed
in benchmarking.
I.e., might help, doesn't hurt, low risk, request was
delivered on a silver platter.
It did, however, change the behavior of one test because
now bytes.Buffer.Grow is eligible for inlining.
Updates #19348.
Change-Id: I85e3088a4911290872b8c6bda9601b5354c48695
Reviewed-on: https://go-review.googlesource.com/c/151977
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
This CL adds several test cases of arithmetic operations for
386/amd64/arm/arm64.
Change-Id: I362687c06249f31091458a1d8c45fc4d006b616a
Reviewed-on: https://go-review.googlesource.com/c/151897
Run-TryBot: Ben Shi <powerman1st@163.com>
Reviewed-by: Keith Randall <khr@golang.org>
While here, rename nonnegintconst to indexconst (because that's
what it is) and add Fatalf calls where we are not expecting the
indexconst call to fail, and fixed wrong comparison in smallintconst.
Fixes#23781.
Change-Id: I86eb13081c450943b1806dfe3ae368872f76639a
Reviewed-on: https://go-review.googlesource.com/c/151599
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
