We need to make sure that there's no possible faulting
instruction between a VarDef and that variable being
fully initialized. If there was, then anything scanning
the stack during the handling of that fault will see
a live but uninitialized variable on the stack.
If we have:
NilCheck p
VarDef x
x = *p
We can't rewrite that to
VarDef x
NilCheck p
x = *p
Particularly, even though *p faults on p==nil, we still
have to do the explicit nil check before the VarDef.
Fixes#32288
Change-Id: Ib8b88e6a5af3bf6f238ff5491ac86f53f3cf9fc9
Reviewed-on: https://go-review.googlesource.com/c/go/+/179239
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
The gccgo compiler crashes with int-to-string conversion with
large integer constant operand. CL 179777 is the fix. This CL
adds a test.
Updates #32347.
Change-Id: Id1d9dbbcdd3addca4636f1b9c5fdbc450cc48c1d
Reviewed-on: https://go-review.googlesource.com/c/go/+/179797
Run-TryBot: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This CL rewrites cmd/compile's package-level initialization ordering
algorithm to be compliant with the Go spec. See documentation in
initorder.go for details.
Incidentally, this CL also improves fidelity of initialization loop
diagnostics by including referenced functions in the emitted output
like go/types does.
Fixes#22326.
Change-Id: I7c9ac47ff563df4d4f700cf6195387a0f372cc7b
Reviewed-on: https://go-review.googlesource.com/c/go/+/170062
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
The code in #29218 resulted in an If block containing only its control.
That block was then converted by fuseIf into a plain block;
as a result, that control value was dead.
However, the control value was still present in b.Values.
This prevented further fusing of that block.
This change beefs up the check in fuseIf to allow fusing
blocks that contain only dead values (if any).
In the case of #29218, this enables enough extra
fusing that the control value could be eliminated,
allowing all values in turn to be eliminated.
This change also fuses 34 new blocks during make.bash.
It is not clear that this fixes every variant of #29218,
but it is a reasonable standalone change.
And code like #29218 is rare and fundamentally buggy,
so we can handle new instances if/when they actually occur.
Fixes#29218
Negligible toolspeed impact.
name old time/op new time/op delta
Template 213ms ± 3% 213ms ± 2% ~ (p=0.914 n=97+88)
Unicode 89.8ms ± 2% 89.6ms ± 2% -0.22% (p=0.045 n=93+95)
GoTypes 712ms ± 3% 709ms ± 2% -0.35% (p=0.023 n=95+95)
Compiler 3.24s ± 2% 3.23s ± 2% -0.30% (p=0.020 n=98+97)
SSA 10.0s ± 1% 10.0s ± 1% ~ (p=0.382 n=98+99)
Flate 135ms ± 3% 135ms ± 2% ~ (p=0.983 n=98+98)
GoParser 158ms ± 2% 158ms ± 2% ~ (p=0.170 n=99+99)
Reflect 447ms ± 3% 447ms ± 2% ~ (p=0.538 n=98+89)
Tar 189ms ± 2% 189ms ± 3% ~ (p=0.874 n=95+96)
XML 251ms ± 2% 251ms ± 2% ~ (p=0.434 n=94+96)
[Geo mean] 427ms 426ms -0.15%
name old user-time/op new user-time/op delta
Template 264ms ± 2% 265ms ± 2% ~ (p=0.075 n=96+90)
Unicode 119ms ± 6% 119ms ± 7% ~ (p=0.864 n=99+98)
GoTypes 926ms ± 2% 924ms ± 2% ~ (p=0.071 n=94+94)
Compiler 4.38s ± 2% 4.37s ± 2% -0.34% (p=0.001 n=98+97)
SSA 13.4s ± 1% 13.4s ± 1% ~ (p=0.693 n=90+93)
Flate 162ms ± 3% 161ms ± 2% ~ (p=0.163 n=99+99)
GoParser 186ms ± 2% 186ms ± 3% ~ (p=0.130 n=96+100)
Reflect 572ms ± 3% 572ms ± 2% ~ (p=0.608 n=97+97)
Tar 239ms ± 2% 239ms ± 3% ~ (p=0.999 n=93+91)
XML 302ms ± 2% 302ms ± 2% ~ (p=0.627 n=91+97)
[Geo mean] 540ms 540ms -0.08%
file before after Δ %
asm 4862704 4858608 -4096 -0.084%
compile 24001568 24001680 +112 +0.000%
total 132520780 132516796 -3984 -0.003%
file before after Δ %
cmd/compile/internal/gc.a 8887638 8887596 -42 -0.000%
cmd/compile/internal/ssa.a 29995056 29998986 +3930 +0.013%
cmd/internal/obj/wasm.a 209444 203652 -5792 -2.765%
total 129471798 129469894 -1904 -0.001%
Change-Id: I2d18f9278e68b9766058ae8ca621e844f9d89dd8
Reviewed-on: https://go-review.googlesource.com/c/go/+/177140
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
This test was designed for #15609 and didn't consider nacl. It's not
worth adding new +build-guarded assembly files in issue15609.dir for
nacl, especially as nacl is going away.
Fixes#32206
Change-Id: Ic5bd48b4f790a1f7019100b8a72d4688df75512f
Reviewed-on: https://go-review.googlesource.com/c/go/+/178698
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Currently, some tests under test/fixedbugs never run:
$ for d in test/fixedbugs/*.dir; do
! test -f "${d%.dir}.go" && echo "$d"
done
test/fixedbugs/issue15071.dir
test/fixedbugs/issue15609.dir
test/fixedbugs/issue29612.dir
Because they missed the corresponding ".go" file, so "go run run.go"
will skip them.
Add missing ".go" files for those tests to make sure they will be
collected and run.
While at it, add another action "runindir", which does "go run ."
inside the t.goDirName then check the output.
Change-Id: I88000b3663a6a615d90c1cf11844ea0377403e3d
Reviewed-on: https://go-review.googlesource.com/c/go/+/177798
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
As an optimization, function literals capture variables by value when
they're not assigned and their address has not been taken. Because
result parameters are implicitly assigned through return statements
(which do not otherwise set the "assigned" flag), result parameters
are explicitly handled to always capture by reference.
However, the logic was slightly mistaken because it was only checking
if the variable in the immediately enclosing context was a return
parameter, whereas in a multiply-nested function literal it would
itself be another closure variable (PAUTOHEAP) rather than a return
parameter (PPARAMOUT).
The fix is to simply test the outermost variable, like the rest of the
if statement's tests were already doing.
Fixes#32175.
Change-Id: Ibadde033ff89a1b47584b3f56c0014d8e5a74512
Reviewed-on: https://go-review.googlesource.com/c/go/+/178541
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
First, remove the randomization of initialization order.
Then, revert to source code order instead of sorted package path order.
This restores the behavior that was in 1.12.
A larger change which will implement the suggestion in #31636 will
wait for 1.14. It's too complicated for 1.13 at this point (it has
tricky interactions with plugins).
Fixes#31636
Change-Id: I35b48e8cc21cf9f93c0973edd9193d2eac197628
Reviewed-on: https://go-review.googlesource.com/c/go/+/178297
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
typecheck type alias always replaces the original definition of the symbol.
This is wrong behavior because if the symbol's definition is replaced by a
local type alias, it ends up being written to compiled file as an alias,
instead of the original type.
To fix, only replace the definition of symbol with global type alias.
Fixes#31959
Change-Id: Id85a15e8a9d6a4b06727e655a95dc81e63df633a
Reviewed-on: https://go-review.googlesource.com/c/go/+/177378
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.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#31987
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>
MOVBstore's value argument is a value, not a flag. We are storing
a byte so just use UInt8.
Fixes#31915.
Change-Id: Id799e5f44efc3a9c3d8480f6f25ad032c2a631bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/176719
Run-TryBot: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
This change is a simple work-around to avoid a compiler crash
and provide a reasonable error message. A future change should
fix the root cause for this problem.
Fixes#23823.
Change-Id: Ifc80d9f4d35e063c378e54d5cd8d1cf4c0d2ec6a
Reviewed-on: https://go-review.googlesource.com/c/go/+/175518
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Before this CL we used to panic with "nil pointer dereference" because
the value we're calling assignTo on is the zero Value. Provide a better
error message.
Fixes#28748
Change-Id: I7dd4c9e30b599863664d91e78cc45878d8b0052e
Reviewed-on: https://go-review.googlesource.com/c/go/+/175440
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
golang.org/cl/174498 add ONAME case to isStaticCompositeLiteral, to
detect global variable as compile-time constant.
It does report wrong for struct field, e.g:
o := one{i: two{i: 42}.i}
field i in two{i: 42} was reported as static composite literal, while it
should not.
In general, adding ONAME case for isStaticCompositeLiteral is probably
wrong.
Fixes#31782
Change-Id: Icde7d43bbb002b75df5c52b948b7126a4265e07b
Reviewed-on: https://go-review.googlesource.com/c/go/+/174837
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
golang.org/cl/174498 removes dynamic map entry handling in maplit, by
filtering the static entry only. It panics if it see a dynamic entry.
It relies on order to remove all dynamic entries.
But after recursively call order on the statics, some static entries
become dynamic, e.g OCONVIFACE node:
type i interface {
j()
}
type s struct{}
func (s) j() {}
type foo map[string]i
var f = foo{
"1": s{},
}
To fix it, we recursively call order on each static entry, if it changed
to dynamic, put entry to dynamic then.
Fixes#31777
Change-Id: I1004190ac8f2d1eaa4beb6beab989db74099b025
Reviewed-on: https://go-review.googlesource.com/c/go/+/174777
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
In the statement x = a[i], the index panic should appear to come from
the line number of the '['. Previous to this CL we sometimes used the
line number of the '=' instead.
Fixes#29504
Change-Id: Ie718fd303c1ac2aee33e88d52c9ba9bcf220dea1
Reviewed-on: https://go-review.googlesource.com/c/go/+/174617
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Any code that imports the testing package forces the testing flags to be
defined, even in non-test binaries. People work around this today by
defining a copy of the testing.TB interface just to avoid importing
testing.
Fix this by moving flag registration into a new function, testing.Init.
Delay calling Init until the testing binary begins to run, in
testing.MainStart.
Init is exported for cases where users need the testing flags to be
defined outside of a "go test" context. In particular, this may be
needed where testing.Benchmark is called outside of a test.
Fixes#21051
Change-Id: Ib7e02459e693c26ae1ba71bbae7d455a91118ee3
Reviewed-on: https://go-review.googlesource.com/c/go/+/173722
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
We already skipped blank field initialization in non-global contexts.
This change makes the global context treatment match.
Fixes#31546
Change-Id: I40acce49b0a9deb351ae0da098f4c114e425ec63
Reviewed-on: https://go-review.googlesource.com/c/go/+/173723
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This pair of packages caused a crash in gollvm, due to a glitch in the
way the front end handles empty/non-name parameters for functions that
are inline candidates.
Updates #31637.
Change-Id: I571c0658a00974dd36025e571638c0c836a3cdfd
Reviewed-on: https://go-review.googlesource.com/c/go/+/173617
Run-TryBot: Benny Siegert <bsiegert@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@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#31252
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>
The special case logic for go/defer arguments in Escape.call was
scattered around a bit and was somewhat inconsistently handled across
different types of function calls and parameters. This CL pulls the
logic out into a separate callStmt method that's used uniformly for
all kinds of function calls and arguments.
Fixes#31573.
Change-Id: Icdcdf611754dc3fcf1af7cb52879fb4b73a7a31f
Reviewed-on: https://go-review.googlesource.com/c/go/+/173019
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
In typecheckclosure, a xfunc node will be put to xtop. But that node can
be shared between multiple closures, like in a const declaration group:
const (
x = unsafe.Sizeof(func() {})
y
)
It makes a xfunc node appears multiple times in xtop, causing duplicate
initLSym run.
To fix this issue, we only do typecheck for xfunc one time, and setup
closure node earlier in typecheckclosure process.
Fixes#30709
Change-Id: Ic924a157ee9f3e5d776214bef5390849ddc8aab9
Reviewed-on: https://go-review.googlesource.com/c/go/+/172298
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The new escape analysis implementation tries to emit debugging
diagnostics that are compatible with the existing implementation, but
there's a handful of cases that are easier to handle by updating the
test expectations instead.
For regress tests that need updating, the original file is copied to
oldescapeXXX.go.go with -newescape=false added to the //errorcheck
line, while the file is updated in place with -newescape=true and new
test requirements.
Notable test changes:
1) escape_because.go looks for a lot of detailed internal debugging
messages that are fairly particular to how esc.go works and that I
haven't attempted to port over to escape.go yet.
2) There are a lot of "leaking param: x to result ~r1 level=-1"
messages for code like
func(p *int) *T { return &T{p} }
that were simply wrong. Here &T must be heap allocated unconditionally
(because it's being returned); and since p is stored into it, p
escapes unconditionally too. esc.go incorrectly reports that p escapes
conditionally only if the returned pointer escaped.
3) esc.go used to print each "leaking param" analysis result as it
discovered them, which could lead to redundant messages (e.g., that a
param leaks at level=0 and level=1). escape.go instead prints
everything at the end, once it knows the shortest path to each sink.
4) esc.go didn't precisely model direct-interface types, resulting in
some values unnecessarily escaping to the heap when stored into
non-escaping interface values.
5) For functions written in assembly, esc.go only printed "does not
escape" messages, whereas escape.go prints "does not escape" or
"leaking param" as appropriate, consistent with the behavior for
functions written in Go.
6) 12 tests included "BAD" annotations identifying cases where esc.go
was unnecessarily heap allocating something. These are all fixed by
escape.go.
Updates #23109.
Change-Id: Iabc9eb14c94c9cadde3b183478d1fd54f013502f
Reviewed-on: https://go-review.googlesource.com/c/go/+/170447
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
For a failed interface conversion not in ",ok" form, getitab
calls itab.init to get the name of the missing method for the
panic message. itab.init will try to find the methods, populate
the method table as it goes. When some method is missing, it sets
itab.fun[0] to 0 before return. There is a small window that
itab.fun[0] could be non-zero.
If concurrently, another goroutine tries to do the same interface
conversion, it will read the same itab's fun[0]. If this happens
in the small window, it sees a non-zero fun[0] and thinks the
conversion succeeded, which is bad.
Fix the race by setting fun[0] to non-zero only when we know the
conversion succeeds. While here, also simplify the syntax
slightly.
Fixes#31419.
Change-Id: Ied34d3043079eb933e330c5877b85e13f98f1916
Reviewed-on: https://go-review.googlesource.com/c/go/+/171759
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Some var declarations return "extra expression" or "missing expression"
errors when they should return “assignment mismatch” instead. Change
the returned error messages to exhibit the desired behavior.
Fixes#30085.
Change-Id: I7189355fbb0f976d70100779db4f81a9ae64fb11
Reviewed-on: https://go-review.googlesource.com/c/go/+/161558
Reviewed-by: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
For most nodes (e.g., OPTRLIT, OMAKESLICE, OCONVIFACE), escape
analysis prints "escapes to heap" or "does not escape" to indicate
whether that node's allocation can be heap or stack allocated.
These messages are also emitted for OADDR, even though OADDR does not
actually allocate anything itself. Moreover, it's redundant because
escape analysis already prints "moved to heap" diagnostics when an
OADDR node like "&x" causes x to require heap allocation.
Because OADDR nodes don't allocate memory, my escape analysis rewrite
doesn't naturally emit the "escapes to heap" / "does not escape"
diagnostics for them. It's also non-trivial to replicate the exact
semantics esc.go uses for OADDR.
Since there are so many of these messages, I'm disabling them in this
CL by themselves. I modified esc.go to suppress the Warnl calls
without any other behavior changes, and then used a shell script to
automatically remove any ERROR messages mentioned by run.go in
"missing error" or "no match for" lines.
Fixes#16300.
Updates #23109.
Change-Id: I3993e2743c3ff83ccd0893f4e73b366ff8871a57
Reviewed-on: https://go-review.googlesource.com/c/go/+/170319
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>
The bug in 29612 is that there are two similar-looking anonymous interface
types in two different packages, ./p1/ssa and ./p2/ssa:
v.(interface{ foo() }).foo()
These types should be treated differently because the unexported method
makes the types different (according to the spec).
But when generating the type descriptors for those two types, they
both have the name "interface { ssa.foo() }". They thus get the same
symbol, and the linker happily unifies them. It picks an arbitrary one
for the runtime to use, but that breaks conversions from concrete types
that have a foo method from the package which had its interface type
overwritten.
We need to encode the metadata symbol for unexported methods as package
path qualified (The same as we did in CL 27791 for struct fields).
So switching from FmtUnsigned to Fmtleft by default fixes the issue.
In case of generating namedata, FmtUnsigned is used.
The benchmark result ends up in no significant change of compiled binary
compare to the immediate parent.
Fixes#29612
Change-Id: I775aff91ae4a1bb16eb18a48d55e3b606f3f3352
Reviewed-on: https://go-review.googlesource.com/c/go/+/170157
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Typechecking treats all untyped numbers as integers for the purposes
of validating operators. However, when I refactoring constant
operation evalution in golang.org/cl/139901, I mistakenly interpreted
that the only invalid case that needed to be preserved was % (modulo)
on floating-point values.
This CL restores the other remaining cases that were dropped from that
CL. It also uses the phrasing "invalid operation" instead of "illegal
constant expression" for better consistency with the rest of
cmd/compile and with go/types.
Lastly, this CL extends setconst to recognize failed constant folding
(e.g., division by zero) so that we can properly mark those
expressions as broken rather than continuing forward with bogus values
that might lead to further spurious errors.
Fixes#31060.
Change-Id: I1ab6491371925e22bc8b95649f1a0eed010abca6
Reviewed-on: https://go-review.googlesource.com/c/go/+/169719
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Update the issue 30908 test to work with the no-opt builder
(this requires a corresponding change in the linker as well).
As part of this change, 'rundir' tests are now linked without
passing "-w" to the linker.
Updates #30908.
Fixes#31034.
Change-Id: Ic776e1607075c295e409e1c8230aaf55a79a6323
Reviewed-on: https://go-review.googlesource.com/c/go/+/169161
Reviewed-by: Ian Lance Taylor <iant@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#30956
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>
Some special-case code paths in order.go didn't expect OCALLFUNC to
have Ninit; in particular, OAS2FUNC and ODEFER/OGO failed to call
o.init on their child OCALLFUNC node. This resulted in not all of the
AST being properly ordered.
This was noticed because order is responsible for introducing an
invariant around how OAPPEND is used, which is enforced by walk.
However, there were perhaps simpler cases (e.g., simple order of
evaluation) that were being silently miscompiled.
Fixes#31010.
Change-Id: Ib928890ab5ec2aebd8e30a030bc2b404387f9123
Reviewed-on: https://go-review.googlesource.com/c/go/+/169257
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
New test case designed to mimic the code in issue 30908, which
features duplicate but non-indentical DWARF abstract subprogram DIEs.
Updates #30908.
Change-Id: Iacb4b53e6a988e46c801cdac236cef883c553f8f
Reviewed-on: https://go-review.googlesource.com/c/go/+/168957
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: David Chase <drchase@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.
Fixes#30977.
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>
The name change init -> init.ializers was initially required for
initialization code.
With CL 161337 there's no wrapper code any more, there's a data
structure instead (named .inittask). So we can go back to just
plain init appearing in tracebacks.
RELNOTE=yes
Update #29919. Followon to CL 161337.
Change-Id: I5a4a49d286df24b53b2baa193dfda482f3ea82a5
Reviewed-on: https://go-review.googlesource.com/c/go/+/167780
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Instead of writing an init function per package that does the same
thing for every package, just write that implementation once in the
runtime. Change the compiler to generate a data structure that encodes
the required initialization operations.
Reduces cmd/go binary size by 0.3%+. Most of the init code is gone,
including all the corresponding stack map info. The .inittask
structures that replace them are quite a bit smaller.
Most usefully to me, there is no longer an init function in every -S output.
(There is an .inittask global there, but it's much less distracting.)
After this CL we could change the name of the "init.ializers" function
back to just "init".
Update #6853
R=go1.13
Change-Id: Iec82b205cc52fe3ade4d36406933c97dbc9c01b1
Reviewed-on: https://go-review.googlesource.com/c/go/+/161337
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
golang.org/cl/166983 started serializing the Ninit field of OCALL
nodes within function inline bodies (necessary to fix a regression in
building crypto/ecdsa with -gcflags=-l=4), but this means the Ninit
field needs to be typechecked when the imported function body is used.
It's unclear why this wasn't necessary for the crypto/ecdsa
regression.
Fixes#30907.
Change-Id: Id5f0bf3c4d17bbd6d5318913b859093c93a0a20c
Reviewed-on: https://go-review.googlesource.com/c/go/+/168199
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
A few examples (for accessing a slice of length 3):
s[-1] runtime error: index out of range [-1]
s[3] runtime error: index out of range [3] with length 3
s[-1:0] runtime error: slice bounds out of range [-1:]
s[3:0] runtime error: slice bounds out of range [3:0]
s[3:-1] runtime error: slice bounds out of range [:-1]
s[3:4] runtime error: slice bounds out of range [:4] with capacity 3
s[0:3:4] runtime error: slice bounds out of range [::4] with capacity 3
Note that in cases where there are multiple things wrong with the
indexes (e.g. s[3:-1]), we report one of those errors kind of
arbitrarily, currently the rightmost one.
An exhaustive set of examples is in issue30116[u].out in the CL.
The message text has the same prefix as the old message text. That
leads to slightly awkward phrasing but hopefully minimizes the chance
that code depending on the error text will break.
Increases the size of the go binary by 0.5% (amd64). The panic functions
take arguments in registers in order to keep the size of the compiled code
as small as possible.
Fixes#30116
Change-Id: Idb99a827b7888822ca34c240eca87b7e44a04fdd
Reviewed-on: https://go-review.googlesource.com/c/go/+/161477
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
This is a re-attempt at CL 153841, which caused two regressions:
1. crypto/ecdsa failed to build with -gcflags=-l=4. This was because
when "t1, t2, ... := g(); f(t1, t2, ...)" was exported, we were losing
the first assignment from the call's Ninit field.
2. net/http/pprof failed to run with -gcflags=-N. This is due to a
conflict with CL 159717: as of that CL, package-scope initialization
statements are executed within the "init.ializer" function, rather
than the "init" function, and the generated temp variables need to be
moved accordingly too.
[Rest of description is as before.]
This CL moves order.go's copyRet logic for rewriting f(g()) into t1,
t2, ... := g(); f(t1, t2, ...) earlier into typecheck. This allows the
rest of the compiler to stop worrying about multi-value functions
appearing outside of OAS2FUNC nodes.
This changes compiler behavior in a few observable ways:
1. Typechecking error messages for builtin functions now use general
case error messages rather than unnecessarily differing ones.
2. Because f(g()) is rewritten before inlining, saved inline bodies
now see the rewritten form too. This could be addressed, but doesn't
seem worthwhile.
3. Most notably, this simplifies escape analysis and fixes a memory
corruption issue in esc.go. See #29197 for details.
Fixes#15992.
Fixes#29197.
Change-Id: I930b10f7e27af68a0944d6c9bfc8707c3fab27a4
Reviewed-on: https://go-review.googlesource.com/c/go/+/166983
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
The only ways to construct an OLITERAL node are (1) a basic literal
from the source package, (2) constant folding within evconst (which
only folds Go language constants), (3) the universal "nil" constant,
and (4) implicit conversions of nil to some concrete type.
Passes toolstash-check.
Change-Id: I30fc6b07ebede7adbdfa4ed562436cbb7078a2ff
Reviewed-on: https://go-review.googlesource.com/c/go/+/166981
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
The CL 164718 adds new condition flags for floating-point comparisons
in arm64 backend, but dose not add the handling in rewrite.go for
corresponding Ops, which causes issue 30679. And this CL fixes this
issue.
Fixes#30679
Change-Id: I8acc749f78227c3e9e74fa7938f05fb442fb62c6
Reviewed-on: https://go-review.googlesource.com/c/go/+/166579
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
New test for issue 30659 (compilation error due to bad
export data).
Updates #30659.
Change-Id: I2541ee3c379e5b22033fea66bb4ebaf720cc5e1f
Reviewed-on: https://go-review.googlesource.com/c/go/+/166917
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
First the insidious bug:
var n uintptr
for n := elemPtrs; n > 120; n -= 120 {
prog = append(prog, 120)
prog = append(prog, mask[:15]...)
mask = mask[15:]
}
prog = append(prog, byte(n))
prog = append(prog, mask[:(n+7)/8]...)
The := breaks this code, because the n after the loop is always 0!
We also do need to handle field padding correctly. In particular
the old padding code doesn't correctly handle fields that are not
a multiple of a pointer in size.
Fixes#30606.
Change-Id: Ifcab9494dc25c20116753c5d7e0145d6c2053ed8
Reviewed-on: https://go-review.googlesource.com/c/go/+/165860
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
They are missing a stop byte at the end.
Normally this doesn't matter, but when including a GC program
in another GC program, we strip the last byte. If that last byte
wasn't a stop byte, then we've thrown away part of the program
we actually need.
Fixes#30606
Change-Id: Ie9604beeb84f7f9442e77d31fe64c374ca132cce
Reviewed-on: https://go-review.googlesource.com/c/go/+/165857
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
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#30566
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>
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.
Fixes#30476.
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>
This CL moves order.go's copyRet logic for rewriting f(g()) into t1,
t2, ... = g(); f(t1, t2, ...) earlier into typecheck. This allows the
rest of the compiler to stop worrying about multi-value functions
appearing outside of OAS2FUNC nodes.
This changes compiler behavior in a few observable ways:
1. Typechecking error messages for builtin functions now use general
case error messages rather than unnecessarily differing ones.
2. Because f(g()) is rewritten before inlining, saved inline bodies
now see the rewritten form too. This could be addressed, but doesn't
seem worthwhile.
3. Most notably, this simplifies escape analysis and fixes a memory
corruption issue in esc.go. See #29197 for details.
Fixes#15992.
Fixes#29197.
Change-Id: I86a70668301efeec8fbd11fe2d242e359a3ad0af
Reviewed-on: https://go-review.googlesource.com/c/153841
Reviewed-by: Robert Griesemer <gri@golang.org>
isGoConst could spuriously return true for variables that shadow a
constant declaration with the same name.
Because even named constants are always represented by OLITERAL nodes,
the easy fix is to just ignore ONAME nodes in isGoConst. We can
similarly ignore ONONAME nodes.
Confirmed that k8s.io/kubernetes/test/e2e/storage builds again with
this fix.
Fixes#30430.
Change-Id: I899400d749982d341dc248a7cd5a18277c2795ec
Reviewed-on: https://go-review.googlesource.com/c/164319
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
If a type switch case expression has failed typechecking, the case body is
likely to also fail with confusing or spurious errors. Suppress
typechecking the case body when this happens.
Fixes#28926
Change-Id: Idfdb9d5627994f2fd90154af1659e9a92bf692c4
Reviewed-on: https://go-review.googlesource.com/c/158617
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Consistent logic for handling both duplicate map keys and case values,
and eliminates ad hoc value hashing code.
Also makes cmd/compile consistent with go/types's handling of
duplicate constants (see #28085), which is at least an improvement
over the status quo even if we settle on something different for the
spec.
As a side effect, this also suppresses cmd/compile's warnings about
duplicate nils in (non-interface expression) switch statements, which
was technically never allowed by the spec anyway.
Updates #28085.
Updates #28378.
Change-Id: I176a251e770c3c5bc11c2bf8d1d862db8f252a17
Reviewed-on: https://go-review.googlesource.com/c/152544
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
When looking for the field specified in a composite literal, check that
the specified name is actually a field and not a method.
Fixes#29855.
Change-Id: Id77666e846f925907b1eec64213b1d25af8a2466
Reviewed-on: https://go-review.googlesource.com/c/158938
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
There are several places where a new (internal) complex constant is allocated
via new(Mpcplx) rather than newMpcmplx(). The problem with using new() is that
the Mpcplx data structure's Real and Imag components don't get initialized with
an Mpflt of the correct precision (they have precision 0, which may be adjusted
later).
In all cases but one, the components of those complex constants are set using
a Set operation which "inherits" the correct precision from the value that is
being set.
But when creating a complex value for an imaginary literal, the imaginary
component is set via SetString which assumes 64bits of precision by default.
As a result, the internal representation of 0.01i and complex(0, 0.01) was
not correct.
Replaced all used of new(Mpcplx) with newMpcmplx() and added a new test.
Fixes#30243.
Change-Id: Ife7fd6ccd42bf887a55c6ce91727754657e6cb2d
Reviewed-on: https://go-review.googlesource.com/c/163000
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
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>
Allow shifts by signed amounts. Panic if the shift amount is negative.
TODO: We end up doing two compares per shift, see Ian's comment
https://github.com/golang/go/issues/19113#issuecomment-443241799 that
we could do it with a single comparison in the normal case.
The prove pass mostly handles this code well. For instance, it removes the
<0 check for cases like this:
if s >= 0 { _ = x << s }
_ = x << len(a)
This case isn't handled well yet:
_ = x << (y & 0xf)
I'll do followon CLs for unhandled cases as needed.
Update #19113
R=go1.13
Change-Id: I839a5933d94b54ab04deb9dd5149f32c51c90fa1
Reviewed-on: https://go-review.googlesource.com/c/158719
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
This CL introduces compiler support for the new binary and octal integer
literals, hexadecimal floats, and digit separators for all number literals.
The new Go 2 number literal scanner accepts the following liberal format:
number = [ prefix ] digits [ "." digits ] [ exponent ] [ "i" ] .
prefix = "0" [ "b" |"B" | "o" | "O" | "x" | "X" ] .
digits = { digit | "_" } .
exponent = ( "e" | "E" | "p" | "P" ) [ "+" | "-" ] digits .
If the number starts with "0x" or "0X", digit is any hexadecimal digit;
otherwise, digit is any decimal digit. If the accepted number is not valid,
errors are reported accordingly.
See the new test cases in scanner_test.go for a selection of valid and
invalid numbers and the respective error messages.
R=Go1.13
Updates #12711.
Updates #19308.
Updates #28493.
Updates #29008.
Change-Id: Ic8febc7bd4dc5186b16a8c8897691e81125cf0ca
Reviewed-on: https://go-review.googlesource.com/c/157677
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Russ Cox <rsc@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>
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>
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>
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>
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>
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>
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>
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>
Don't convert values that aren't Go constants, like
uintptr(unsafe.Pointer(nil)), to a literal constant. This avoids
assuming they are constants for things like indexing, array sizes,
case duplication, etc.
Also, nil is an allowed duplicate in switches. CTNILs aren't Go constants.
Fixes#28078Fixes#28079
Change-Id: I9ab8af47098651ea09ef10481787eae2ae2fb445
Reviewed-on: https://go-review.googlesource.com/c/151320
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
When a slice composite literal is sparse, initialize it dynamically
instead of statically.
s := []int{5:5, 20:20}
To initialize the backing store for s, use 2 constant writes instead
of copying from a static array with 21 entries.
This CL also fixes pathologies in the compiler when the slice is
*very* sparse.
Fixes#23780
Change-Id: Iae95c6e6f6a0e2994675cbc750d7a4dd6436b13b
Reviewed-on: https://go-review.googlesource.com/c/151319
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Abort evconst if its argument isn't a Go constant. The SSA backend
will do the optimizations in question later. They tend to be weird
cases, like uintptr(unsafe.Pointer(uintptr(1))).
Fix OADDSTR and OCOMPLEX cases in isGoConst.
OADDSTR has its arguments in n.List, not n.Left and n.Right.
OCOMPLEX might have a 2-result function as its arg in List[0]
(in which case it isn't a Go constant).
Fixes#24760
Change-Id: Iab312d994240d99b3f69bfb33a443607e872b01d
Reviewed-on: https://go-review.googlesource.com/c/151338
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
In assembly free packages (aka "complete" or "pure go"), allow
bodyless functions if they are linkname'd to something else.
Presumably the thing the function is linkname'd to has a definition.
If not, the linker will complain. And linkname is unsafe, so we expect
users to know what they are doing.
Note this handles only one direction, where the linkname directive
is in the local package. If the linkname directive is in the remote
package, this CL won't help. (See os/signal/sig.s for an example.)
Fixes#23311
Change-Id: I824361b4b582ee05976d94812e5b0e8b0f7a18a6
Reviewed-on: https://go-review.googlesource.com/c/151318
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
This commit allows the runtime to handle 64bits addresses returned by
mmap syscall on AIX.
Mmap syscall returns addresses on 59bits on AIX. But the Arena
implementation only allows addresses with less than 48 bits.
This commit increases the arena size up to 1<<60 for aix/ppc64.
Update: #25893
Change-Id: Iea72e8a944d10d4f00be915785e33ae82dd6329e
Reviewed-on: https://go-review.googlesource.com/c/138736
Reviewed-by: Austin Clements <austin@google.com>
When using soft-float, OMUL might be rewritten to function call
so we should ensure it was evaluated first.
Fixes#28688
Change-Id: I30b87501782fff62d35151f394a1c22b0d490c6c
Reviewed-on: https://go-review.googlesource.com/c/148837
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Because run.go doesn't pass the package being compiled to the compiler
via the -p flag, it can't match up the main·f symbol from the
assembler with the "func f" stub in Go, so it doesn't produce the
correct assembly stub.
Fix this by removing the package prefix from the assembly definition.
Alternatively, we could make run.go pass -p to the compiler, but it's
nicer to remove these package prefixes anyway.
Should fix the linux-arm builder, which was broken by the introduction
of function ABIs in CL 147160.
Updates #27539.
Change-Id: Id62b7701e1108a21a5ad48ffdb5dad4356c273a6
Reviewed-on: https://go-review.googlesource.com/c/149483
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
