Remove implicit write of PC in case of a write to RA in UnwindAssemblyInstEmulation

[tberghammer]

Summary

Remove implicit write of PC in case of a write to RA in UnwindAssemblyInstEmulation

Previously the calculated location of PC is changed in case of a write
to RA inside UnwindAssemblyInstEmulation::WriteRegister. It caused an
issue on arm because RA can be used as a general purpose register so a
register load to RA don't mean the function will return to the given
address. The value (location) of PC should be updated explicitly by the
instruction emulator when an instruction writes to it.

[bhushan]

Looks good

[clayborg]

This one makes me nervous. Jason Molenda should be added as a reviewer since he does all things stack crawl related in LLDB.

So the client of this when building the unwind info from assembly, should note at which address the RA is stored to the stack and keep that saved somewhere so that when one of these loads happens, it can correctly set the rule for unwinding the PC to be grabbing the value from RA. You are effectively stopping this from happening with this modification which will mean we will probably backtrace incorrectly out of a frame after the frame has been removed and the RA has been restored to the RA register.

[clayborg]

Marking as request changes due to previous concerns.

[tberghammer]

This part of code was added to LLDB about 2.5 months ago (http://reviews.llvm.org/D7696) to handle an issue supposed to be arising on mips. I had a discussion about it with the original author (@bhushan) and we agreed that the issue this code tries to address is very low priority (isn't present in any code we seen so far and it won't make too much sense for the compiler to generate code what use it) and it have negative effect in the case when RA register used for general calculations.

So the client of this when building the unwind info from assembly, should note at which address the RA is stored to the stack and keep that saved somewhere so that when one of these loads happens, it can correctly set the rule for unwinding the PC to be grabbing the value from RA. You are effectively stopping this from happening with this modification which will mean we will probably backtrace incorrectly out of a frame after the frame has been removed and the RA has been restored to the RA register.

I am not stopping this from happening because when RA is pushed onto the stack we note that location to use it for unwinding PC. What I am stopping is when RA is loaded from a random memory location then setting the previous PC to the same value we loaded for RA. So after my change we still have a location for the PC and the only case I can imagine when this will cause some issue is the following:

0x00: push RA ; (RA=SP+4)
0x04: ldr RA, [SP+4]
0x08: str R1, [SP+4]
0x0c: pop R1 
0x10: mov PC, RA

In this case at 0x0c the emulator will still believe that PC is saved at [SP+4] what is not true but I think this is a highly unlikely situation (I never seen code like this). On the other hand the current implementation fails in the following case:

0x00: push RA
0x04: ldr RA, [PC-0x8] ; Load value to RA from memory for general calculations
0x08: XXX ; Do calculation where an extra register (RA) is useful
0x0c: pop PC

With the current implementation at 0x08 in the previous the emulator will believe that PC is same as RA what have a value from memory. This situation isn't happen very often but I seen several case in libc.so where happens.

PS.: I added Jason as a reviewer also

[tberghammer]

Abandon because of an upcoming patch fixing this issue and several other issue also

source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp

//            }
         case EmulateInstruction::eContextRegisterLoad:
             {
                 const uint32_t unwind_reg_kind = m_unwind_plan_ptr->GetRegisterKind();
                 const uint32_t reg_num = reg_info->kinds[unwind_reg_kind];
                 if (reg_num != LLDB_INVALID_REGNUM)
                 {
                     m_curr_row->SetRegisterLocationToRegister (reg_num, reg_num, must_replace);
                     m_curr_row_modified = true;
-
-                    if (reg_info->kinds[eRegisterKindGeneric] == LLDB_REGNUM_GENERIC_RA)
-                    {
-                        // This load was restoring the return address register,
-                        // so this is also how we will unwind the PC...
-                        RegisterInfo pc_reg_info;
-                        if (instruction->GetRegisterInfo (eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC, pc_reg_info))
-                        {
-                            uint32_t pc_reg_num = pc_reg_info.kinds[unwind_reg_kind];
-                            if (pc_reg_num != LLDB_INVALID_REGNUM)
-                            {
-                                m_curr_row->SetRegisterLocationToRegister (pc_reg_num, reg_num, must_replace);
-                                m_curr_row_modified = true;
-                            }
-                        }
-                    }
                 }
             }
             break;
 
         case EmulateInstruction::eContextRelativeBranchImmediate:
             {
                 
                 {