Opened 4 years ago

Closed 3 years ago

Last modified 8 months ago

#11353 closed bug (fixed)

DWARF call frame information incorrect in the presence of unsafe foreign calls

Reported by: bgamari Owned by: bgamari
Priority: high Milestone: 8.2.1
Component: Compiler (NCG) Version: 7.10.3
Keywords: DWARF Cc: scpmw, simonmar
Operating System: Unknown/Multiple Architecture: x86_64 (amd64)
Type of failure: Debugging information is incorrect Test Case:
Blocked By: Blocking:
Related Tickets: #11338, #11337 Differential Rev(s):
Wiki Page:

Description

Unsafe foreign calls result in adjustments to $rsp made in the NCG to comply with calling convention alignment requirements (see X86.CodeGen.genCCall64'). Unfortunately, this happens after we've generated unwind information. This results in incorrect unwinding information for $rsp when inside of a foreign call. The results can be quite catastrophic (e.g. segmentation faults while unwinding).

Unfortunately it's really not clear what can be done about this given that these adjustments aren't present in the Cmm representation that we use to produce frame information.

Change History (13)

comment:1 Changed 4 years ago by bgamari

Perhaps we can feed the DebugBlocks as state into the NCG. The NCG can then make the appropriate updates before finally producing the final debug information?

Ugh.

comment:2 Changed 4 years ago by bgamari

In fact, it looks like the NCG already has read-only access to the DebugBlocks. Perhaps this idea isn't so crazy. The tricky part is ensuring that the debug entries remain properly sorted. Perhaps you could say something like "insert this UnwindTable after label c2TN"?

comment:3 Changed 4 years ago by bgamari

Another option along these lines would be to abandon tracking label ordering at all until code generation. Then the code generator could modify the DebugBlock to its hearts content, so long as in the end it returned the unwinding tables in the proper order.

For those playing along at home, the issue here is that DWARF requires that frame unwinding tables (FDEs) must be written in order of increasing address.

comment:4 Changed 4 years ago by simonmar

The NCG tracks the current state of %rsp during code generation (see getDeltaNat) and emits hints into the generated code with the pseudo-instruction DELTA. It seems like we should be able to generate correct directives for the assembler from the DELTA instructions, no?

comment:5 Changed 4 years ago by bgamari

comment:6 Changed 4 years ago by bgamari

The problem is that you need both details from the code generator (to account for the unwind adjustments as described in this ticket) as well as the procedure's control flow graph (in order to push unwind information down to successor blocks).

My current plan is,

  • Introduce a UNWIND pseudo-instruction
  • CmmUnwind nodes will be translated by the UNWIND instructions
  • Add a lazy field to NEWBLOCK which will capture the successors of the block
  • Introduce a pass after code generation which traverses the [Instruction] of each block and,
    • extract the UNWIND and DELTA instructions from a block's [Instruction] and use them to build a LabelMap (UnwindTable, [SuccessorBlockId]) (the SuccessorBlockId being taken from the successors field of the block's NEWBLOCK instruction)
    • accumulate a [BlockId] to preserve the order in which we must emit unwinding tables (since they must be written in order of increasing address)
  • Introduce another pass of shape LabelMap (UnwindTable, [SuccessorBlockId]) -> LabelMap UnwindTable which pushes blocks' unwind tables "down" to their successors
  • Pass the resulting UnwindTables to the existing unwinding table generation code

comment:7 Changed 4 years ago by scpmw

Not sure why you are worrying about the order - that gets derived from the NCG output using cmmDebugLink. This is precisely so NCG is free to do whatever it wants with blocks, even reorder or optimise them out.

My first impulse here would be that you will need some sort of UNWIND anyway to have NCG create labels. So maybe have it carry its unwind rules, that would allow NCG to update it. In either case, you would feed that information back roughly the same way that currently ngs_labels gets threaded back to debug information generation. As UNWIND would carry unique labels, this could just be replacing the current [Label] block list with an an ordered [(Label, UnwindTable)] debug label to unwind table map.

Not sure - this is exactly the kind of complexity that I tried my best to avoid, so I haven't put a lot of thought into it.

Last edited 4 years ago by scpmw (previous) (diff)

comment:8 Changed 3 years ago by bgamari

Component: Compiler (CodeGen)Compiler (NCG)

comment:9 Changed 3 years ago by bgamari

Owner: set to bgamari

comment:10 Changed 3 years ago by bgamari

Priority: normalhigh

The patches for this are essentially done. I just need to clean them up and merge them.

comment:11 Changed 3 years ago by Ben Gamari <ben@…>

In 3eb737e/ghc:

Generalize CmmUnwind and pass unwind information through NCG

As discussed in D1532, Trac Trac #11337, and Trac Trac #11338, the stack
unwinding information produced by GHC is currently quite approximate.
Essentially we assume that register values do not change at all within a
basic block. While this is somewhat true in normal Haskell code, blocks
containing foreign calls often break this assumption. This results in
unreliable call stacks, especially in the code containing foreign calls.
This is worse than it sounds as unreliable unwinding information can at
times result in segmentation faults.

This patch set attempts to improve this situation by tracking unwinding
information with finer granularity. By dispensing with the assumption of
one unwinding table per block, we allow the compiler to accurately
represent the areas surrounding foreign calls.

Towards this end we generalize the representation of unwind information
in the backend in three ways,

 * Multiple CmmUnwind nodes can occur per block

 * CmmUnwind nodes can now carry unwind information for multiple
   registers (while not strictly necessary; this makes emitting
   unwinding information a bit more convenient in the compiler)

 * The NCG backend is given an opportunity to modify the unwinding
   records since it may need to make adjustments due to, for instance,
   native calling convention requirements for foreign calls (see
   #11353).

This sets the stage for resolving #11337 and #11338.

Test Plan: Validate

Reviewers: scpmw, simonmar, austin, erikd

Subscribers: qnikst, thomie

Differential Revision: https://phabricator.haskell.org/D2741

comment:12 Changed 3 years ago by bgamari

Resolution: fixed
Status: newclosed

comment:13 Changed 8 months ago by bgamari

Keywords: DWARF added
Note: See TracTickets for help on using tickets.