cmd/compile: unexpected nil dereference on s390x #38195

mundaym · 2020-04-01T10:17:39Z

This is a follow up issue to mikefarah/yq#394 which only appears to affect s390x. That issue indicates a bug in the compiler that affects Go 1.13.x and Go 1.14.x. Git bisect points to CL 173323 and if that CL is applied to Go 1.12.x then that release is also affected. I am fairly certain that CL 173323 isn't the root cause however. It appears to have revealed a latent bug.

To reproduce:

git clone https://gopkg.in/yaml.v3
cd yaml.v3
go test

You will see unexpected nil dereference panics in most of the tests. For example:

--- FAIL: ExampleUnmarshal_embedded (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x16a06c]

goroutine 1 [running]:
testing.(*InternalExample).processRunResult(0xc001477d08, 0x0, 0x0, 0x11961, 0x235940, 0x425c20, 0x0)
        /home/mndy/go/master/src/testing/example.go:89 +0x52a
testing.runExample.func2(0xbf99381e5b419297, 0x5bbc282, 0x43e0c0, 0xc00160e6e0, 0xc000010018, 0xc0000785a0, 0xc0005b5d08, 0xc001477d38)
        /home/mndy/go/master/src/testing/run_example.go:58 +0xfc
panic(0x235940, 0x425c20)
        /home/mndy/go/master/src/runtime/panic.go:969 +0x132
gopkg.in/yaml%2ev3.handleErr(0xc0005b5bd0)
        /home/mndy/go/yaml.v3/yaml.go:276 +0xae
panic(0x235940, 0x425c20)
        /home/mndy/go/master/src/runtime/panic.go:969 +0x132
gopkg.in/yaml%2ev3.yaml_parser_parse_node(0xc001437000, 0xc0014372b0, 0x100000000000080, 0xf)
        /home/mndy/go/yaml.v3/parserc.go:568 +0x49c
gopkg.in/yaml%2ev3.yaml_parser_state_machine(0xc001437000, 0xc0014372b0, 0x0)
        /home/mndy/go/yaml.v3/parserc.go:170 +0x1a4
gopkg.in/yaml%2ev3.yaml_parser_parse(0xc001437000, 0xc0014372b0, 0x3ff8e0bb560)
        /home/mndy/go/yaml.v3/parserc.go:129 +0x82
gopkg.in/yaml%2ev3.(*parser).peek(0xc001437000, 0x0)
        /home/mndy/go/yaml.v3/decode.go:102 +0x3c
gopkg.in/yaml%2ev3.(*parser).parse(0xc001437000, 0x0)
        /home/mndy/go/yaml.v3/decode.go:141 +0x36
gopkg.in/yaml%2ev3.(*parser).parseChild(0xc001437000, 0xc0001208c0, 0x0)
        /home/mndy/go/yaml.v3/decode.go:186 +0x2a
gopkg.in/yaml%2ev3.(*parser).document(0xc001437000, 0x300000000000030)
        /home/mndy/go/yaml.v3/decode.go:195 +0xa6
gopkg.in/yaml%2ev3.(*parser).parse(0xc001437000, 0x0)
        /home/mndy/go/yaml.v3/decode.go:151 +0x144
gopkg.in/yaml%2ev3.unmarshal(0xc0014145c0, 0x35, 0x40, 0x216740, 0xc00059c740, 0x40, 0x0, 0x0)
        /home/mndy/go/yaml.v3/yaml.go:161 +0x224
gopkg.in/yaml%2ev3.Unmarshal(...)
        /home/mndy/go/yaml.v3/yaml.go:89
gopkg.in/yaml%2ev3_test.ExampleUnmarshal_embedded()
        /home/mndy/go/yaml.v3/example_embedded_test.go:47 +0x88
testing.runExample(0x27966a, 0x19, 0x2842e0, 0x2806ca, 0x2e, 0x0, 0x0)
        /home/mndy/go/master/src/testing/run_example.go:62 +0x1a6
testing.runExamples(0xc001477ed8, 0x426de0, 0x1, 0x1, 0x0)
        /home/mndy/go/master/src/testing/example.go:44 +0x13c
testing.(*M).Run(0xc000136000, 0x0)
        /home/mndy/go/master/src/testing/testing.go:1221 +0x244
main.main()
        _testmain.go:71 +0x142

Things that 'fix' the issue:

Reverting CL 173323

Removing the 64-bit store multiple rules:

go/src/cmd/compile/internal/ssa/gen/S390X.rules

Lines 1948 to 1968 in 8e6a8d9

    
           (MOVDstore [i] {s} p w1 x:(MOVDstore [i-8] {s} p w0 mem)) 
        
             && p.Op != OpSB 
        
             && x.Uses == 1 
        
             && is20Bit(i-8) 
        
             && clobber(x) 
        
             -> (STMG2 [i-8] {s} p w0 w1 mem) 
        
           (MOVDstore [i] {s} p w2 x:(STMG2 [i-16] {s} p w0 w1 mem)) 
        
             && x.Uses == 1 
        
             && is20Bit(i-16) 
        
             && clobber(x) 
        
             -> (STMG3 [i-16] {s} p w0 w1 w2 mem) 
        
           (MOVDstore [i] {s} p w3 x:(STMG3 [i-24] {s} p w0 w1 w2 mem)) 
        
             && x.Uses == 1 
        
             && is20Bit(i-24) 
        
             && clobber(x) 
        
             -> (STMG4 [i-24] {s} p w0 w1 w2 w3 mem) 
        
           (STMG2 [i] {s} p w2 w3 x:(STMG2 [i-16] {s} p w0 w1 mem)) 
        
             && x.Uses == 1 
        
             && is20Bit(i-16) 
        
             && clobber(x) 
        
             -> (STMG4 [i-16] {s} p w0 w1 w2 w3 mem)

Using the -N (no opt) or -l=4 (max inlining) flags

None of these are the cause I think. I suspect an op is missing a clobberFlags mark or something like that. The store multiple ops should have the clobberFlags mark and currently don't but that doesn't appear to affect this issue (store multiple will only clobber flags if the displacement does not fit inside a int20 and this can only happen with autos - which would need a 512K stack frame).

The text was updated successfully, but these errors were encountered:

gopherbot · 2020-04-01T10:32:59Z

Change https://golang.org/cl/226759 mentions this issue: cmd/compile: mark 'store multiple' as clobbering flags on s390x

Store multiple instructions can clobber flags on s390x when the offset passed into the assembler is outside the range representable with a signed 20 bit integer. This is because the assembler uses the agfi instruction to implement the large offset. The assembler could use a different sequence of instructions, but for now just mark the instruction as 'clobberFlags' since this is risk free. Noticed while investigating #38195. No test yet since I'm not sure how to get this bug to trigger and I haven't seen it affect real code. Change-Id: I4a6ab96455a3ef8ffacb76ef0166b97eb40ff925 Reviewed-on: https://go-review.googlesource.com/c/go/+/226759 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>

mundaym · 2020-04-06T11:27:44Z

This looks like a bug in the register allocation for the function. v1909 is allocated to R5, then R5 is taken by v1408 but v1909 (still in R5) is then used as a source to v1368.

b1:
    v9 (455) = Arg <*yaml_parser_t> {parser} : parser[*yaml_parser_t] (parser[*yaml_parser_t], parser[*yaml_parser_t], parser[*yaml_parser_t], parser[*yaml_parser_t], parser[*yaml_parser_t], parser[*yaml_parser_t], parser[*yaml_parser_t], parser[*yaml_parser_t], parser[*yaml_parser_t], parser[*yaml_parser_t], parser[*yaml_parser_t], parser[*yaml_parser_t], parser[*yaml_parser_t])
    v10 (455) = Arg <*yaml_event_t> {event} : event[*yaml_event_t] (event[*yaml_event_t], event[*yaml_event_t], event[*yaml_event_t], event[*yaml_event_t], event[*yaml_event_t])
    v11 (455) = Arg <bool> {block} : block[bool] (block[bool])
    v12 (455) = Arg <bool> {indentless_sequence} : indentless_sequence[bool] (indentless_sequence[bool])
    v1 (?) = InitMem <mem>
    v2 (?) = SP <uintptr> : SP
    v1910 (458) = LoadReg <*yaml_parser_t> v9 : R0
    v15 (458) = MOVDstore <mem> [8] v2 v1910 v1
    v16 (+458) = CALLstatic <mem> {"".peek_token} [16] v15
    v18 (458) = MOVDload <*yaml_token_t> [16] v2 v16 : R0 (token[*yaml_token_t])
CGIJ {LessOrGreater} v18 → b2 b3 (likely) (+459)

b2: ← b1
    v3 (?) = SB <uintptr> : SB
    v1909 (463) = Copy <*yaml_token_t> v18 : R5               <=== R5 = v1909
    v28 (+463) = MOVDload <yaml_token_type_t> v1909 v16 : R6
CGIJ {Equal} v28 → b5 b4 (unlikely) (463)

b4: ← b2
    v1055 (477) = MOVDload <int> [24] v1909 v16 : R3
    v846 (477) = StoreReg <int> v1055 : .autotmp_110[int]
    v1070 (+477) = MOVDload <int> [16] v1909 v16 : R2
    v1905 (+477) = StoreReg <int> v1070 : .autotmp_111[int]
    v1049 (+478) = MOVDload <int> [8] v1909 v16 : R1
    v1903 (+478) = StoreReg <int> v1049 : .autotmp_112[int]
CGIJ {Equal} v28 → b17 b18 (unlikely) (+483)

b18: ← b4
CGIJ {Equal} v28 → b32 b88 (unlikely) (+504)

b88: ← b18
    v1472 (477) = Copy <int> v1055 : R4
    v1480 (477) = Copy <int> v1070 : R5                      <=== R5 = v1480
    v1237 (478) = Copy <int> v1049 : R6
    v1578 (?) = MOVDconst <bool> [0] : R7
    v1334 (?) = MOVDconst <int> [0] : R8
    v1412 (?) = MOVDconst <*byte> [0] : R9
    v1948 (?) = MOVDconst <int> [0] : R11
    v1407 (?) = MOVDconst <int> [0] : R12
    v1327 (?) = MOVDconst <*byte> [0] : R14
    v1326 (541) = MOVDconst <int> [0] : R0
    v1396 (541) = StoreReg <int> v1326 : tag_mark.column[int]
    v1323 (541) = MOVDconst <int> [0] : R1
    v1632 (541) = StoreReg <int> v1323 : tag_mark.line[int]
    v1627 (541) = MOVDconst <int> [0] : R2
    v1623 (541) = StoreReg <int> v1627 : tag_mark.index[int]
    v1368 (551) = Copy <*yaml_token_t> v1909 : R0            <=== R0 = v1909 !!!
    v675 (646) = MOVDconst <int> [0] : R3
    v1217 (646) = StoreReg <int> v675 : anchor.cap[int]
    v1216 (646) = MOVDconst <int> [0] : R4
    v873 (646) = StoreReg <int> v1216 : anchor.len[int]
    v954 (646) = MOVDconst <*byte> [0] : R5
    v1213 (646) = StoreReg <*byte> v954 : anchor.ptr[*byte]
    v1212 (660) = LoadReg <int> v846 : R3
    v964 (660) = LoadReg <int> v1905 : R2
    v974 (660) = Copy <int> v1237 : R1
    v1209 (556) = Copy <int> v1212 : R4
    v1208 (556) = Copy <int> v964 : R5
Plain → b31 (+504)

Any thoughts on what might be the issue here @randall77 @dr2chase @cherrymui?

mundaym · 2020-04-06T11:55:03Z

v1909 is created because v18 has been allocated to R0, which cannot be used as an address for loads or stores (it is always evaluated as 0 in address calculations).

This means that the value of v1909 (R5) and v18 (R0) both contain the same value. So I wonder if this is a subtle bug in the way we handle multiple registers containing the same value.

mundaym · 2020-04-06T13:19:02Z

To generate the ssa.html for the bad code you can use master at 815509a:

git clone https://gopkg.in/yaml.v3
cd yaml.v3
git checkout 9f266ea
GOSSAFUNC=yaml_parser_parse_node GOOS=linux GOARCH=s390x go test -c

The value numbers will be different but the pattern is the same:

b1:
   ...
   v18 (458) = MOVDload <*yaml_token_t> [16] v2 v16 : R0 (token[*yaml_token_t]) <== set R0=v18
CGIJ {LessOrGreater} v18 → b2 b3 (likely) (+459)

b2: ← b1
   ...
   v1476 (463) = Copy <*yaml_token_t> v18 : R5 <== set R5=v1476
   ...
CGIJ {Equal} v28 → b5 b4 (unlikely) (463)

b4: ← b2
   ...
CGIJ {Equal} v28 → b17 b18 (unlikely) (+483)

b18: ← b4
CGIJ {Equal} v28 → b32 b90 (unlikely) (+504)

b90: ← b18
   ...
   v1241 (477) = Copy <int> v1074 : R5 <== clobber v1476, set R5=v1241
   ...
   v1631 (541) = MOVDconst <int> [0] : R0 <== clobber v18 (might not be relevant), set R0=v1631
   ...
   v877 (551) = Copy <*yaml_token_t> v1476 : R0 <== use v1476 but instead get v1241
   ...
Plain → b31 (+504)

gopherbot · 2020-04-14T09:21:43Z

Change https://golang.org/cl/228060 mentions this issue: cmd/compile: error if register is reused when setting edge state

mundaym · 2020-04-14T14:44:18Z

I think the problem is this code - specifically the call to s.freeRegs:

go/src/cmd/compile/internal/ssa/regalloc.go

Lines 986 to 990 in 34e38ac

    
           if !regValLiveSet.contains(a.ID) { 
        
           	// Input is dead beyond the phi, deallocate 
        
           	// anywhere else it might live. 
        
           	s.freeRegs(s.values[a.ID].regs) 
        
           } else {

That call frees all the copies of the value that is the input to the phi - some of which might actually be live and used by other values (as in this case). If the free'd value is subsequently used to copy a different phi input then the append in this code (as opposed to a replace function) leads to two copies of the same register in the predecessors endRegs slice, which is an invalid state and causes weird problems:

go/src/cmd/compile/internal/ssa/regalloc.go

Line 1012 in 34e38ac

s.endRegs[p.ID] = append(s.endRegs[p.ID], endReg{r2, a, c})

CL 228060 adds an ICE if this invalid state is seen.

I think the solution to this specific bug is just to deallocate the register holding the input to the phi and leave any copies untouched.

In general though this code is really hairy and I'm still not really sure that even with this change it is correct. It might be worth spending some time simplifying how phi allocations work so there are fewer data structures to mentally track.

gopherbot · 2020-04-14T14:55:39Z

Change https://golang.org/cl/228061 mentions this issue: cmd/compile: fix deallocation of live value copies in regalloc

mundaym · 2020-04-14T19:04:18Z

Keith put a good summary of this issue in the CL for anyone following along:

Here's what I think the bug in the old code is:

s.freeRegs(s.values[a.ID].regs)

This frees those registers in the new block (the one with the phi), but it does not free those registers in the predecessor block. In particular, they are still in s.endRegs[p.ID].
But it does remove those registers from s.used, allowing them to be reused at the end of the predecessor block to make a copy. If, for a later phi, we choose one of those deallocated-in-the-current-block-but-not-in-the-predecessor-block registers, we get two things in the same register.

The fix looks correct. If we only deallocate registers that are in phiUsed, then those registers would never be reused during later phis. (We allocate from all - s.used - phiUsed.)

When setting the edge state in register allocation we should only be setting each register once. It is not possible for a register to hold multiple values at once. This CL converts the runtime error seen in #38195 into an internal compiler error (ICE). It is better for the compiler to fail than generate an incorrect program. The bug reported in #38195 is now exposed as: ./parserc.go:459:11: internal compiler error: 'yaml_parser_parse_node': R5 is already set (v1074/v1241) [stack trace] Updates #38195. Change-Id: Id95842fd850b95494cbd472b6fd5a55513ecacec Reviewed-on: https://go-review.googlesource.com/c/go/+/228060 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>

mundaym · 2020-04-14T19:18:48Z

@gopherbot please consider this for a backport

This is a bug in the register allocator that results in the compiled program behaving incorrectly. There is currently no workaround and it results in very hard to debug issues. It has only been seen to cause problems on s390x but theoretically it affects all platforms.

gopherbot · 2020-04-14T19:18:56Z

Backport issue(s) opened: #38442 (for 1.13), #38443 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

mundaym added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. arch-s390x Issues solely affecting the s390x architecture. labels Apr 1, 2020

mundaym added this to the Go1.15 milestone Apr 1, 2020

mundaym self-assigned this Apr 1, 2020

alicefr mentioned this issue Apr 6, 2020

All yq commands on linux s390x nil pointer exception mikefarah/yq#394

Closed

gopherbot closed this as completed in 382fe3e Apr 14, 2020

This was referenced Apr 14, 2020

cmd/compile: unexpected nil dereference on s390x [1.13 backport] #38442

Closed

cmd/compile: unexpected nil dereference on s390x [1.14 backport] #38443

Closed

mundaym mentioned this issue Jun 17, 2020

cmd/compile: s390x floating point <-> integer conversions clobbering the condition code #39651

Closed

Prashanth684 mentioned this issue Oct 22, 2020

fcct crashes on s390x with "panic: runtime error: invalid memory address or nil pointer dereference" coreos/butane#142

Closed

golang locked and limited conversation to collaborators Apr 14, 2021

gopherbot added the FrozenDueToAge label Apr 14, 2021

rsc unassigned mundaym Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/compile: unexpected nil dereference on s390x #38195

cmd/compile: unexpected nil dereference on s390x #38195

mundaym commented Apr 1, 2020

gopherbot commented Apr 1, 2020

mundaym commented Apr 6, 2020 •

edited

mundaym commented Apr 6, 2020

mundaym commented Apr 6, 2020 •

edited

gopherbot commented Apr 14, 2020

mundaym commented Apr 14, 2020

gopherbot commented Apr 14, 2020

mundaym commented Apr 14, 2020

mundaym commented Apr 14, 2020

gopherbot commented Apr 14, 2020

cmd/compile: unexpected nil dereference on s390x #38195

cmd/compile: unexpected nil dereference on s390x #38195

Comments

mundaym commented Apr 1, 2020

gopherbot commented Apr 1, 2020

mundaym commented Apr 6, 2020 • edited

mundaym commented Apr 6, 2020

mundaym commented Apr 6, 2020 • edited

gopherbot commented Apr 14, 2020

mundaym commented Apr 14, 2020

gopherbot commented Apr 14, 2020

mundaym commented Apr 14, 2020

mundaym commented Apr 14, 2020

gopherbot commented Apr 14, 2020

mundaym commented Apr 6, 2020 •

edited

mundaym commented Apr 6, 2020 •

edited