Page MenuHomePhabricator

UnwindPlan::Row refactor -- add support for CFA set by a DWARF expression
ClosedPublic

Authored by labath on Feb 19 2015, 6:34 AM.

Details

Summary

This change refactors UnwindPlan::Row to be able to store the fact that the CFA is value is set
by evaluating a dwarf expression (DW_CFA_def_cfa_expression). This is achieved by creating a new
class CFAValue and moving all CFA setting/getting code there. Note that code using the new
CFAValue::isDWARFExpression is not yet present and will be added in a follow-up patch. Therefore,
this patch should not change the functionality in any way.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 20290.Feb 19 2015, 6:34 AM
labath retitled this revision from to UnwindPlan::Row refactor -- add support for CFA set by a DWARF expression.
labath updated this object.
labath edited the test plan for this revision. (Show Details)
labath added reviewers: jasonmolenda, clayborg.
labath added a subscriber: Unknown Object (MLST).
clayborg edited edge metadata.Feb 19 2015, 11:32 AM

I will defer to Jason on this.

jasonmolenda accepted this revision.Feb 19 2015, 7:12 PM
jasonmolenda edited edge metadata.

This looks fine to me. I don't have a problem with the CFAValue::IncOffset() method but it would be more in keeping with the typical lldb style if this class still had a SetOffset() method. It's more a consistency thing than one being better than the other - if I see GetOffset(), I'm going to assume there is also a SetOffset(). I'm fine with keeping IncOffset() - in some code sequences it will be more natural than SetOffset. And you've made the code a little less clear in places where that is the natural way to express what is being done. Like in UnwindAssembly-x86.cpp we went from

row->SetCFAOffset (current_sp_bytes_offset_from_cfa);

to

row->GetCFAValue().SetIsRegisterPlusOffset(m_lldb_sp_regnum,
        current_sp_bytes_offset_from_cfa);

which obscures what is really being done IMO. I'd prefer the places where you changed SetOffset to SetIsRegisterPlusOffset remain SetOffset.

include/lldb/Symbol/UnwindPlan.h
244 ↗(On Diff #20290)

Very minor comment but I don't know if RestoreType is the best nomenclature in this case. I don't have a better suggestion right now. In an unwind scenario, I think of "restore" as meaning "retrieving the value of a register that was saved by a callee". Maybe I'll think up something better later. CFACalculateType would be closer, but that sounds horrible.

316 ↗(On Diff #20290)

I was worried about the ownership of the opcode bytes in SetIsDWARFExpression but I guess the assumption is that this will come from the DWARF for the Module and since the UnwindPlans are also owned by the Module, they'll have the same lifetime. This should be fine.

This revision is now accepted and ready to land.Feb 19 2015, 7:13 PM

The reason I avoided the SetOffset() function was that carried the assumption that the only way to set the CFA value was reg+offset, which I think was true at some point (and I think some of the code still assumes that, like the Row::Dump part which I removed), but it wasn't true even before this patch --- it could be set as [reg], in which case the offset was ignored. If we allow setting the offset separately, then the question is what should be done when the current value type is not "isRegisterPlusOffset":

  • set the type to "isRegisterPlusOffset", but leave the register undefined?
  • set the type to "isRegisterPlusOffset", and set the register to some default value?
  • ignore the SetOffset() command?
  • return an error?

IncOffset is slightly better I think because it is more obvious that if you want to increment something, it has to be valid in the first place. I agree that the SetIsRegisterPlusOffset makes things a bit cumbersome, but actually most of those places could be naturally changed to use IncOffset instead. And we have a precedent for now having a SetOffset function: Row::RegisterLocation provides a GetOffset method, but is only settable via SetAtCFAPlusOffset and the like.

What do you think?

include/lldb/Symbol/UnwindPlan.h
244 ↗(On Diff #20290)

How about just ValueType, given that the class is called CFAValue , and the getter is GetValueType()?

316 ↗(On Diff #20290)

I modeled this class after Row::RegisterLocation, where they do the same thing, basically. So I think it should be ok...

labath updated this revision to Diff 20386.Feb 20 2015, 4:01 AM
labath edited edge metadata.

Renamed CFAValue::RestoreType -> ValueType

labath updated this revision to Diff 20411.Feb 20 2015, 8:53 AM

Forgot to add break statements to switch. I blame Go.

Yeah, I can see what you're talking about with CFAValue::SetOffset() assuming the type of the CFA value but for instance in UnwindAssembly-x86, when it knows that the CFA is set in terms of rsp and the stack pointer value changes, it already knows that the CFA value type is IsRegisterPlusOffset - it is safe to simply call SetOffset() here.

In the case of UnwindAssembly-x86, it needs to track the stack pointer as it is modified throughout the lifetime of the function. When a register is saved on the stack, it records where that register was saved. It would be possible for it to track all of this in the CFAValue class - calling GetOffset() instead of using a local, calling IncrementOffset() whenever the stack value is moved down. There'd need to be a matching DecrementOffset() to match, of course. But I think it's more natural (to me, anyway) to have a local var tracking these changes and calling SetOffset() whenever needed.

I don't think the CFAValue class needs to dictate how the users behave - I'm fine with having methods to adjust the value - but I'd rather classes like UnwindAssembly-x86 continue to call SetOffset(). We could make it return a bool if the operation was invalid, but IncrementOffset() would have the same issues for that matter. I don't think it's worth worrying about.

Thanks for making the adjustment to the patch.

I have added a SetOffset function and refactored UnwindAssembly-x86 to use it. Submitting.

This revision was automatically updated to reflect the committed changes.