[GHC] #14999: unwinding info for stg_catch_frame is wrong
#14999: unwinding info for stg_catch_frame is wrong -------------------------------------+------------------------------------- Reporter: niteria | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: Keywords: | Operating System: Linux Architecture: x86_64 | Type of failure: Debugging (amd64) | information is incorrect Test Case: | Blocked By: Blocking: | Related Tickets: Differential Rev(s): | Wiki Page: -------------------------------------+------------------------------------- Minimized `stg_catch_frame` (`Small.cmm`): {{{ #define CATCH_FRAME 34 #define SIZEOF_StgCatchFrame (SIZEOF_StgHeader+16) INFO_TABLE_RET(stg_catch_frame, CATCH_FRAME, bits64 info_ptr, bits64 exceptions_blocked, gcptr handler) return (gcptr ret) { unwind Sp = Sp + SIZEOF_StgCatchFrame; return (ret); } }}} Compile `"inplace/bin/ghc-stage2" -O2 -g -c Small.cmm` using GHC HEAD. `objdump -D` for `stg_catch_frame_info`: {{{ 0000000000000010 <stg_catch_frame_info>: 10: 48 83 c5 18 add $0x18,%rbp 14: ff 65 00 jmpq *0x0(%rbp) }}} `readelf --debug-dump=frames-interp Small.o`: {{{ Contents of the .debug_frame section: 00000000 0000000000000014 ffffffff CIE "" cf=1 df=-8 ra=16 LOC CFA rbp rsp ra 0000000000000000 rbp+0 v+0 s c+0 00000018 000000000000002c 00000000 FDE cie=00000000 pc=000000000000000f..0000000000000017 LOC CFA rbp rsp ra 000000000000000f rbp+0 v+0 s c+0 000000000000000f rbp+24 v+0 s c+0 0000000000000010 rbp+0 v+0 s c+0 }}} **How do I know this is wrong?** http://www.dwarfstd.org/doc/dwarf-2.0.0.pdf has a nice example in Appendix 5 (page 101 of the pdf). The question that I had was if the CFA value at LOC is after or before the instruction at LOC executes. Appendix 5 clearly shows that it's **before**. Hence for LOC = 0x10 here CFA shouldn't have changed. It can and should change at LOC = 0x14. For comparison this is what `8.0.2` produced (and it worked more often): {{{ 0000000000000010 <stg_catch_frame_info>: 10: 48 83 c5 18 add $0x18,%rbp 14: ff 65 00 jmpq *0x0(%rbp) }}} {{{ Contents of the .debug_frame section: 00000000 0000000000000014 ffffffff CIE "" cf=1 df=-8 ra=16 LOC CFA rbp rsp ra 0000000000000000 rbp+0 v+0 s c+0 00000018 0000000000000024 00000000 FDE cie=00000000 pc=000000000000000f..0000000000000017 LOC CFA rbp rsp ra 000000000000000f rbp+0 v+0 s c+0 000000000000000f rbp+24 v+0 s c+0 }}} Debugging follows in the comments. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14999> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14999: unwinding info for stg_catch_frame is wrong -------------------------------------+------------------------------------- Reporter: niteria | Owner: niteria Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: Resolution: | Keywords: Operating System: Linux | Architecture: x86_64 Type of failure: Debugging | (amd64) information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by niteria): * owner: (none) => niteria -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14999#comment:1> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14999: unwinding info for stg_catch_frame is wrong -------------------------------------+------------------------------------- Reporter: niteria | Owner: niteria Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: Resolution: | Keywords: Operating System: Linux | Architecture: x86_64 Type of failure: Debugging | (amd64) information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by niteria): First thing is to understand how to read unwind information in Cmm. If you have: {{{ A // starts at addr X and ends at addr Y unwind Sp = Just Sp + 16; B // starts at addr Y and ends at addr Z }}} Does the unwind information reflect the state after `A` or `B`? If it reflects the state after `A` then you'd expect the LOC to be `Y`. If it reflects the state after `B` then you'd expect the LOC to be `Z`. I couldn't find a relevant note, so I resorted to experimentation. It appears that the unwind annotation reflects the state after `A`, but before `B`. With that in mind we can look at relevant Cmm dumps (`-ddump-cmm -ddump- opt-cmm -ddump-cmm-proc -ddump-cmm-verbose`): {{{ ==================== Parsed Cmm ==================== [stg_catch_frame_ret() // [R1] { info_tbl: [(c5, label: stg_catch_frame_info rep:tag:34 StackRep [True, False])] stack_info: arg_space: 32 updfr_space: Just 8 } {offset c5: // c1 _c1::I64 = I64[(old + 32)]; // CmmAssign _c2::I64 = I64[(old + 24)]; // CmmAssign _c3::P64 = P64[(old + 16)]; // CmmAssign _c4::P64 = R1; // CmmAssign //tick src<Small.cmm:(8,1)-(11,1)> unwind Sp = Just Sp + (8 + 16); // CmmUnwind R1 = _c4::P64; // CmmAssign call (P64[(old + 8)])(R1) args: 8, res: 0, upd: 8; // CmmCall } }] ==================== Post control-flow optimisations ==================== {offset c5: // c1 _c1::I64 = I64[(old + 32)]; // CmmAssign _c2::I64 = I64[(old + 24)]; // CmmAssign _c3::P64 = P64[(old + 16)]; // CmmAssign _c4::P64 = R1; // CmmAssign //tick src<Small.cmm:(8,1)-(11,1)> unwind Sp = Just Sp + (8 + 16); // CmmUnwind R1 = _c4::P64; // CmmAssign call (P64[(old + 8)])(R1) args: 8, res: 0, upd: 8; // CmmCall } ==================== Post common block elimination ==================== {offset c5: // c1 _c1::I64 = I64[(old + 32)]; // CmmAssign _c2::I64 = I64[(old + 24)]; // CmmAssign _c3::P64 = P64[(old + 16)]; // CmmAssign _c4::P64 = R1; // CmmAssign //tick src<Small.cmm:(8,1)-(11,1)> unwind Sp = Just Sp + (8 + 16); // CmmUnwind R1 = _c4::P64; // CmmAssign call (P64[(old + 8)])(R1) args: 8, res: 0, upd: 8; // CmmCall } ==================== Post switch plan ==================== {offset c5: // c1 _c1::I64 = I64[(old + 32)]; // CmmAssign _c2::I64 = I64[(old + 24)]; // CmmAssign _c3::P64 = P64[(old + 16)]; // CmmAssign _c4::P64 = R1; // CmmAssign //tick src<Small.cmm:(8,1)-(11,1)> unwind Sp = Just Sp + (8 + 16); // CmmUnwind R1 = _c4::P64; // CmmAssign call (P64[(old + 8)])(R1) args: 8, res: 0, upd: 8; // CmmCall } ==================== Layout Stack ==================== {offset c5: // c1 unwind Sp = Just Sp + 24; // CmmUnwind _c1::I64 = I64[Sp]; // CmmAssign _c2::I64 = I64[Sp + 8]; // CmmAssign _c3::P64 = P64[Sp + 16]; // CmmAssign _c4::P64 = R1; // CmmAssign //tick src<Small.cmm:(8,1)-(11,1)> unwind Sp = Just Sp + (8 + 16); // CmmUnwind R1 = _c4::P64; // CmmAssign unwind Sp = Just Sp + 0; // CmmUnwind Sp = Sp + 24; // CmmAssign call (P64[Sp])(R1) args: 8, res: 0, upd: 8; // CmmCall } ==================== Sink assignments ==================== {offset c5: // c1 unwind Sp = Just Sp + 24; // CmmUnwind //tick src<Small.cmm:(8,1)-(11,1)> unwind Sp = Just Sp + 24; // CmmUnwind R1 = R1; // CmmAssign unwind Sp = Just Sp; // CmmUnwind Sp = Sp + 24; // CmmAssign call (P64[Sp])(R1) args: 8, res: 0, upd: 8; // CmmCall } ==================== Post common block elimination 2 ==================== {offset c5: // c1 unwind Sp = Just Sp + 24; // CmmUnwind //tick src<Small.cmm:(8,1)-(11,1)> unwind Sp = Just Sp + 24; // CmmUnwind R1 = R1; // CmmAssign unwind Sp = Just Sp; // CmmUnwind Sp = Sp + 24; // CmmAssign call (P64[Sp])(R1) args: 8, res: 0, upd: 8; // CmmCall } ==================== CAFEnv ==================== [(c5, {})] ==================== after setInfoTableStackMap ==================== stg_catch_frame_ret() // [R1] { info_tbl: [(c5, label: stg_catch_frame_info rep:tag:34 StackRep [True, False])] stack_info: arg_space: 32 updfr_space: Just 8 } {offset c5: // c1 unwind Sp = Just Sp + 24; // CmmUnwind //tick src<Small.cmm:(8,1)-(11,1)> unwind Sp = Just Sp + 24; // CmmUnwind R1 = R1; // CmmAssign unwind Sp = Just Sp; // CmmUnwind Sp = Sp + 24; // CmmAssign call (P64[Sp])(R1) args: 8, res: 0, upd: 8; // CmmCall } } ==================== Post control-flow optimisations ==================== stg_catch_frame_ret() // [R1] { info_tbl: [(c5, label: stg_catch_frame_info rep:tag:34 StackRep [True, False])] stack_info: arg_space: 32 updfr_space: Just 8 } {offset c5: // c1 unwind Sp = Just Sp + 24; // CmmUnwind //tick src<Small.cmm:(8,1)-(11,1)> unwind Sp = Just Sp + 24; // CmmUnwind R1 = R1; // CmmAssign unwind Sp = Just Sp; // CmmUnwind Sp = Sp + 24; // CmmAssign call (P64[Sp])(R1) args: 8, res: 0, upd: 8; // CmmCall } } ==================== Post CPS Cmm ==================== [stg_catch_frame_ret() // [R1] { info_tbl: [(c5, label: stg_catch_frame_info rep:tag:34 StackRep [True, False])] stack_info: arg_space: 32 updfr_space: Just 8 } {offset c5: // c1 unwind Sp = Just Sp + 24; // CmmUnwind //tick src<Small.cmm:(8,1)-(11,1)> unwind Sp = Just Sp + 24; // CmmUnwind R1 = R1; // CmmAssign unwind Sp = Just Sp; // CmmUnwind Sp = Sp + 24; // CmmAssign call (P64[Sp])(R1) args: 8, res: 0, upd: 8; // CmmCall } }] ==================== Optimised Cmm ==================== stg_catch_frame_ret() // [R1] { [(c5, stg_catch_frame_info: const 66; const 34;)] } {offset c5: // c1 unwind Sp = Just Sp + 24; // CmmUnwind //tick src<Small.cmm:(8,1)-(11,1)> unwind Sp = Just Sp + 24; // CmmUnwind // nop unwind Sp = Just Sp; // CmmUnwind Sp = Sp + 24; // CmmAssign call (P64[Sp])(R1) args: 8, res: 0, upd: 8; // CmmCall } } }}} The first place where things go obviously wrong is `Layout Stack`. Let's focus on this fragment: {{{ unwind Sp = Just Sp + (8 + 16); // CmmUnwind R1 = _c4::P64; // CmmAssign unwind Sp = Just Sp + 0; // CmmUnwind Sp = Sp + 24; // CmmAssign }}} It looks like we swapped the unwind with the `Sp` update. Here's what we'd want: {{{ unwind Sp = Just Sp + (8 + 16); // CmmUnwind R1 = _c4::P64; // CmmAssign Sp = Sp + 24; // CmmAssign unwind Sp = Just Sp + 0; // CmmUnwind }}} It turns out that the place that's responsible for this is `manifestSp` in `CmmLayoutStack`. After a small change: {{{ diff --git a/compiler/cmm/CmmLayoutStack.hs b/compiler/cmm/CmmLayoutStack.hs index d2525d1..1e8c7c4 100644 --- a/compiler/cmm/CmmLayoutStack.hs +++ b/compiler/cmm/CmmLayoutStack.hs @@ -860,7 +860,7 @@ manifestSp dflags stackmaps stack0 sp0 sp_high = block where sp_unwind = CmmRegOff spReg (sp0 - wORD_SIZE dflags) - -- Add unwind pseudo-instruction right before the Sp adjustment + -- Add unwind pseudo-instruction right after the Sp adjustment -- if there is one. add_adj_unwind block | debugLevel dflags > 0 @@ -870,8 +870,9 @@ manifestSp dflags stackmaps stack0 sp0 sp_high = block where sp_unwind = CmmRegOff spReg (sp0 - wORD_SIZE dflags - sp_off) - final_middle = maybeAddSpAdj dflags sp_off - . add_adj_unwind + final_middle = + add_adj_unwind + . maybeAddSpAdj dflags sp_off . add_initial_unwind . blockFromList . map adj_pre_sp }}} We get: {{{ 0000000000000010 <stg_catch_frame_info>: 10: 48 83 c5 18 add $0x18,%rbp 14: ff 65 00 jmpq *0x0(%rbp) }}} {{{ Contents of the .debug_frame section: 00000000 0000000000000014 ffffffff CIE "" cf=1 df=-8 ra=16 LOC CFA rbp rsp ra 0000000000000000 rbp+0 v+0 s c+0 00000018 000000000000002c 00000000 FDE cie=00000000 pc=000000000000000f..0000000000000017 LOC CFA rbp rsp ra 000000000000000f rbp+0 v+0 s c+0 000000000000000f rbp+24 v+0 s c+0 0000000000000014 rbp+0 v+0 s c+0 }}} As a side note, the manually inserted unwind info in `stg_catch_frame` appears to be redundant. Layout Stack appears to be doing the same thing. 5279b08ba3f01e4b7e28d12b2751413d789d9fbe is what introduced this behavior, so I'd like Ben to check if I didn't go off the rails somewhere. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14999#comment:2> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14999: unwinding info for stg_catch_frame is wrong -------------------------------------+------------------------------------- Reporter: niteria | Owner: niteria Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: Resolution: | Keywords: Operating System: Linux | Architecture: x86_64 Type of failure: Debugging | (amd64) information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by bgamari):
I couldn't find a relevant note, so I resorted to experimentation. It appears that the unwind annotation reflects the state after A, but before B.
That is correct. Indeed it looks like my patch is wrong. I don't know what I was thinking when I wrote it. Good catch! -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14999#comment:3> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14999: unwinding info for stg_catch_frame is wrong -------------------------------------+------------------------------------- Reporter: niteria | Owner: niteria Type: bug | Status: patch Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: Resolution: | Keywords: Operating System: Linux | Architecture: x86_64 Type of failure: Debugging | (amd64) information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): phab:D4606 Wiki Page: | -------------------------------------+------------------------------------- Changes (by niteria): * status: new => patch * differential: => phab:D4606 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14999#comment:4> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14999: unwinding info for stg_catch_frame is wrong -------------------------------------+------------------------------------- Reporter: niteria | Owner: niteria Type: bug | Status: patch Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: Resolution: | Keywords: Operating System: Linux | Architecture: x86_64 Type of failure: Debugging | (amd64) information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): phab:D4606 Wiki Page: | -------------------------------------+------------------------------------- Comment (by Ben Gamari <ben@…>): In [changeset:"6132d7c5e6404936ef281a6f3be333fea780906e/ghc" 6132d7c5/ghc]: {{{ #!CommitTicketReference repository="ghc" revision="6132d7c5e6404936ef281a6f3be333fea780906e" Correctly add unwinding info in manifestSp and makeFixupBlocks In `manifestSp` the unwind info was before the relevant instruction, not after. I added some notes to establish semantics. Also removes redundant annotation in stg_catch_frame. For `makeFixupBlocks` it looks like we were off by `wORD_SIZE dflags`. I'm not sure why, but it lines up with `manifestSp`. In fact it lines up so well so that I can consolidate the Sp unwind logic in `maybeAddUnwind`. I detected the problems with `makeFixupBlocks` by running T14779b after patching D4559. Test Plan: added a new test Reviewers: bgamari, scpmw, simonmar, erikd Reviewed By: bgamari Subscribers: thomie, carter GHC Trac Issues: #14999 Differential Revision: https://phabricator.haskell.org/D4606 }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14999#comment:5> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14999: unwinding info for stg_catch_frame is wrong -------------------------------------+------------------------------------- Reporter: niteria | Owner: niteria Type: bug | Status: closed Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.4.2 Resolution: fixed | Keywords: Operating System: Linux | Architecture: x86_64 Type of failure: Debugging | (amd64) information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): phab:D4606 Wiki Page: | -------------------------------------+------------------------------------- Changes (by bgamari): * status: patch => closed * version: => 8.4.2 * resolution: => fixed -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14999#comment:6> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14999: unwinding info for stg_catch_frame is wrong -------------------------------------+------------------------------------- Reporter: niteria | Owner: niteria Type: bug | Status: closed Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.4.2 Resolution: fixed | Keywords: Operating System: Linux | Architecture: x86_64 Type of failure: Debugging | (amd64) information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): phab:D4606 Wiki Page: | -------------------------------------+------------------------------------- Comment (by bgamari): I've opened #15117 to track the addition of DWARF linters. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14999#comment:7> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14999: unwinding info for stg_catch_frame is wrong -------------------------------------+------------------------------------- Reporter: niteria | Owner: niteria Type: bug | Status: closed Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.4.2 Resolution: fixed | Keywords: DWARF Operating System: Linux | Architecture: x86_64 Type of failure: Debugging | (amd64) information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): phab:D4606 Wiki Page: | -------------------------------------+------------------------------------- Changes (by bgamari): * keywords: => DWARF -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14999#comment:8> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
participants (1)
-
GHC