Add new ABI callback to provide fallback unwind register locations

[uweigand]

Summary

If the UnwindPlan did not identify how to unwind the stack pointer
register, LLDB currently assumes it can determine to caller's SP
from the current frame's CFA. This is true on most platforms
where CFA is by definition equal to the incoming SP at function
entry.

However, on the s390x target, we instead define the CFA to equal
the incoming SP plus an offset of 160 bytes. This is because
our ABI defines that the caller has to provide a register save
area of size 160 bytes. This area is allocated by the caller,
but is considered part of the callee's stack frame, and therefore
the CFA is defined as pointing to the top of this area.

In order to make this work on s390x, this patch introduces a new
ABI callback GetFallbackRegisterLocation that provides platform-
specific fallback register locations for unwinding. The existing
code to handle SP unwinding as well as volatile registers is moved
into the default implementation of that ABI callback, to allow
targets where that implementation is incorrect to override it.

This patch in itself is a no-op for all existing platforms.
But it is a pre-requisite for adding s390x support.

[clayborg]

I am not sure why this offset of 160 isn't represented in the unwind info. I guess the OS unwinder that uses the EH frame info just knows that it must adjust the CFA? That seems like a hack. It seems like the unwind info should be fixed and the OS level unwinder should be fixed so that the info can be used correctly without every debugger or tool that has to parse this know about such a hack? Do you have control over the OS and are you able to fix this? That seems like the better fix.

[uweigand]

In D18977#397626, @clayborg wrote:

I am not sure why this offset of 160 isn't represented in the unwind info. I guess the OS unwinder that uses the EH frame info just knows that it must adjust the CFA? That seems like a hack. It seems like the unwind info should be fixed and the OS level unwinder should be fixed so that the info can be used correctly without every debugger or tool that has to parse this know about such a hack? Do you have control over the OS and are you able to fix this? That seems like the better fix.

The offset of 160 is represented in the unwind info when computing the CFA itself. This change is about unwinding the SP register when no explicit unwind info for SP is present. In this case, the LLDB unwinder assumes it should set SP to the CFA, which is where the problem comes in.

Now, on s390, most frames actually have explicit unwind instructions for SP, so this special case is not even triggered. Only for leaf functions without a stack frame can we ever see the scenario that there are no SP unwind instructions. In those cases, the correct result on s390 is to simply leave SP unchanged (since the function did not in fact allocate a frame).

However, due to the special case in the LLDB unwinder, SP is not left unchanged, but set to the CFA (i.e. SP + 160 in this case). This is wrong. There are two possible ways to fix this:

• Disable the special case of setting SP to CFA on s390. Instead, when there is no unwind info for SP, just leave it unchanged (just like any other call-saved register). This solution is implemented in the platform unwinder (libgcc) and also in GDB.
• Fix the special case of setting SP to CFA by actually taking the CFA bias into account. This is what this patch implements for LLDB.

I've implemented the second method for LLDB since it actually seemed a bit more general to me (describes more precisely what actually happens on the platform), and also it seemed to fit nicely into the ABI scheme.

If you prefer, I can implement the first method instead. However, since we cannot unconditionally disable the special case of setting SP to CFA (other platforms depend on that -- which is arguably the real hack to begin with), we would still need some ABI method to tell the unwinder whether or not the special case is needed.

There's no real way to "fix the unwind info", since this is not a new platform and binaries containing this unwind info have been in the field for more than 15 years. In any case, it is not clear that there is anything to "fix" -- a binary containing no unwind info for SP to indicate that SP does not change isn't really "wrong" IMO.

[clayborg]

In D18977#398157, @uweigand wrote:

In D18977#397626, @clayborg wrote:

I am not sure why this offset of 160 isn't represented in the unwind info. I guess the OS unwinder that uses the EH frame info just knows that it must adjust the CFA? That seems like a hack. It seems like the unwind info should be fixed and the OS level unwinder should be fixed so that the info can be used correctly without every debugger or tool that has to parse this know about such a hack? Do you have control over the OS and are you able to fix this? That seems like the better fix.

The offset of 160 is represented in the unwind info when computing the CFA itself. This change is about unwinding the SP register when no explicit unwind info for SP is present. In this case, the LLDB unwinder assumes it should set SP to the CFA, which is where the problem comes in.

This is usually represented by the CIE (Common Information Entry) which is pointed to from the FDE (Frame Description Entry). The CIE has the initial state that all FDEs can share. Seems like there should be an entry for the SP that says the rule to unwind it is "CFA+160"?

Now, on s390, most frames actually have explicit unwind instructions for SP, so this special case is not even triggered. Only for leaf functions without a stack frame can we ever see the scenario that there are no SP unwind instructions. In those cases, the correct result on s390 is to simply leave SP unchanged (since the function did not in fact allocate a frame).

However, due to the special case in the LLDB unwinder, SP is not left unchanged, but set to the CFA (i.e. SP + 160 in this case). This is wrong. There are two possible ways to fix this:

• Disable the special case of setting SP to CFA on s390. Instead, when there is no unwind info for SP, just leave it unchanged (just like any other call-saved register). This solution is implemented in the platform unwinder (libgcc) and also in GDB.
• Fix the special case of setting SP to CFA by actually taking the CFA bias into account. This is what this patch implements for LLDB.

There seems to be magic happening here. It seems like the CIE or FDE should describe how to unwind the SP when things are tricky. We do have the notion that registers that have no rule can have a default rule applied to them. Currently this is only used for callee saved registers and for any such registers they rule defaults to "the registers is in the register itself". For volatile registers, their default rule is "the register is not available". This is the part where I get fuzzy with respect to how the OS unwinder works: does it have this same notion of default rules for registers? If so, we should be implementing the same thing for LLDB's EH frame parser.

So a third solution is to allow the ABI plug-in to specify the default rules for registers when they aren't specified. I think the ABI plug-in is where we get the fact that a register is volatile or not...

I've implemented the second method for LLDB since it actually seemed a bit more general to me (describes more precisely what actually happens on the platform), and also it seemed to fit nicely into the ABI scheme.

If you prefer, I can implement the first method instead. However, since we cannot unconditionally disable the special case of setting SP to CFA (other platforms depend on that -- which is arguably the real hack to begin with), we would still need some ABI method to tell the unwinder whether or not the special case is needed.

There's no real way to "fix the unwind info", since this is not a new platform and binaries containing this unwind info have been in the field for more than 15 years. In any case, it is not clear that there is anything to "fix" -- a binary containing no unwind info for SP to indicate that SP does not change isn't really "wrong" IMO.

So I think implementing the default rule for unspecified registers more completely will help us solve this problem correctly. Let me know what you think?

[uweigand]

In D18977#398557, @clayborg wrote:

In D18977#398157, @uweigand wrote:

In D18977#397626, @clayborg wrote:

I am not sure why this offset of 160 isn't represented in the unwind info. I guess the OS unwinder that uses the EH frame info just knows that it must adjust the CFA? That seems like a hack. It seems like the unwind info should be fixed and the OS level unwinder should be fixed so that the info can be used correctly without every debugger or tool that has to parse this know about such a hack? Do you have control over the OS and are you able to fix this? That seems like the better fix.

The offset of 160 is represented in the unwind info when computing the CFA itself. This change is about unwinding the SP register when no explicit unwind info for SP is present. In this case, the LLDB unwinder assumes it should set SP to the CFA, which is where the problem comes in.

This is usually represented by the CIE (Common Information Entry) which is pointed to from the FDE (Frame Description Entry). The CIE has the initial state that all FDEs can share. Seems like there should be an entry for the SP that says the rule to unwind it is "CFA+160"?

Well, there could be, just like there could be a default "same register" rule for call-saved registers so we wouldn't have to apply ABI knowledge. However, none of that has been done historically on any DWARF platform I know of, mostly because this would duplicate information known via the ABI into every object file and thus waste space ... In any case, the current state is as it is, we have to handle object files that currently exist.

Now, on s390, most frames actually have explicit unwind instructions for SP, so this special case is not even triggered. Only for leaf functions without a stack frame can we ever see the scenario that there are no SP unwind instructions. In those cases, the correct result on s390 is to simply leave SP unchanged (since the function did not in fact allocate a frame).

However, due to the special case in the LLDB unwinder, SP is not left unchanged, but set to the CFA (i.e. SP + 160 in this case). This is wrong. There are two possible ways to fix this:

• Disable the special case of setting SP to CFA on s390. Instead, when there is no unwind info for SP, just leave it unchanged (just like any other call-saved register). This solution is implemented in the platform unwinder (libgcc) and also in GDB.
• Fix the special case of setting SP to CFA by actually taking the CFA bias into account. This is what this patch implements for LLDB.

There seems to be magic happening here. It seems like the CIE or FDE should describe how to unwind the SP when things are tricky. We do have the notion that registers that have no rule can have a default rule applied to them. Currently this is only used for callee saved registers and for any such registers they rule defaults to "the registers is in the register itself". For volatile registers, their default rule is "the register is not available". This is the part where I get fuzzy with respect to how the OS unwinder works: does it have this same notion of default rules for registers? If so, we should be implementing the same thing for LLDB's EH frame parser.

Actually, there are three different default rules: the ones for call-saved and call-clobbered registers you mention, and the special rule for the SP. The platform unwinder does not care about volatile registers (because when you dispatch an exception, the catch point by definition must not care about the contents of those), but it does implement the "same register" rule for call-saved registers, and it does implement the special rule for SP on the platforms that need it.

So a third solution is to allow the ABI plug-in to specify the default rules for registers when they aren't specified. I think the ABI plug-in is where we get the fact that a register is volatile or not...

Yes, that would be a solution, in fact this is the exact solution I implemented for GDB back in the days ...

Right now, if RegisterContextLLDB::SavedLocationForRegister finds no unwind info for a register, it implements (itself) the three default rules, in this order: if it is SP, use the CFA rule; otherwise, if the ABI says the register is volatile, use the "not available rule"; otherwise, say "register not found", which the caller interprets as "same as in the next frame".

One option to fix the s390x problem would be to instead of hard-coding the SP rule and the volatile check here, always call an ABI callback to define a "fallback register rule" by returning a UnwindPlan::Row::RegisterLocation. The ABI could return a "isCFAPlusOffset" fallback rule for the SP if appropriate, or it could return a "undefined" fallback rule for volatile registers.

Is this what you were thinking about? I can try to come up with a patch along those lines.

[clayborg]

Yes, that plan sounds great.

[uweigand]

OK, here's an implementation of the new approach. In addition to the changes discussed, I had to add handling of undefined register locations -- this might now theoretically also trigger when CFI contains an explicit DW_CFA_undefined, but it should actually do the right thing then as well.

Everything still works on SystemZ using this approach, and I've also tested on Intel with no regressions.

[jasonmolenda]

Thanks for doing this work Ulrich, the assumption that SP==CFA was mine, I should have put this in the ABI from the start.

I worry a little about the change to RegisterContextLLDB::SavedLocationForRegister() where you've moved the ABI is-volatile check into ABI::GetFallbackRegisterLocation. If this function can't find an unwind rule for a non-volatile register, it will return UnwindLLDB::RegisterSearchResult::eRegisterNotFound and UnwindLLDB will continue searching for a definition down the stack.

You're changing the call to ABI::RegisterIsVolatile to unwindplan_regloc.IsUndefined(), assuming that GetFallbackRegisterLocation set it to undefined. But could a register have an undefined state from the eh_frame rules? I'm not sure what that would indicate - but presumably this function doesn't use that register and lldb should continue its search.

I may not be correct about this point - but what do you think about this possibility?

[jasonmolenda]

Just to be clear, my concern is we get an register location which is "undefined" (I've never seen it, I don't know what it means) and now SavedLocationForRegister() will say "this register is volatile, not looking any further" when it used to say "register not found in this stack frame" and the unwinder would keep looking down the stack for a save location.

[tberghammer]

My understanding of the eh_frame is that "undefined" means the value of the register is not recoverable in the current frame (default for volatile registers) while the meaning of "same" is that this frame haven't messed with the register so we should continue looking for its value down the stack. Based on this I think this change is doing the right thing and our previous implementation is the one what is incorrect in some case.

The relevant information from the dwarf spec (from section 6.4.1):

undefined: A register that has this rule has no recoverable value in the previous frame. (By convention, it is not preserved by a callee.)
same value: This register has not been modified from the previous frame. (By convention, it is preserved by the callee, but the callee has not modified it.)

Additionally I run this code through TestStandartUnwind on ARM what is a basic stress test for the unwinding system and it detected no issues.

[uweigand]

In D18977#399344, @jasonmolenda wrote:

Just to be clear, my concern is we get an register location which is "undefined" (I've never seen it, I don't know what it means) and now SavedLocationForRegister() will say "this register is volatile, not looking any further" when it used to say "register not found in this stack frame" and the unwinder would keep looking down the stack for a save location.

Note that the default state of a register location if no CFI was found at all is "unspecified", which my patch leaves unchanged. A register location can only be "undefined" if it was explicitly set to that state. The only way this can happen, except for my new code in the GetFallbackRegisterLocation, is as a result of a DW_CFA_undefined entry in DWARF CFI.

As Tamas points out, that DWARF opcode is defined to mean the register is not recoverable in this frame, i.e. treating it as "volatile" should do exactly the right thing. (Note that this may be mostly theoretical, since I've not seen any compiler ever in fact issue DW_CFA_undefined ...)

This is was I was briefly alluding to in my original comment "this might now theoretically also trigger when CFI contains an explicit DW_CFA_undefined, but it should actually do the right thing then as well."

[clayborg]

Is the SystemZ ABI not in this patch? Looks good otherwise as long as Jason Molenda is OK with the patch.

[uweigand]

In D18977#400047, @clayborg wrote:

Is the SystemZ ABI not in this patch? Looks good otherwise as long as Jason Molenda is OK with the patch.

The SystemZ ABI is in D18978, which I just re-uploaded.

[jasonmolenda]

Hi Tamas & Ulrich. I'm good with this patch, thanks for double checking that detail for me.

lldb/trunk/include/lldb/Symbol/UnwindPlan.h

public:
             }
 
             bool 
             IsUnspecified () const
             {
                 return m_type == unspecified; 
             }
 
-            bool 
+            bool
+            IsUndefined () const
+            {
+                return m_type == undefined;
+            }
+
+            bool 
             IsCFAPlusOffset () const
             {
                 return m_type == isCFAPlusOffset; 
             }
 
             bool 
             IsAtCFAPlusOffset () const
             {

lldb/trunk/include/lldb/Target/ABI.h

 #define liblldb_ABI_h_
 
 // C Includes
 // C++ Includes
 // Other libraries and framework includes
 // Project includes
 #include "lldb/Core/Error.h"
 #include "lldb/Core/PluginInterface.h"
+#include "lldb/Symbol/UnwindPlan.h"
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/ArrayRef.h"
 
 // forward define the llvm::Type class
 namespace llvm { class Type; }
 
 namespace lldb_private {
public:
     CreateFunctionEntryUnwindPlan (UnwindPlan &unwind_plan) = 0;
 
     virtual bool
     CreateDefaultUnwindPlan (UnwindPlan &unwind_plan) = 0;
 
     virtual bool
     RegisterIsVolatile (const RegisterInfo *reg_info) = 0;
 
+    virtual bool
+    GetFallbackRegisterLocation (const RegisterInfo *reg_info,
+                                 UnwindPlan::Row::RegisterLocation &unwind_regloc);
+
     // Should take a look at a call frame address (CFA) which is just the stack
     // pointer value upon entry to a function. ABIs usually impose alignment
     // restrictions (4, 8 or 16 byte aligned), and zero is usually not allowed.
     // This function should return true if "cfa" is valid call frame address for
     // the ABI, and false otherwise. This is used by the generic stack frame unwinding
     // code to help determine when a stack ends.
     virtual bool
     CallFrameAddressIsValid (lldb::addr_t cfa) = 0;

lldb/trunk/source/Plugins/Process/Utility/RegisterContextLLDB.cpp

if (!have_unwindplan_regloc)
                 {
                     // We were unable to fall back to another unwind plan
                     have_unwindplan_regloc = false;
                 }
             }
         }
     }
 
-    if (have_unwindplan_regloc == false)
-    {
-        // Did the UnwindPlan fail to give us the caller's stack pointer?  
-        // The stack pointer is defined to be the same as THIS frame's CFA, so return the CFA value as
-        // the caller's stack pointer.  This is true on x86-32/x86-64 at least.
-
-        RegisterNumber sp_regnum (m_thread, eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP);
-        if (sp_regnum.GetAsKind (eRegisterKindLLDB) != LLDB_INVALID_REGNUM 
-            && sp_regnum.GetAsKind (eRegisterKindLLDB) == regnum.GetAsKind (eRegisterKindLLDB))
-        {
-            // make sure we won't lose precision copying an addr_t (m_cfa) into a uint64_t (.inferred_value)
-            assert (sizeof (addr_t) <= sizeof (uint64_t));
-            regloc.type = UnwindLLDB::RegisterLocation::eRegisterValueInferred;
-            regloc.location.inferred_value = m_cfa;
-            m_registers[regnum.GetAsKind (eRegisterKindLLDB)] = regloc;
-            UnwindLogMsg ("supplying caller's stack pointer %s (%d) value, computed from CFA", 
-                        regnum.GetName(), regnum.GetAsKind (eRegisterKindLLDB));
-            return UnwindLLDB::RegisterSearchResult::eRegisterFound;
-        }
-    }
-
     ExecutionContext exe_ctx(m_thread.shared_from_this());
     Process *process = exe_ctx.GetProcessPtr();
     if (have_unwindplan_regloc == false)
     {
-        // If a volatile register is being requested, we don't want to forward the next frame's register contents
-        // up the stack -- the register is not retrievable at this frame.
+        // If the UnwindPlan failed to give us an unwind location for this register, we may be able to fall back
+        // to some ABI-defined default.  For example, some ABIs allow to determine the caller's SP via the CFA.
+        // Also, the ABI may set volatile registers to the undefined state.
         ABI *abi = process ? process->GetABI().get() : NULL;
         if (abi)
         {
             const RegisterInfo *reg_info = GetRegisterInfoAtIndex(regnum.GetAsKind (eRegisterKindLLDB));
-            if (reg_info && abi->RegisterIsVolatile (reg_info))
+            if (reg_info && abi->GetFallbackRegisterLocation (reg_info, unwindplan_regloc))
             {
-                UnwindLogMsg ("did not supply reg location for %s (%d) because it is volatile",
+                UnwindLogMsg ("supplying caller's saved %s (%d)'s location using ABI default",
                               regnum.GetName(), regnum.GetAsKind (eRegisterKindLLDB));
-                return UnwindLLDB::RegisterSearchResult::eRegisterIsVolatile;
-            }
-        }
+                have_unwindplan_regloc = true;
+            }
 
+    if (have_unwindplan_regloc == false)
+    {
         if (IsFrameZero ())
         {
             // This is frame 0 - we should return the actual live register context value
             lldb_private::UnwindLLDB::RegisterLocation new_regloc;
             new_regloc.type = UnwindLLDB::RegisterLocation::eRegisterInLiveRegisterContext;
             new_regloc.location.register_number = regnum.GetAsKind (eRegisterKindLLDB);
             m_registers[regnum.GetAsKind (eRegisterKindLLDB)] = new_regloc;
             regloc = new_regloc;
if (unwindplan_regloc.IsUnspecified())
         lldb_private::UnwindLLDB::RegisterLocation new_regloc;
         new_regloc.type = UnwindLLDB::RegisterLocation::eRegisterNotSaved;
         m_registers[regnum.GetAsKind (eRegisterKindLLDB)] = new_regloc;
         UnwindLogMsg ("save location for %s (%d) is unspecified, continue searching", 
                       regnum.GetName(), regnum.GetAsKind (eRegisterKindLLDB));
         return UnwindLLDB::RegisterSearchResult::eRegisterNotFound;
     }
 
+    if (unwindplan_regloc.IsUndefined())
+    {
+         UnwindLogMsg ("did not supply reg location for %s (%d) because it is volatile",
+                       regnum.GetName(), regnum.GetAsKind (eRegisterKindLLDB));
+         return UnwindLLDB::RegisterSearchResult::eRegisterIsVolatile;
+    }
+
     if (unwindplan_regloc.IsSame())
     {
         regloc.type = UnwindLLDB::RegisterLocation::eRegisterInRegister;
         regloc.location.register_number = regnum.GetAsKind (eRegisterKindLLDB);
         m_registers[regnum.GetAsKind (eRegisterKindLLDB)] = regloc;
         UnwindLogMsg ("supplying caller's register %s (%d), saved in register %s (%d)", 
                       regnum.GetName(), regnum.GetAsKind (eRegisterKindLLDB), 
                       regnum.GetName(), regnum.GetAsKind (eRegisterKindLLDB));

lldb/trunk/source/Target/ABI.cpp

ABI::PrepareTrivialCall (Thread &thread,
                     lldb::addr_t returnAddress,
                     llvm::Type  &returntype,
                     llvm::ArrayRef<ABI::CallArgument> args) const
 {
     // dummy prepare trivial call
     assert( !"Should never get here!" );
     return false;
 }
+
+bool
+ABI::GetFallbackRegisterLocation (const RegisterInfo *reg_info,
+                                  UnwindPlan::Row::RegisterLocation &unwind_regloc)
+{
+    // Did the UnwindPlan fail to give us the caller's stack pointer?
+    // The stack pointer is defined to be the same as THIS frame's CFA, so return the CFA value as
+    // the caller's stack pointer.  This is true on x86-32/x86-64 at least.
+    if (reg_info->kinds[eRegisterKindGeneric] == LLDB_REGNUM_GENERIC_SP)
+    {
+        unwind_regloc.SetIsCFAPlusOffset(0);
+        return true;
+    }
+
+    // If a volatile register is being requested, we don't want to forward the next frame's register contents
+    // up the stack -- the register is not retrievable at this frame.
+    if (RegisterIsVolatile(reg_info))
+    {
+        unwind_regloc.SetUndefined();
+        return true;
+    }
+
+    return false;
+}