Set orig_eax to -1 for Linux x86 platforms

[ravitheja]

Summary

For Linux x86 based environments the orig_eax/orig_rax
register should be set to -1 to prevent the instruction pointer
to be decremented, which was the cause for the SIGILL exception.

Fix for Bug 23659

[labath]

Ravi, thanks for working on this.

Greg, could you please take a look at this?

The general story is that on x86 linux we have a special pseudo-register, which controls what happens when we resume a process which was interrupted in a syscall. When evaluating an expression, we need to set it to -1 to avoid nasty surprises. Doing this inside a ThreadPlan doesn't sound like a good idea, but I am don't know enough about the codebase to say where it should go: ABI plugin? Platform?

source/Target/ThreadPlanCallFunction.cpp

189 ↗

(On Diff #30343)

This loop can be replaced by reg_ctx->GetRegisterInfo(eRegisterKindLLDB, lldb_orig_rax_x86_64) (and similarly for i386), right?

[clayborg]

Please add Jim Ingham as a reviewer. He will have the best input on the subject.

source/Target/ThreadPlanCallFunction.cpp

188–200 ↗

(On Diff #30343)

Don't you actually know it should be "orig_eax" by if it is i386 or "orig_rax" for x86_64?

Best to use:

const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoByName ("orig_eax");
if (reg_info == nullptr)
    reg_info = reg_ctx->GetRegisterInfoByName ("orig_rax");

instead of your for loop.

But this isn't the way to do this. This seems like some sort of thing that should be Process::PrepareForExpression(), or Platform::PrepareForExpression(). We might then need a Process::CleanupFromExpression()/Platform::CleanupFromExpression() to cleanup?

[clayborg]

The main questions is why is there an extra decrement of the PC? Is there a breakpoint being set on the return address and then that breakpoint is being removed before we detect it was there and we backup the PC incorrectly? Can you elaborate on 23659 a bit?

[labath]

In D11411#214398, @clayborg wrote:

The main questions is why is there an extra decrement of the PC? Is there a breakpoint being set on the return address and then that breakpoint is being removed before we detect it was there and we backup the PC incorrectly? Can you elaborate on 23659 a bit?

This is pretty crazy stuff. The extra decrement happens inside the kernel when we resume the process. This is how syscall restart is implemented on linux. When we stop the inferior inside a syscall (let's say we interrupt a sleep() for example), the kernel presents us with the following view:

previous instructions
syscall
next instruction <--- PC

However, when the process resumes, the kernel will actually decrement the PC by 2 before letting it run. This will cause the process to re-execute the syscall instruction, which will give the kernel a chance to finish the syscall. All this is fine unless we actually move the PC, like we do when evaluating expressions, since then the extra decrement will likely land us in the middle of an instruction. This behavior is controlled by the orig_rax pseudo-register.

I don't think we need to do any special cleanup after this, since this "register" should get saved when we save all registers and then restored. Ravi, can you confirm this?

[ravitheja]

Changes for accessing RegiserInfo
Replaced for loop with GetRegisterInfoByName to access
orig_eax/orig_rax register.

[ravitheja]

In D11411#214417, @labath wrote:

In D11411#214398, @clayborg wrote:

The main questions is why is there an extra decrement of the PC? Is there a breakpoint being set on the return address and then that breakpoint is being removed before we detect it was there and we backup the PC incorrectly? Can you elaborate on 23659 a bit?

This is pretty crazy stuff. The extra decrement happens inside the kernel when we resume the process. This is how syscall restart is implemented on linux. When we stop the inferior inside a syscall (let's say we interrupt a sleep() for example), the kernel presents us with the following view:

previous instructions
syscall
next instruction <--- PC

However, when the process resumes, the kernel will actually decrement the PC by 2 before letting it run. This will cause the process to re-execute the syscall instruction, which will give the kernel a chance to finish the syscall. All this is fine unless we actually move the PC, like we do when evaluating expressions, since then the extra decrement will likely land us in the middle of an instruction. This behavior is controlled by the orig_rax pseudo-register.

I don't think we need to do any special cleanup after this, since this "register" should get saved when we save all registers and then restored. Ravi, can you confirm this?

Yes, the explanation is correct, its more a Linux kernel implementation thing and no special cleanup is required. I did check before submitting for review that, after doing the expression evaluation the program resumes the interrupted syscall when the continue command is issued, so the registers are saved.

[clayborg]

So we have something similar on MacOSX where if a thread is in the kernel doing a syscall we have to ask the thread to abort safely.

So you can actually fix this in lldb-server by doing what you need to do in response to the:

< 37> send packet: $QSaveRegisterState;thread:8edbb7;#4a
< 5> read packet: $1#00

When you see this "QSaveRegisterState" packet, you can do what you need to do for a thread. When the expression is done, you will get the following packet:

< 42> send packet: $QRestoreRegisterState:1;thread:8edbb7;#0a
< 6> read packet: $OK#00

Then you can restore the registers however you need to. So no changes should be required on the LLDB side. Just in lldb-server.

[ravitheja]

@clayborg this patch was done in context of Bug 23659 which was reported on Linux, could you please check if the Bug is applicable to MacOS as well ?

[jingham]

On Mac OS X when we call a function on a thread we have to force the thread into a safe state before we can call functions on it. This has the effect of interrupting any system call pending on that thread, but we have to do it or we would end up corrupting register state (not the PC but also any of the other registers that were changed in the kernel but not reflected in the user thread state.)

So we don't see the bug you are seeing. We see the undesirable but unavoidable behavior that calling a function on a thread sitting in a syscall will cause that syscall to be interrupted.

Anyway, the point relevant to this bug is that lldb offloads all the "storing of state before running a function on a thread" and "restoring said state" to debugserver, which it implements in response to the QSaveRegisterState, QRestoreRegisterState packets. You could do the same thing, and hide the Linux specific gnarliness to your server. This packet is also more efficient since you just send a token back and forth rather than a complete copy of the register set in both directions.

[labath]

The only issue I have with doing this on the server is that the name of the packet (QSaveRegisterState) gives no indication that it is going to change the state of the inferior in any way (ok, maybe except the capital Q). However, the description does state that it is used to prepare for expression evaluation and it matches the OSX behavior most closely, so I guess that could be the way to go.

[ravitheja]

Moved implementation to lldb-server

[ravitheja]

Correcting previous mistake
In last update, accidentally few files were excluded.
I am readding them

[ravitheja]

Sorry again.

[ravitheja]

I am sorry for those accidents, I have verified the present diff and it is correct.

[clayborg]

It would be great to be able to make this change somewhere lower in the chain without having to define the extra orig_eax register. No one should have to see this register and the implementation should be in the x86_64 and i386 specific code. See inline comments and make new virtual functions in NativeRegisterContext to save and restore register state, move the existing code from GDBRemoteCommunicationServerLLGS::Handle_QSaveRegisterState() that gets all the registers into a buffer and move it to the NativeRegisterContext::SaveRegisterState(...). If we can, then much of this patch goes away.

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

52

No one should ever need to see this register, hopefully we can not declare it and do the work in NativeRegisterContext::ReadAllRegisterValues()?

161

No one should ever need to see this register, hopefully we can not declare it and do the work in NativeRegisterContext::ReadAllRegisterValues()?

source/Plugins/Process/Utility/RegisterContextLinux_i386.cpp

29

Should be fine to have in this structure, but no one outside of this class should know about this register.

source/Plugins/Process/Utility/RegisterContextLinux_x86_64.cpp

35

Should be fine to have in this structure, but no one outside of this class should know about this register.

source/Plugins/Process/Utility/RegisterInfos_i386.h

117–119 ↗

(On Diff #31240)

No one outside of this class should know about this register, so remove this.

source/Plugins/Process/Utility/RegisterInfos_x86_64.h

128–130 ↗

(On Diff #31240)

No one outside of this class should know about this register, so remove this.

336–338 ↗

(On Diff #31240)

No one outside of this class should know about this register, so remove this.

source/Plugins/Process/Utility/lldb-x86-register-enums.h

39–41 ↗

(On Diff #31240)

Hopefully we can hide this and not define this.

157–159 ↗

(On Diff #31240)

Hopefully we can hide this and not define this.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

2633–2657 ↗

(On Diff #31240)

Move this code into the x86_64 and i386 versions of NativeRegisterContext::ReadAllRegisterValues(...). which is called on line 2615 of this file.

[ravitheja]

@clayborg
The register orig_eax/orig_rax could be important in some debugging scenarios and the user may want to see its value (one particular use case is the investigation of this bug), with the present changes the user can directly print the value of this register.
Secondly the register offsets calculation is done in RegisterInfos_x86_64.h and RegisterInfos_i386.h, in case if I drop those changes I need to then hardcode the register offset (or something close). The actual settings of the register could be moved to a place lower in the chain as u suggested. It would be great if you could clarify on these points i.e ->

1. Whether we want the user to be able to print this register during debugging.
2. Remove the offset calculation in RegisterInfos_x86_64.h and RegisterInfos_i386.h and do something like hardcoding the register offset for orig_rax/eax (plus the enums in lldb-x86-register-enums.h are needed otherwise there are assertions for the mismatch in the number of registers).
3. The actual settings of the register can be moved to a place lower in the chain without disturbing the other changes.

[clayborg]

Comments for your questions:

• Whether we want the user to be able to print this register during debugging.

If this is purely an implementation detail, then we shouldn't show it to the user. The user shouldn't know or care about this register. Furthermore you don't want them to start relying on having it be around in case you take it away later after the kernel changes how it handles such things.

• Remove the offset calculation in RegisterInfos_x86_64.h and RegisterInfos_i386.h and do something like hardcoding the register offset for orig_rax/eax (plus the enums in lldb-x86-register-enums.h are needed otherwise there are assertions for the mismatch in the number of registers).

This could always be stored at the end of all the registers at a dynamic offset. Or you could always place it at offset zero.

• The actual settings of the register can be moved to a place lower in the chain without disturbing the other changes.

Lets try to hide it completely if we can. Who knows what will change in the future and if certain linux kernel versions will do different things, so the less the user knows about implementation details keeps our debugger doing what it should with no distractions.

You also commented on it being nice to see when debugging this issue: this can and should be handled by logging.

[ravitheja]

Changes for previous feedback

[clayborg]

Looks good.

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

g_gpr_regnums_i386[] =
         lldb_eip_i386,
         lldb_eflags_i386,
         lldb_cs_i386,
         lldb_fs_i386,
         lldb_gs_i386,
         lldb_ss_i386,
         lldb_ds_i386,
         lldb_es_i386,
         lldb_ax_i386,
         lldb_bx_i386,
         lldb_cx_i386,
         lldb_dx_i386,
         lldb_di_i386,
         lldb_si_i386,
         lldb_bp_i386,
         lldb_sp_i386,
         lldb_ah_i386,
uint32_t g_gpr_regnums_x86_64[] =
         lldb_rip_x86_64,
         lldb_rflags_x86_64,
         lldb_cs_x86_64,
         lldb_fs_x86_64,
         lldb_gs_x86_64,
         lldb_ss_x86_64,
         lldb_ds_x86_64,
         lldb_es_x86_64,
         lldb_eax_x86_64,
         lldb_ebx_x86_64,
         lldb_ecx_x86_64,
         lldb_edx_x86_64,
         lldb_edi_x86_64,
         lldb_esi_x86_64,
         lldb_ebp_x86_64,
         lldb_esp_x86_64,
         lldb_r8d_x86_64,    // Low 32 bits or r8
else if (GetFPRType () == eFPRTypeXSAVE)
         // Copy the extended register state including the assembled ymm registers.
         ::memcpy (dst, &m_fpr, sizeof (m_fpr));
     }
     else
     {
         assert (false && "how do we save the floating point registers?");
         error.SetErrorString ("unsure how to save the floating point registers");
     }
+    /** The following code is specific to Linux x86 based architectures,
+     *  where the register orig_eax (32 bit)/orig_rax (64 bit) is set to
+     *  -1 to solve the bug 23659, such a setting prevents the automatic
+     *  decrement of the instruction pointer which was causing the SIGILL
+     *  exception.
+     * **/
+    llvm::Triple t_triple = GetRegisterInfoInterface().GetTargetArchitecture().GetTriple();
+
+        if (t_triple.getOS() == llvm::Triple::Linux &&
+           (t_triple.getArch() == llvm::Triple::x86 ||
+            t_triple.getArch() == llvm::Triple::x86_64))
+        {
+            RegisterValue value((uint64_t) -1);
+            const RegisterInfo *reg_info = GetRegisterInfoInterface().GetDynamicRegisterInfo("orig_eax");
+            if (reg_info == nullptr)
+                reg_info = GetRegisterInfoInterface().GetDynamicRegisterInfo("orig_rax");
+
+            if (reg_info != nullptr) {
+                NativeProcessProtocolSP process_sp(m_thread.GetProcess());
+                if (!process_sp)
+                    return Error("NativeProcessProtocol is NULL");
+
+                NativeProcessLinux* process_p = static_cast<NativeProcessLinux*>(process_sp.get());
+                return process_p->DoOperation([&] {
+                    return DoWriteRegisterValue(reg_info->byte_offset,reg_info->name,value);
+                });
+            }
+        }
 
     return error;
 }
 
 Error
 NativeRegisterContextLinux_x86_64::WriteAllRegisterValues (const lldb::DataBufferSP &data_sp)
 {
     Error error;

source/Plugins/Process/Utility/RegisterContextLinux_i386.h

public:
     const lldb_private::RegisterInfo *
     GetRegisterInfo() const override;
 
     uint32_t
     GetRegisterCount () const override;
 
     uint32_t
     GetUserRegisterCount () const override;
+
+    const std::vector<lldb_private::RegisterInfo> *
+    GetDynamicRegisterInfoP() const override;
+
+private:
+    std::vector<lldb_private::RegisterInfo> d_register_infos;
 };
 
 #endif

source/Plugins/Process/Utility/RegisterContextLinux_i386.cpp

struct GPR
     uint32_t esi;
     uint32_t edi;
     uint32_t ebp;
     uint32_t eax;
     uint32_t ds;
     uint32_t es;
     uint32_t fs;
     uint32_t gs;
-    uint32_t orig_ax;
+    uint32_t orig_eax;
     uint32_t eip;
     uint32_t cs;
     uint32_t eflags;
     uint32_t esp;
     uint32_t ss;
 };
 
 struct FPR_i386

 //---------------------------------------------------------------------------
 #define DECLARE_REGISTER_INFOS_I386_STRUCT
 #include "RegisterInfos_i386.h"
 #undef DECLARE_REGISTER_INFOS_I386_STRUCT
 
 RegisterContextLinux_i386::RegisterContextLinux_i386(const ArchSpec &target_arch) :
     RegisterInfoInterface(target_arch)
 {
+    RegisterInfo orig_ax = { "orig_eax", NULL, sizeof(((GPR*)NULL)->orig_eax), (LLVM_EXTENSION offsetof(GPR, orig_eax)), eEncodingUint, \
+              eFormatHex, { LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM }, NULL, NULL };
+    d_register_infos.push_back(orig_ax);
 }
 
 size_t
 RegisterContextLinux_i386::GetGPRSize() const
 {
     return sizeof(GPR);
 }
 
RegisterContextLinux_i386::GetRegisterCount () const
     return static_cast<uint32_t> (sizeof (g_register_infos_i386) / sizeof (g_register_infos_i386 [0]));
 }
 
 uint32_t
 RegisterContextLinux_i386::GetUserRegisterCount () const
 {
     return static_cast<uint32_t> (k_num_user_registers_i386);
 }
+
+const std::vector<lldb_private::RegisterInfo> *
+RegisterContextLinux_i386::GetDynamicRegisterInfoP() const
+{
+    return &d_register_infos;
+}

source/Plugins/Process/Utility/RegisterContextLinux_x86_64.h

public:
     GetRegisterInfo() const override;
 
     uint32_t
     GetRegisterCount () const override;
 
     uint32_t
     GetUserRegisterCount () const override;
 
+    const std::vector<lldb_private::RegisterInfo> *
+    GetDynamicRegisterInfoP() const override;
+
 private:
     const lldb_private::RegisterInfo *m_register_info_p;
     uint32_t m_register_info_count;
     uint32_t m_user_register_count;
+    std::vector<lldb_private::RegisterInfo> d_register_infos;
+
 };
 
 #endif

source/Plugins/Process/Utility/RegisterContextLinux_x86_64.cpp

typedef struct _GPR
     uint64_t r10;
     uint64_t r9;
     uint64_t r8;
     uint64_t rax;
     uint64_t rcx;
     uint64_t rdx;
     uint64_t rsi;
     uint64_t rdi;
-    uint64_t orig_ax;
+    uint64_t orig_rax;
     uint64_t rip;
     uint64_t cs;
     uint64_t rflags;
     uint64_t rsp;
     uint64_t ss;
     uint64_t fs_base;
     uint64_t gs_base;
     uint64_t ds;

 }
 
 RegisterContextLinux_x86_64::RegisterContextLinux_x86_64(const ArchSpec &target_arch) :
     lldb_private::RegisterInfoInterface(target_arch),
     m_register_info_p (GetRegisterInfoPtr (target_arch)),
     m_register_info_count (GetRegisterInfoCount (target_arch)),
     m_user_register_count (GetUserRegisterInfoCount (target_arch))
 {
+    RegisterInfo orig_ax = { "orig_rax", NULL, sizeof(((GPR*)NULL)->orig_rax), (LLVM_EXTENSION offsetof(GPR, orig_rax)), eEncodingUint, \
+              eFormatHex, { LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM }, NULL, NULL };
+    d_register_infos.push_back(orig_ax);
 }
 
 size_t
 RegisterContextLinux_x86_64::GetGPRSize() const
 {
     return sizeof(GPR);
 }
 
+const std::vector<lldb_private::RegisterInfo> *
+RegisterContextLinux_x86_64::GetDynamicRegisterInfoP() const
+{
+    return &d_register_infos;
+}
+
 const RegisterInfo *
 RegisterContextLinux_x86_64::GetRegisterInfo() const
 {
     return m_register_info_p;
 }
 
 uint32_t
 RegisterContextLinux_x86_64::GetRegisterCount () const

source/Plugins/Process/Utility/RegisterInfoInterface.h

public:
         {
             return GetRegisterCount();
         }
 
         const lldb_private::ArchSpec&
         GetTargetArchitecture() const
             { return m_target_arch; }
 
+        virtual const lldb_private::RegisterInfo *
+        GetDynamicRegisterInfo(const char *reg_name) const
+        {
+            const std::vector <lldb_private::RegisterInfo> * d_register_infos = GetDynamicRegisterInfoP();
+            if(d_register_infos != nullptr)
+            {
+                std::vector <lldb_private::RegisterInfo> ::const_iterator pos = d_register_infos->begin();
+                for(; pos < d_register_infos->end() ; pos++)
+                {
+                    if(::strcmp(reg_name, pos->name) == 0)
+                        return(d_register_infos->data() + (pos - d_register_infos->begin()) );
+                }
+            }
+            return nullptr;
+        }
+
+        virtual const std::vector<lldb_private::RegisterInfo> *
+        GetDynamicRegisterInfoP() const
+        { return nullptr; }
+
     public:
         // FIXME make private.
         lldb_private::ArchSpec m_target_arch;
     };
 
 }
 
 #endif

test/expression_command/expr-in-syscall/TestExpressionInSyscall.py

class ExprSyscallTestCase(TestBase):
     mydir = TestBase.compute_mydir(__file__)
 
     @skipUnlessDarwin
     @dsym_test
     def test_setpgid_with_dsym(self):
         self.buildDsym()
         self.expr_syscall()
 
-    @expectedFailureAll("llvm.org/pr23659", oslist=["linux"], archs=["i386", "x86_64"])
     @dwarf_test
     def test_setpgid_with_dwarf(self):
         self.buildDwarf()
         self.expr_syscall()
 
     def expr_syscall(self):
         exe = os.path.join(os.getcwd(), 'a.out')