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()?

162

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

36

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

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

source/Plugins/Process/Utility/RegisterInfos_x86_64.h

128–130

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

336–338

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

Hopefully we can hide this and not define this.

157–159

Hopefully we can hide this and not define this.

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

2633–2657

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

 //===-- NativeRegisterContextLinux_x86_64.cpp ---------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #if defined(__i386__) || defined(__x86_64__)
 
+#include "Plugins/Process/Utility/RegisterContextLinux_i386.h"
+#include "Plugins/Process/Utility/RegisterContextLinux_x86_64.h"
+
 #include "NativeRegisterContextLinux_x86_64.h"
 
 #include "lldb/Core/Log.h"
 #include "lldb/Core/DataBufferHeap.h"
 #include "lldb/Core/Error.h"
 #include "lldb/Core/RegisterValue.h"
 #include "lldb/Host/HostInfo.h"
 
-#include "Plugins/Process/Utility/RegisterContextLinux_i386.h"
-#include "Plugins/Process/Utility/RegisterContextLinux_x86_64.h"
-
 using namespace lldb_private;
 using namespace lldb_private::process_linux;
 
 // ----------------------------------------------------------------------------
 // Private namespace.
 // ----------------------------------------------------------------------------
 
 namespace
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_orig_eax_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,
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_orig_rax_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,

source/Plugins/Process/Utility/RegisterContextLinux_i386.cpp

 //===-- RegisterContextLinux_i386.cpp --------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
-#include "RegisterContextPOSIX_x86.h"
 #include "RegisterContextLinux_i386.h"
+#include "RegisterContextPOSIX_x86.h"
 
 using namespace lldb_private;
 using namespace lldb;
 
 struct GPR
 {
     uint32_t ebx;
     uint32_t ecx;
     uint32_t edx;
     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 DR_0_OFFSET 0xFC
 #define DR_OFFSET(reg_index) \
     (DR_0_OFFSET + (reg_index * 4))
 #define FPR_SIZE(reg) sizeof(((FPR_i386*)NULL)->reg)
 
 //---------------------------------------------------------------------------
 // Include RegisterInfos_i386 to declare our g_register_infos_i386 structure.
 //---------------------------------------------------------------------------
+#define LINUX
 #define DECLARE_REGISTER_INFOS_I386_STRUCT
 #include "RegisterInfos_i386.h"
 #undef DECLARE_REGISTER_INFOS_I386_STRUCT
+#undef LINUX
 
 RegisterContextLinux_i386::RegisterContextLinux_i386(const ArchSpec &target_arch) :
     RegisterInfoInterface(target_arch)
 {
 }
 
 size_t
 RegisterContextLinux_i386::GetGPRSize() const

source/Plugins/Process/Utility/RegisterContextLinux_x86_64.cpp

 //===-- RegisterContextLinux_x86_64.cpp ------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #include <vector>
-#include "RegisterContextPOSIX_x86.h"
-#include "RegisterContextLinux_i386.h"
+
 #include "RegisterContextLinux_x86_64.h"
+#include "RegisterContextLinux_i386.h"
+#include "RegisterContextPOSIX_x86.h"
 
 using namespace lldb_private;
 using namespace lldb;
 
 typedef struct _GPR
 {
     uint64_t r15;
     uint64_t r14;
     uint64_t r13;
     uint64_t r12;
     uint64_t rbp;
     uint64_t rbx;
     uint64_t r11;
     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;

 
 #define DR_OFFSET(reg_index) \
     (LLVM_EXTENSION offsetof(UserArea, dbg) + \
      LLVM_EXTENSION offsetof(DBG, dr[reg_index]))
 
 //---------------------------------------------------------------------------
 // Include RegisterInfos_x86_64 to declare our g_register_infos_x86_64 structure.
 //---------------------------------------------------------------------------
+#define LINUX
 #define DECLARE_REGISTER_INFOS_X86_64_STRUCT
 #include "RegisterInfos_x86_64.h"
 #undef DECLARE_REGISTER_INFOS_X86_64_STRUCT
+#undef LINUX
 
 static std::vector<lldb_private::RegisterInfo>&
 GetPrivateRegisterInfoVector ()
 {
     static std::vector<lldb_private::RegisterInfo> g_register_infos;
     return g_register_infos;
 }
 
if (g_register_infos.empty())
         std::unique_ptr<RegisterContextLinux_i386> reg_interface(new RegisterContextLinux_i386 (arch));
         const RegisterInfo *base_info = reg_interface->GetRegisterInfo();
         g_register_infos.insert(g_register_infos.end(), &base_info[0], &base_info[k_num_registers_i386]);
 
         //---------------------------------------------------------------------------
         // Include RegisterInfos_x86_64 to update the g_register_infos structure
         //  with x86_64 offsets.
         //---------------------------------------------------------------------------
+        #define LINUX
         #define UPDATE_REGISTER_INFOS_I386_STRUCT_WITH_X86_64_OFFSETS
         #include "RegisterInfos_x86_64.h"
         #undef UPDATE_REGISTER_INFOS_I386_STRUCT_WITH_X86_64_OFFSETS
+        #undef LINUX
     }
 
     return &g_register_infos[0];
 }
 
 static const RegisterInfo *
 GetRegisterInfoPtr (const ArchSpec &target_arch)
 {

source/Plugins/Process/Utility/RegisterInfos_i386.h

g_register_infos_i386[] =
     DEFINE_GPR(eip,    "pc",    gcc_eip_i386,        dwarf_eip_i386,      LLDB_REGNUM_GENERIC_PC,    gdb_eip_i386),
     DEFINE_GPR(eflags, "flags", gcc_eflags_i386,     dwarf_eflags_i386,   LLDB_REGNUM_GENERIC_FLAGS, gdb_eflags_i386),
     DEFINE_GPR(cs,     NULL,    LLDB_INVALID_REGNUM, dwarf_cs_i386,       LLDB_INVALID_REGNUM,       gdb_cs_i386),
     DEFINE_GPR(fs,     NULL,    LLDB_INVALID_REGNUM, dwarf_fs_i386,       LLDB_INVALID_REGNUM,       gdb_fs_i386),
     DEFINE_GPR(gs,     NULL,    LLDB_INVALID_REGNUM, dwarf_gs_i386,       LLDB_INVALID_REGNUM,       gdb_gs_i386),
     DEFINE_GPR(ss,     NULL,    LLDB_INVALID_REGNUM, dwarf_ss_i386,       LLDB_INVALID_REGNUM,       gdb_ss_i386),
     DEFINE_GPR(ds,     NULL,    LLDB_INVALID_REGNUM, dwarf_ds_i386,       LLDB_INVALID_REGNUM,       gdb_ds_i386),
     DEFINE_GPR(es,     NULL,    LLDB_INVALID_REGNUM, dwarf_es_i386,       LLDB_INVALID_REGNUM,       gdb_es_i386),
+#ifdef LINUX
+    DEFINE_GPR(orig_eax,     NULL,    LLDB_INVALID_REGNUM,     LLDB_INVALID_REGNUM,     LLDB_INVALID_REGNUM,       LLDB_INVALID_REGNUM),
+#endif
 
     DEFINE_GPR_PSEUDO_16(ax,  eax),
     DEFINE_GPR_PSEUDO_16(bx,  ebx),
     DEFINE_GPR_PSEUDO_16(cx,  ecx),
     DEFINE_GPR_PSEUDO_16(dx,  edx),
     DEFINE_GPR_PSEUDO_16(di,  edi),
     DEFINE_GPR_PSEUDO_16(si,  esi),
     DEFINE_GPR_PSEUDO_16(bp,  ebp),

source/Plugins/Process/Utility/RegisterInfos_x86_64.h

g_register_infos_x86_64[] =
     DEFINE_GPR(rip,    "pc",    gcc_dwarf_rip_x86_64,    gcc_dwarf_rip_x86_64,    LLDB_REGNUM_GENERIC_PC,    gdb_rip_x86_64),
     DEFINE_GPR(rflags, "flags", gcc_dwarf_rflags_x86_64, gcc_dwarf_rflags_x86_64, LLDB_REGNUM_GENERIC_FLAGS, gdb_rflags_x86_64),
     DEFINE_GPR(cs,     NULL,    gcc_dwarf_cs_x86_64,     gcc_dwarf_cs_x86_64,     LLDB_INVALID_REGNUM,       gdb_cs_x86_64),
     DEFINE_GPR(fs,     NULL,    gcc_dwarf_fs_x86_64,     gcc_dwarf_fs_x86_64,     LLDB_INVALID_REGNUM,       gdb_fs_x86_64),
     DEFINE_GPR(gs,     NULL,    gcc_dwarf_gs_x86_64,     gcc_dwarf_gs_x86_64,     LLDB_INVALID_REGNUM,       gdb_gs_x86_64),
     DEFINE_GPR(ss,     NULL,    gcc_dwarf_ss_x86_64,     gcc_dwarf_ss_x86_64,     LLDB_INVALID_REGNUM,       gdb_ss_x86_64),
     DEFINE_GPR(ds,     NULL,    gcc_dwarf_ds_x86_64,     gcc_dwarf_ds_x86_64,     LLDB_INVALID_REGNUM,       gdb_ds_x86_64),
     DEFINE_GPR(es,     NULL,    gcc_dwarf_es_x86_64,     gcc_dwarf_es_x86_64,     LLDB_INVALID_REGNUM,       gdb_es_x86_64),
+#ifdef LINUX
+    DEFINE_GPR(orig_rax,     NULL,    LLDB_INVALID_REGNUM,     LLDB_INVALID_REGNUM,     LLDB_INVALID_REGNUM,       LLDB_INVALID_REGNUM),
+#endif
 
     DEFINE_GPR_PSEUDO_32(eax, rax),
     DEFINE_GPR_PSEUDO_32(ebx, rbx),
     DEFINE_GPR_PSEUDO_32(ecx, rcx),
     DEFINE_GPR_PSEUDO_32(edx, rdx),
     DEFINE_GPR_PSEUDO_32(edi, rdi),
     DEFINE_GPR_PSEUDO_32(esi, rsi),
     DEFINE_GPR_PSEUDO_32(ebp, rbp),
do {                                                                            \
     UPDATE_GPR_INFO(eip,    rip);
     UPDATE_GPR_INFO(eflags, rflags);
     UPDATE_GPR_INFO(cs,     cs);
     UPDATE_GPR_INFO(fs,     fs);
     UPDATE_GPR_INFO(gs,     gs);
     UPDATE_GPR_INFO(ss,     ss);
     UPDATE_GPR_INFO(ds,     ds);
     UPDATE_GPR_INFO(es,     es);
-
+#ifdef LINUX
+    UPDATE_GPR_INFO(orig_eax, orig_rax);
+#endif
     UPDATE_GPR_INFO(ax,     rax);
     UPDATE_GPR_INFO(bx,     rbx);
     UPDATE_GPR_INFO(cx,     rcx);
     UPDATE_GPR_INFO(dx,     rdx);
     UPDATE_GPR_INFO(di,     rdi);
     UPDATE_GPR_INFO(si,     rsi);
     UPDATE_GPR_INFO(bp,     rbp);
     UPDATE_GPR_INFO(sp,     rsp);

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

enum
         lldb_eip_i386,
         lldb_eflags_i386,
         lldb_cs_i386,
         lldb_fs_i386,
         lldb_gs_i386,
         lldb_ss_i386,
         lldb_ds_i386,
         lldb_es_i386,
-
+#ifdef liblldb_RegisterContextLinux_i386_H_
+        lldb_orig_eax_i386,
+#endif
         k_first_alias_i386,
         lldb_ax_i386 = k_first_alias_i386,
         lldb_bx_i386,
         lldb_cx_i386,
         lldb_dx_i386,
         lldb_di_i386,
         lldb_si_i386,
         lldb_bp_i386,
enum
         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,
-
+#ifdef liblldb_RegisterContextLinux_x86_64_H_
+        lldb_orig_rax_x86_64,
+#endif
         k_first_alias_x86_64,
         lldb_eax_x86_64 = k_first_alias_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,

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

GDBRemoteCommunicationServerLLGS::Handle_QSaveRegisterState (StringExtractorGDBRemote &packet)
     assert ((m_saved_registers_map.find (save_id) == m_saved_registers_map.end ()) && "GetNextRegisterSaveID() returned an existing register save id");
 
     // Save the register data buffer under the save id.
     {
         Mutex::Locker locker (m_saved_registers_mutex);
         m_saved_registers_map[save_id] = register_data_sp;
     }
 
+    /** 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.
+     * **/
+
+    ArchSpec process_arch;
+    if (m_debugged_process_sp->GetArchitecture(process_arch))
+    {
+        llvm::Triple t_triple = process_arch.GetTriple();
+
+        if (t_triple.getOS() == llvm::Triple::Linux &&
+           (t_triple.getArch() == llvm::Triple::x86 ||
+            t_triple.getArch() == llvm::Triple::x86_64))
+        {
+            const RegisterInfo *reg_info = reg_context_sp->GetRegisterInfoByName ("orig_eax");
+            if (reg_info == nullptr)
+                reg_info = reg_context_sp->GetRegisterInfoByName ("orig_rax");
+
+            if (reg_info != nullptr)
+                reg_context_sp->WriteRegisterFromUnsigned(reg_info, -1);
+
+        }
+    }
     // Write the response.
     StreamGDBRemote response;
     response.Printf ("%" PRIu32, save_id);
     return SendPacketNoLock(response.GetData(), response.GetSize());
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_QRestoreRegisterState (StringExtractorGDBRemote &packet)

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')