> On Oct 8, 2020, at 10:29 PM, Jason Molenda <jmole...@apple.com> wrote: > > > >> On Oct 8, 2020, at 10:06 PM, Jason Molenda <jmole...@apple.com> wrote: >> >> >> >>> 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. > > > > just to be clear, though, my initial reaction to this bug is "we should > always push a row at offset 0." I don't want to sound dumb or anything, but > I don't understand how unwinding would work if we didn't have a Row at offset > 0. You step into the function, you're at the first instruction, you want to > find the caller stack frame, and without knowing the rule for establishing > the CFA and finding the saved pc, I don't know how you get that. And the > only way to get the CFA / saved pc rule is to get the Row. Do we really not > have a Row at offset 0 when an UnwindPlan is created from CFI? I might be > forgetting some trick of UnwindPlans, but I don't see how it works.
What you are saying makes sense, but that isn't how it is encoded. A quick example: 00000000 00000010 ffffffff CIE Version: 4 Augmentation: "" Address size: 4 Segment desc size: 0 Code alignment factor: 1 Data alignment factor: -4 Return address column: 14 DW_CFA_def_cfa: reg13 +0 DW_CFA_nop: DW_CFA_nop: 00000014 00000024 00000000 FDE cie=00000000 pc=0001bb2c...0001bc90 DW_CFA_advance_loc: 4 DW_CFA_def_cfa_offset: +32 DW_CFA_offset: reg14 -4 DW_CFA_offset: reg10 -8 DW_CFA_offset: reg9 -12 DW_CFA_offset: reg8 -16 DW_CFA_offset: reg7 -20 DW_CFA_offset: reg6 -24 DW_CFA_offset: reg5 -28 DW_CFA_offset: reg4 -32 DW_CFA_advance_loc: 2 DW_CFA_def_cfa_offset: +112 DW_CFA_nop: DW_CFA_nop: DW_CFA_advance_loc is what pushes a row. As you can see in the FDE, it is the first thing it does. If we were to always push a row after parsing the CIE instructions, then we would end up with two rows at the address for the FDE. The reason we need the FDE is to fill in a valid address for the CIE instructions into the row _before_ pushing it. So the CIE instructions don't make any sense without the FDE making it make sense. So as I originally stated: it probably won't affect us since this almost always happens, but if I have learned anything from years of parsing info compilers emit: if they can do it, they will. So the FDE _could_ modify more register values on top of the CIE instructions before pushing a row... > > >> >> >>> >>> >>>> >>>> >>>> >>>>> 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
