> 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. > > > >> 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
