[Lldb-commits] [PATCH] D12556: Introduce new address class eAddressClassDataIntermixedCode

[Tamas Berghammer via lldb-commits]

tberghammer added a comment.

In http://reviews.llvm.org/D12556#238457, @clayborg wrote:

> Changing all $d symbols to always say they are eAddressClassDataIntermixedCode is wrong because the symbols in the .data section now would be marked as eAddressClassDataIntermixedCode.


We only use the m_address_class_map for the code section (when the address class of the section is eAddressClassCode) so we won't mark the symbols in the .data section as eAddressClassDataIntermixedCode.

> To clarify a few things, lets say we have the following code:

> 

> 0x1000: bx <addr> Non-tail call in a no return function

>  0x1004: [data-pool] Marked with $d mapping symbol

> 

> Should just claim that 0x1000 is eAddressClassCode and that 0x1004 is eAddressClassData. We don't need a new eAddressClassDataIntermixedCode for this, it should just say eAddressClassData for 0x1004.


This is the current implementation (before this change).

> For return addresses we that are on the stack on in the LR, we should actually be sanitizing them before we start doing lookups. So the return address would be 0x1005 in this case you are talking about right? Maybe we always get the address class of the return address and check if it is eAddressClassData. If it is, we know something is wrong since the return address can't go to data and we work around this by recognizing that fact.


It is incorrect in some sense if we see that the return address points to a data section but in the described case this is the best what LLDB can say (and it is true that if the function return, then it will try to execute some code from the data segment), and from this address the unwinding code can continue without any issue (assuming lr is saved somewhere).

> I would rather not just say that all data is eAddressClassDataIntermixedCode for ARM and Thumb, that seems like the wrong fix.


What is your opinion about changing GetOpcodeLoadAddress and GetCallableLoadAddress to always mask out the LSB on arm/thumb and never return LLDB_INVALID_ADDRESS? It would fix the issue and will be (mostly) consistent with the other architectures where we don't do any checking, but it will drop a slight safety net for the case when something went seriously wrong and we really ended up in a data section (we don't have this check for other architectures).


http://reviews.llvm.org/D12556