GetEHFrameAugmentedUnwindPlan duplicated the work of GetEHFrameUnwindPlan in getting the original
plan from DWARF CFI. This changes the function to call GetEHFrameUnwindPlan instead of doing all
the work itself.
Details
Diff Detail
Event Timeline
My only concern about the patch is the UnwindPlan copy constructor which is the default cc that the compiler generates. I think that's why I duplicated the code originally -- I wasn't sure if it was safe.
The UnwindPlan has a vector<> of Row shared pointers (std::Vector<RowSP>) - won't both copies of the UnwindPlan be pointing to the same vector of rows, and then when we add the epilogue Rows to the m_unwind_plan_eh_frame_augmented_sp unwind plan, m_unwind_plan_eh_frame_sp will have its rows updated as well?
The right answer here is probably to make the copy constructor do a deep copy of the object. Or am I misremembering how this works?
You are correct. I have seen copies being made this way, but I guess that was of individual rows and not whole plans... I'll fix this and get back.
Add deep copy UnwindPlan constructor.
So, the case you mentioned (adding Rows to the copied UnwindPlan) would actually work fine, as
the copies shared the individual rows, not their vector container. The problematic case would be
if someone went in and changed one of the rows. Then this change would unexpectedly propagate to
the other plan. However, I have checked the all the users of unwind plans and none of them does
this (and I don't see a reason why they should). So, another way of avoiding the deep copy would
be to make the UnwindPlan contain a std::vector<std::shared_ptr<const Row>>. Then the Rows could
no longer be modified after creation and you could share them among unwind plans freely.
What do you think?
This looks good to me, thanks for adding the deep copy constructor, this will make the class easier for people to use in the future without introducing bugs by accident.