> On Oct 8, 2020, at 9:17 PM, Greg Clayton <clayb...@gmail.com> wrote: > > > >> On Oct 8, 2020, at 8:55 PM, Jason Molenda <jmole...@apple.com> wrote: >> >> Good bug find! >> >> It seems to me that DWARFCallFrameInfo should push the initial CIE register >> setup instructions as being the state at offset 0 in the function (in fact >> I'd say it's a bug if it's not). If that were done, then getting >> RowAtIndex(0) should be synonymous with get-the-CIE-register-unwind-rules, >> and this code would be correct. >> >> Looking at DWARFCallFrameInfo::FDEToUnwindPlan, we do >> >> unwind_plan.SetPlanValidAddressRange(range); >> UnwindPlan::Row *cie_initial_row = new UnwindPlan::Row; >> *cie_initial_row = cie->initial_row; >> UnwindPlan::RowSP row(cie_initial_row); >> >> unwind_plan.SetRegisterKind(GetRegisterKind()); >> unwind_plan.SetReturnAddressRegister(cie->return_addr_reg_num); >> >> cie->initial_row is set by DWARFCallFrameInfo::HandleCommonDwarfOpcode. >> >> I think the bug here is DWARFCallFrameInfo::FDEToUnwindPlan not pushing that >> initial row at offset 0, isn't it? We don't really use DWARF CFI on darwin >> any more so I don't have a lot of real world experience here. > > The only opcodes that push a row are DW_CFA_advance_locXXX and > DW_CFA_set_loc, so I don't think that is the right fix. I think we need to > pass a copy of just the registers from the "cie->initial_row" object around > to the opcode parsing code for restoration purposes.
I think everything I'm saying here is besides the point, though. Unless we ALWAYS push the initial unwind state (from the CIE) to an UnwindPlan, the DW_CFA_restore is not going to work. If an unwind rule for r12 says "DW_CFA_restore" and at offset 0 in the function, the unwind rule for r12 was "same" (i.e. no rule), but we return the RowAtIndex(0) and the first instruction, I don't know, spills it or something, then the DW_CFA_restore would set the r12 rule to "r12 was spilled" instead of "r12 is same". So the only way DW_CFA_restore would behave correctly, with this, is if we always push a Row at offset 0 with the rules from the CIE, or with no rules at all, just the initial unwind state showing how the CFA is set and no register rules. > > >> >> >> >>> On Oct 8, 2020, at 4:01 PM, Greg Clayton <clayb...@gmail.com> wrote: >>> >>> Hello LLDB devs, >>> >>> This is a deep dive into an issue I found in the LLDB handling of DWARF >>> call frame information, so no need to read further if this doesn't interest >>> you! >>> >>> I am in the process of adding some support to LLVM for parsing the opcode >>> state machines for CIE and FDE objects that produces unwind rows. While >>> making unit tests to test DW_CFA_restore and DW_CFA_restore_extended >>> opcodes, I read the DWARF spec that states: >>> >>> "The DW_CFA_restore instruction takes a single operand (encoded with the >>> opcode) that represents a register number. The required action is to change >>> the rule for the indicated register to the rule assigned it by the >>> initial_instructions in the CIE." >>> >>> Looking at the LLDB code in DWARFCallFrameInfo.cpp I see code that is >>> simplified to: >>> >>> case DW_CFA_restore: >>> if (unwind_plan.IsValidRowIndex(0) && >>> unwind_plan.GetRowAtIndex(0)->GetRegisterInfo(reg_num, reg_location)) >>> row->SetRegisterInfo(reg_num, reg_location); >>> break; >>> >>> >>> The issue is, the CIE contains initial instructions, but it doesn't push a >>> row after doing these instructions, the FDE will push a row when it emits a >>> DW_CFA_advance_loc, DW_CFA_advance_loc1, DW_CFA_advance_loc2, >>> DW_CFA_advance_loc4 or DW_CFA_set_loc opcode. So the DWARF spec says we >>> should restore the register rule to be what it was in the CIE's initial >>> instructions, but we are restoring it to the first row that was parsed. >>> This will mostly not get us into trouble because .debug_frame and .eh_frame >>> usually have a DW_CFA_advance_locXXX or DW_CFA_set_loc opcode as the first >>> opcode, but it isn't a requirement and a FDE could modify a register value >>> prior to pushing the first row at index zero. So we might be restoring the >>> register incorrectly in some cases according to the spec. Also, what if >>> there was no value specified in the CIE's initial instructions for a >>> register? Should we remove the register value to match the state of the >>> CIE's initial instructions if there is no rule for the register? We are >>> currently leaving this register as having the same value if there is no >>> value for the register in the first row. >>> >>> Let me know what you think. >>> >>> Greg Clayton >>> >> > _______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
