[Gcl-devel] Re: siginfo fault addresses now apprently working on hppa

[Camm Maguire]

Greetings!

Carlos O'Donell <address@hidden> writes:

> On Sat, Feb 07, 2004 at 01:42:13PM -0500, Camm Maguire wrote:
> > Greetings!  GCL and its dependent programs (axiom, maxima, acl2) can
> > make use of a more efficient garbage collection algorithm by marking
> > pages read only, catching SIGSEGV on write attempt, reprotecting and
> > marking the page, and then restarting.  This has not worked on the
> > past on hppa as the fault address could not (at least to my knowledge)
> > be recovered from the siginfo_t struct in the handler.  Now apparently
> > si_ptr is correctly set, though si_addr, which the man page says
> > should have the fault address (and does on many other platforms) does
> > not.
> > 
> > Can I now reliably depend on this behavior and configure GCL on hppa
> > to take advantage of this optimization?
> 
> Cam,
> 
> Thanks for your work on GCL. I've recently submitted some siginfo_t

Thanks.  Hope its useful.

> fixes upstream, so I'll take a stab at enumerating your options.
> 
> What works and what doesn't work:
> =================================
> 
> a. siginfo_t is properly delivered to 32-bit userspace from a 32-bit
>    kernel. If it doesn't, please show me the example code, kernel config,
>    and hardware used to run the test. This has always worked.
> 

Well, I'm not sure how long is meant by always, but lets forget about
the past for the moment.  Right now on paer I can get the address in

 ((siginfo_t *)code)->si_ptr

where the handler is defined as

void
memprotect_handler(int sig, long code, void *scp, char *addr)

and setup with SA_RESTART and SA_SIGINFO.  On most machines its under 

 ((siginfo_t *)code)->si_addr

but not on paer.  I can show you a gdb trace if you need.


> b. siginfo_t is *not* delivered properly to a 32-bit userspace from 
>    a 64-bit 2.4 kernel. This has never worked.
> 
> c. siginfo_t is *correctly* delivered to a 32-bit userspace from a 
>    64-bit 2.6 kernel. This started working in December when I put
>    the fixes into cvs.parisc-linux.org. These fixes are going upstream.
> 
> To fix this for 2.4 would require backporting my compat_siginfo_t
> patches, along with the rt_sigframe compat, and jejb's 64-bit process
> changes (it's a lot of stuff). I currently don't have time for this :( 
> if another hppa developer would like to do the backport I'd be happy to 
> review.
> 
> In summary, you'd have to do atleast two runtime checks, perhaps
> something like the following (though I don't quit know what to use
> here):
> 
>       o If `uname -a` returns parisc64, and the kernel is 
>           2.6.0 or greater, then you're golden, else disable
>         looking at siginfo_t information.
> 
>       o If `uname -a` returns parisc, then it has always worked.
>         Enable looking at siginfo_t information.
> 
> How you handle this is up to you :)

Thanks for this jog.  This has been on my todo list for sometime now.
I've just implemented a runtime test for the fault recovery
mechanism.  I'm not checking kernel versions per se, just the compile
time determined code snippet for recovering the fault address on the
runtime kernel.  Here's the outline:

=============================================================================
Index: o/main.c
===================================================================
RCS file: /cvsroot/gcl/gcl/o/main.c,v
retrieving revision 1.26.4.1.2.6
diff -u -b -r1.26.4.1.2.6 main.c
--- o/main.c    12 Nov 2003 15:41:35 -0000      1.26.4.1.2.6
+++ o/main.c    12 Feb 2004 02:57:17 -0000
@@ -284,6 +284,9 @@
 #endif 
 #endif 
        
+#ifdef SGC
+       memprotect_test_reset();
+#endif
 
        if (initflag) {
                if (saving_system) {
Index: o/sgbc.c
===================================================================
RCS file: /cvsroot/gcl/gcl/o/sgbc.c,v
retrieving revision 1.9.4.1.2.3
diff -u -b -r1.9.4.1.2.3 sgbc.c
--- o/sgbc.c    12 Nov 2003 15:41:36 -0000      1.9.4.1.2.3
+++ o/sgbc.c    12 Feb 2004 02:57:17 -0000
@@ -1035,6 +1035,135 @@
 
 #endif   
 
+typedef enum {memprotect_none,memprotect_cannot_protect,memprotect_sigaction,
+             memprotect_bad_return,memprotect_no_signal,
+             memprotect_multiple_invocations,memprotect_no_restart,
+             memprotect_bad_fault_address,memprotect_success} memprotect_enum;
+static memprotect_enum memprotect_result;
+static int memprotect_handler_invocations,memprotect_print_enable;
+static void *memprotect_test_address;
+
+#define MEM_ERR_CASE(a_) \
+  case a_: \
+    fprintf(stderr,"The SGC segfault recovery test failed with %s, SGC 
disabled\n",#a_); \
+    break
+
+static void
+memprotect_print(void) {
+
+  if (!memprotect_print_enable)
+    return;
+
+  switch(memprotect_result) {
+  case memprotect_none: case memprotect_success:
+    break;
+
+    MEM_ERR_CASE(memprotect_cannot_protect);
+    MEM_ERR_CASE(memprotect_sigaction);
+    MEM_ERR_CASE(memprotect_bad_return);
+    MEM_ERR_CASE(memprotect_no_signal);
+    MEM_ERR_CASE(memprotect_no_restart);
+    MEM_ERR_CASE(memprotect_bad_fault_address);
+    MEM_ERR_CASE(memprotect_multiple_invocations);
+
+  }
+
+}
+
+
+static void
+memprotect_handler_test(int sig, long code, void *scp, char *addr) {
+
+  char *faddr;
+  faddr=GET_FAULT_ADDR(sig,code,scp,addr); 
+
+  if (memprotect_handler_invocations) {
+    memprotect_result=memprotect_multiple_invocations;
+    exit(-1);
+  }
+  memprotect_handler_invocations=1;
+  if (faddr!=memprotect_test_address)
+    memprotect_result=memprotect_bad_fault_address;
+  else
+    memprotect_result=memprotect_none;
+  mprotect(memprotect_test_address,PAGESIZE,PROT_READ|PROT_WRITE);
+
+}
+
+static int
+memprotect_test(void) {
+
+  char b1[2*PAGESIZE],b2[PAGESIZE];
+  struct sigaction sa,sao;
+
+  if (memprotect_result!=memprotect_none)
+    return memprotect_result!=memprotect_success;
+  if (atexit(memprotect_print)) {
+    fprintf(stderr,"Cannot setup memprotect_print on exit\n");
+    exit(-1);
+  }
+
+  memset(b1,32,sizeof(b1));
+  memset(b2,0,sizeof(b2));
+  memprotect_test_address=(void *)(((unsigned long)b1+PAGESIZE-1) & 
~(PAGESIZE-1));
+  if (mprotect(memprotect_test_address,PAGESIZE,PROT_READ)) {
+    memprotect_result=memprotect_cannot_protect;
+    return -1;
+  }
+  sa.sa_sigaction=(void *)memprotect_handler_test;
+  sa.sa_flags=MPROTECT_ACTION_FLAGS;
+  if (sigaction(SIGSEGV,&sa,&sao)) {
+    memprotect_result=memprotect_sigaction;
+    return -1;
+  }
+  memprotect_result=memprotect_bad_return;
+  memset(memprotect_test_address,0,PAGESIZE);
+  if (memprotect_result==memprotect_bad_return)
+    memprotect_result=memprotect_no_signal;
+  if (memprotect_result!=memprotect_none) {
+    sigaction(SIGSEGV,&sao,NULL);
+    return -1;
+  }
+  if (memcmp(memprotect_test_address,b2,PAGESIZE)) {
+    memprotect_result=memprotect_no_restart;
+    sigaction(SIGSEGV,&sao,NULL);
+    return -1;
+  }
+  memprotect_result=memprotect_success;
+  sigaction(SIGSEGV,&sao,NULL);
+  return 0;
+
+}
+
+static int
+do_memprotect_test(void) {
+
+  int rc=0;
+
+  memprotect_print_enable=1;
+  if (memprotect_test()) {
+    memprotect_print();
+    if (sgc_enabled)
+      sgc_quit();
+    rc=-1;
+  }
+  memprotect_print_enable=0;
+  return rc;
+
+}
+
+void
+memprotect_test_reset(void) {
+
+  memprotect_result=memprotect_none;
+  memprotect_handler_invocations=0;
+  memprotect_test_address=NULL;
+
+  if (sgc_enabled)
+    do_memprotect_test();
+
+}
+
 int
 sgc_start(void) {
 
@@ -1044,6 +1173,10 @@
   object f;
   struct typemanager *tm;
   int npages;
+
+  if (memprotect_result!=memprotect_success && do_memprotect_test())
+    return 0;
+
   if (sgc_type_map[page((&sgc_type_map[0]))] != SGC_PERM_WRITABLE)
     perm_writable(&sgc_type_map[0],sizeof(sgc_type_map));
   if (sgc_enabled)
@@ -1567,7 +1700,8 @@
   check_arg(1);
   if(vs_base[0]==Cnil) 
     sgc_quit();
-  else sgc_start();
+  else 
+    vs_base[0]=sgc_start() ? Ct : Cnil;
 }
 
 /* make permanently writable pages containing pointers p thru p+n-1 */
=============================================================================

We'll see how it goes in the just uploaded 2.6.1-24.

Take care,


> 
> c.
> 
> 
> 
> 

-- 
Camm Maguire                                            address@hidden
==========================================================================
"The earth is but one country, and mankind its citizens."  --  Baha'u'llah