[RFC][PATCH] Demultiplexing SIGTRAP signal

Previous thread: [PATCH] x86: do_boot_cpu - add check if we have ESR register by Cyrill Gorcunov on Monday, September 22, 2008 - 2:36 am. (8 messages)

Next thread: [PATCH 0/13] memory cgroup updates v4 by KAMEZAWA Hiroyuki on Monday, September 22, 2008 - 3:51 am. (37 messages)
From: Srinivasa Ds
Date: Monday, September 22, 2008 - 3:32 am

Currently a SIGTRAP signal can denote any one of below reasons.
	- Breakpoint hit
	- H/W debug register hit
	- Single step
	- SIGTRAP signal sent through kill() or rasie()

Architectures like powerpc/parisc provides infrastructure to demultiplex
SIGTRAP signal by passing down the information for receiving SIGTRAP through
si_code of siginfot_t structure. Here is an attempt is generalise this 
infrastructure by extending it to x86 and x86_64 archs. 

Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com>



---
 arch/ia64/include/asm/siginfo.h    |    5 -----
 arch/powerpc/include/asm/siginfo.h |    5 -----
 arch/x86/kernel/traps_32.c         |   19 +++++++++++++++++--
 arch/x86/kernel/traps_64.c         |    7 ++++++-
 include/asm-generic/siginfo.h      |    2 ++
 include/asm-parisc/siginfo.h       |    5 -----
 6 files changed, 25 insertions(+), 18 deletions(-)

Index: linux-2.6.27-rc7/arch/ia64/include/asm/siginfo.h
===================================================================
--- linux-2.6.27-rc7.orig/arch/ia64/include/asm/siginfo.h
+++ linux-2.6.27-rc7/arch/ia64/include/asm/siginfo.h
@@ -113,11 +113,6 @@ typedef struct siginfo {
 #undef NSIGSEGV
 #define NSIGSEGV	3
 
-/*
- * SIGTRAP si_codes
- */
-#define TRAP_BRANCH	(__SI_FAULT|3)	/* process taken branch trap */
-#define TRAP_HWBKPT	(__SI_FAULT|4)	/* hardware breakpoint or watchpoint */
 #undef NSIGTRAP
 #define NSIGTRAP	4
 
Index: linux-2.6.27-rc7/arch/powerpc/include/asm/siginfo.h
===================================================================
--- linux-2.6.27-rc7.orig/arch/powerpc/include/asm/siginfo.h
+++ linux-2.6.27-rc7/arch/powerpc/include/asm/siginfo.h
@@ -15,11 +15,6 @@
 
 #include <asm-generic/siginfo.h>
 
-/*
- * SIGTRAP si_codes
- */
-#define TRAP_BRANCH	(__SI_FAULT|3)	/* process taken branch trap */
-#define TRAP_HWBKPT	(__SI_FAULT|4)	/* hardware breakpoint or watchpoint */
 #undef NSIGTRAP
 #define NSIGTRAP	4
 
Index: ...
From: Ingo Molnar
Date: Monday, September 22, 2008 - 3:42 am

no fundamental objections - assuming existing x86 apps have not grown an 
ABI dependency on the existing send_sigtrap() semantics. (Debuggers and 
JITs would be a candidate for such dependencies.)


should be pushed into [a sufficiently extended] send_sigtrap() instead.


should be separated into a helper function as well i guess.

Roland, any objections to the core idea (or to the implementation)?

	Ingo
--

From: Srinivasa Ds
Date: Monday, September 22, 2008 - 6:11 am

Assuming that no ABI dependency exist between x86 apps and send_sigtrap(),
And implementing some of the Ingo's suggestions, Iam resending the patch.

Still waiting for Roland's reply.


Currently a SIGTRAP can denote any one of below reasons.
	- Breakpoint hit
	- H/W debug register hit
	- Single step
	- Signal sent through kill() or rasie()

Architectures like powerpc/parisc provides infrastructure to demultiplex
SIGTRAP signal by passing down the information for receiving SIGTRAP through
si_code of siginfot_t strucutre. Here is an attempt is generalise this 
infrasturcutre by extending it to x86 and x86_64 archs. 

Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com>

---
 arch/ia64/include/asm/siginfo.h    |    5 -----
 arch/powerpc/include/asm/siginfo.h |    5 -----
 arch/x86/kernel/ptrace.c           |    7 ++++---
 arch/x86/kernel/traps_32.c         |    4 +++-
 arch/x86/kernel/traps_64.c         |    2 +-
 include/asm-generic/siginfo.h      |    2 ++
 include/asm-parisc/siginfo.h       |    5 -----
 include/asm-x86/ptrace.h           |    2 +-
 include/asm-x86/traps.h            |   10 ++++++++++
 9 files changed, 21 insertions(+), 21 deletions(-)

Index: linux-2.6.27-rc7/arch/ia64/include/asm/siginfo.h
===================================================================
--- linux-2.6.27-rc7.orig/arch/ia64/include/asm/siginfo.h
+++ linux-2.6.27-rc7/arch/ia64/include/asm/siginfo.h
@@ -113,11 +113,6 @@ typedef struct siginfo {
 #undef NSIGSEGV
 #define NSIGSEGV	3
 
-/*
- * SIGTRAP si_codes
- */
-#define TRAP_BRANCH	(__SI_FAULT|3)	/* process taken branch trap */
-#define TRAP_HWBKPT	(__SI_FAULT|4)	/* hardware breakpoint or watchpoint */
 #undef NSIGTRAP
 #define NSIGTRAP	4
 
Index: linux-2.6.27-rc7/arch/powerpc/include/asm/siginfo.h
===================================================================
--- linux-2.6.27-rc7.orig/arch/powerpc/include/asm/siginfo.h
+++ linux-2.6.27-rc7/arch/powerpc/include/asm/siginfo.h
@@ -15,11 +15,6 @@
 
 #include ...
From: Ingo Molnar
Date: Monday, September 22, 2008 - 7:54 am

please do not send patches that modify include/asm/ files, the 
include/asm-x86/ file should be modified instead.

(this problem will go away in v2.6.28 when we'll move include/asm-x86/ 






please declare inline functions explicitly as 'static inline'.

	Ingo
--

From: Srinivasa Ds
Date: Tuesday, September 23, 2008 - 2:53 am

Ingo, Sorry if I have confused you. Arch specific header files of ia64 and powerpc are 
already moved to arch/ia64/include and arch/powerpc/include dirs.
So I have developed patch against these files. So Resending patch again. 

Currently a SIGTRAP can denote any one of below reasons.
	- Breakpoint hit
	- H/W debug register hit
	- Single step
	- Signal sent through kill() or rasie()

Architectures like powerpc/parisc provides infrastructure to demultiplex
SIGTRAP signal by passing down the information for receiving SIGTRAP through
si_code of siginfot_t structure. Here is an attempt is generalise this 
infrastructure by extending it to x86 and x86_64 archs. 

Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com>


---
 arch/ia64/include/asm/siginfo.h    |    5 -----
 arch/powerpc/include/asm/siginfo.h |    5 -----
 arch/x86/kernel/ptrace.c           |    7 ++++---
 arch/x86/kernel/traps_32.c         |    4 +++-
 arch/x86/kernel/traps_64.c         |    2 +-
 include/asm-generic/siginfo.h      |    2 ++
 include/asm-parisc/siginfo.h       |    5 -----
 include/asm-x86/ptrace.h           |    2 +-
 include/asm-x86/traps.h            |   10 ++++++++++
 9 files changed, 21 insertions(+), 21 deletions(-)

Index: linux-2.6.27-rc7/arch/ia64/include/asm/siginfo.h
===================================================================
--- linux-2.6.27-rc7.orig/arch/ia64/include/asm/siginfo.h
+++ linux-2.6.27-rc7/arch/ia64/include/asm/siginfo.h
@@ -113,11 +113,6 @@ typedef struct siginfo {
 #undef NSIGSEGV
 #define NSIGSEGV	3
 
-/*
- * SIGTRAP si_codes
- */
-#define TRAP_BRANCH	(__SI_FAULT|3)	/* process taken branch trap */
-#define TRAP_HWBKPT	(__SI_FAULT|4)	/* hardware breakpoint or watchpoint */
 #undef NSIGTRAP
 #define NSIGTRAP	4
 
Index: linux-2.6.27-rc7/arch/powerpc/include/asm/siginfo.h
===================================================================
--- linux-2.6.27-rc7.orig/arch/powerpc/include/asm/siginfo.h
+++ ...
From: Ingo Molnar
Date: Tuesday, September 23, 2008 - 4:28 am

applied to [the new topic] tip/core/signal, thanks Srinivasa! There are 
some other pending x86 signal changes already, so i based 
tip/core/signal on tip/x86/signal.

Roland, any opinion on this change?

	Ingo
--

From: Ingo Molnar
Date: Tuesday, September 23, 2008 - 4:30 am

-tip testing found the following build error with the attached config:

In file included from arch/x86/mm/fault.c:41:
include/asm/traps.h: In function 
From: Srinivasa Ds
Date: Tuesday, September 23, 2008 - 7:25 am

Ingo, Reproduced build break issue with your config on tip tree. It was a costly overlook 
to miss one header file. I included it in this patch and tested it out.


Currently a SIGTRAP can denote any one of below reasons.
	- Breakpoint hit
	- H/W debug register hit
	- Single step
	- Signal sent through kill() or rasie()

Architectures like powerpc/parisc provides infrastructure to demultiplex
SIGTRAP signal by passing down the information for receiving SIGTRAP through
si_code of siginfot_t structure. Here is an attempt is generalise this 
infrastructure by extending it to x86 and x86_64 archs. 

Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com>


---
 arch/ia64/include/asm/siginfo.h    |    5 -----
 arch/powerpc/include/asm/siginfo.h |    5 -----
 arch/x86/kernel/ptrace.c           |    7 ++++---
 arch/x86/kernel/traps_32.c         |    4 +++-
 arch/x86/kernel/traps_64.c         |    2 +-
 include/asm-generic/siginfo.h      |    2 ++
 include/asm-parisc/siginfo.h       |    5 -----
 include/asm-x86/ptrace.h           |    2 +-
 include/asm-x86/traps.h            |   12 ++++++++++++
 9 files changed, 23 insertions(+), 21 deletions(-)

Index: linux-2.6.27-rc7/arch/ia64/include/asm/siginfo.h
===================================================================
--- linux-2.6.27-rc7.orig/arch/ia64/include/asm/siginfo.h
+++ linux-2.6.27-rc7/arch/ia64/include/asm/siginfo.h
@@ -113,11 +113,6 @@ typedef struct siginfo {
 #undef NSIGSEGV
 #define NSIGSEGV	3
 
-/*
- * SIGTRAP si_codes
- */
-#define TRAP_BRANCH	(__SI_FAULT|3)	/* process taken branch trap */
-#define TRAP_HWBKPT	(__SI_FAULT|4)	/* hardware breakpoint or watchpoint */
 #undef NSIGTRAP
 #define NSIGTRAP	4
 
Index: linux-2.6.27-rc7/arch/powerpc/include/asm/siginfo.h
===================================================================
--- linux-2.6.27-rc7.orig/arch/powerpc/include/asm/siginfo.h
+++ linux-2.6.27-rc7/arch/powerpc/include/asm/siginfo.h
@@ -15,11 +15,6 @@
 
 #include <asm-generic/siginfo.h>
 ...
From: Ingo Molnar
Date: Tuesday, September 23, 2008 - 7:31 am

thanks - applied the delta fix below to tip/core/signal.

	Ingo

--------------->
From e8d3f455de4f42d4bab2f6f1aeb2cf3bd18eb508 Mon Sep 17 00:00:00 2001
From: Srinivasa Ds <srinivasa@in.ibm.com>
Date: Tue, 23 Sep 2008 15:23:52 +0530
Subject: [PATCH] signals: demultiplexing SIGTRAP signal, fix

fix build breakage, missing header file.

Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/asm-x86/traps.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/asm-x86/traps.h b/include/asm-x86/traps.h
index 4b1e904..7a692ba 100644
--- a/include/asm-x86/traps.h
+++ b/include/asm-x86/traps.h
@@ -1,6 +1,8 @@
 #ifndef ASM_X86__TRAPS_H
 #define ASM_X86__TRAPS_H
 
+#include <asm/debugreg.h>
+
 /* Common in X86_32 and X86_64 */
 asmlinkage void divide_error(void);
 asmlinkage void debug(void);
--

From: Roland McGrath
Date: Friday, September 26, 2008 - 2:06 am

I certainly have no objection in principle.  I doubt that any x86 userland
apps expect certain si_code values for SIGTRAP now, since the existing
values are not of any real use.  (Signal handlers get the thread.trap_no and
thread.error_code values from hardware to guess from, and debuggers via
ptrace get the hardware %db6 value to guess from.)  I do have a few comments.

If you're doing it, I think you should do the do_int3 case too,
so every machine-generated SIGTRAP has a meaningful si_code value.

The only use of send_sigtrap is for do_debug (and for faking that do_debug
happened in the syscall_trace_leave case).  You should consolidate all the
uses in both 32 and 64 to use send_sigtrap uniformly, change its signature
as needed.  I'm inclined to consolidate the si_code logic there, and just
pass it the hardware bits or let it get them from the thread_struct
(trap_nr, error_code, debugreg6).

About that si_code logic based on %db6.  There are some funny "sticky"
properties to how that register gets set in hardware.  Even reading the
hardware manuals doesn't always make it plain what to expect.  I wouldn't
want to testify that the patch's logic is correct in distinguishing which
event really just happened.  (I'm not sure, but I think it may also be
possible to have a single do_debug trap for both a single-step trap and a
hardware breakpoint trap generated by the same instruction.)  I know that
Alan Stern figured out a lot of the magic empirically a while back.  That
deserves a careful double-checking if we are now trying to make si_code
tell a clear and reliable story.


Thanks,
Roland
--

From: Srinivasa DS
Date: Monday, September 29, 2008 - 6:34 am

Roland

That sounds like a good idea. Let me go through code and get back to you.

Thanks
  Srinivasa DS

--

From: Gabriel Paubert
Date: Tuesday, September 23, 2008 - 8:53 am

Typo: s/rasie/raise/

No strong opinion about the patch, but more info is usually better.

	Regards,
	Gabriel
--

Previous thread: [PATCH] x86: do_boot_cpu - add check if we have ESR register by Cyrill Gorcunov on Monday, September 22, 2008 - 2:36 am. (8 messages)

Next thread: [PATCH 0/13] memory cgroup updates v4 by KAMEZAWA Hiroyuki on Monday, September 22, 2008 - 3:51 am. (37 messages)