[PATCH] src/gc/system.c compile on HPUX

[rrauenza]

I'm not sure what to do with this code -- it looked incomplete to begin with -- why is getcontext() called and nothing done with it? And how do I test it?

Anyway, it wouldn't compile on HPUX and I realized the \_\_ia64\_\_ section must be for a linux on ia64 compile. So I reimplemented the code using inline assembly (I was also getting an undefined symbol for the flush_reg_store() function supposedly implemented in assembly somewhere else) and a more robust way of getting the bottom of the rsestack.

I also added couple of headers to gc_private.h for the pstat call and the inline assembly.

The inline assembly won't work under gcc. HP C defines \_\_HP\_cc if we want to have a separate gcc implementation. (I had to escape the underscores here!)

Index: src/gc/system.c
===================================================================
--- src/gc/system.c     (revision 38492)
+++ src/gc/system.c     (working copy)
@@ -93,6 +93,34 @@
 
 #elif defined(__ia64__)
 
+
+#if defined(__hpux)
+       /* This is untested, but should be functionally equivalent to the ia64 
+          code below, which must be IA64 Linux code?  It did not compile on HPUX. */
+        ucontext_t ucp;
+        void *current_regstore_top;
+
+        getcontext(&ucp);
+        _Asm_flushrs();
+
+#if defined(_LP64)
+        current_regstore_top = (void*)(uint64_t)_Asm_mov_from_ar(_AREG_BSP);
+#else
+        current_regstore_top = (void*)(uint32_t)_Asm_mov_from_ar(_AREG_BSP);
+#endif
+
+        size_t base = 0;
+        struct pst_vm_status buf;
+        int i = 0;
+
+        while(pstat_getprocvm (&buf, sizeof(buf), 0, i++) == 1) {
+           if (buf.pst_type == PS_RSESTACK) {
+              base = (size_t)buf.pst_vaddr;
+              break;
+           }
+        }
+
+#else /* is this code for ia64 linux?  This doesn't seem portable.  Is this better? http://www.mail-archive.com/guile-devel@gnu.org/msg01299.html */
         /* On IA64 systems, we use the function getcontext() to get the current
            processor context. This function is located in <ucontext.h>, included
            above. */
@@ -110,8 +138,13 @@
            is separate from the normal system stack. The register backing
            stack starts at memory address 0x80000FFF80000000 and ends at
            current_regstore_top. */
-        trace_mem_block(interp, 0x80000fff80000000,
+        size_t base = 0x80000fff80000000;
+
+#endif
+
+        trace_mem_block(interp, base, 
                 (size_t)current_regstore_top);
+
 #else
 
 #  ifdef PARROT_HAS_HEADER_SETJMP
Index: src/gc/gc_private.h
===================================================================
--- src/gc/gc_private.h (revision 38492)
+++ src/gc/gc_private.h (working copy)
@@ -29,8 +29,13 @@
 extern void *flush_reg_store(void);
 #  define BACKING_STORE_BASE 0x80000fff80000000
 
+#ifdef __hpux
+#include <sys/pstat.h>
+#include <ia64/sys/inline.h>
 #endif
 
+#endif
+
 /* We're using this here to add an additional pointer to a PObj without
    having to actually add an entire pointer to every PObj-alike structure
    in Parrot. Astute observers may notice that if the PObj is comprised of

[jkeenan]

Replying to rrauenza:

Could you post the output you got before you applied the patch? It would be helpful to see the failures you got.

Thank you very much. kid51

[rrauenza]

smoketst@hairball.cup.hp.com $ make
Compiling with:
xx.c
cc -I./include -Ae -D_HPUX_SOURCE -Wl,+vnocompatwarnings -I/usr/local/include -D_LARGEFILE_SOURCE -g +Z -I. -o xx.o -c xx.c
src/gc/api.c
src/gc/mark_sweep.c
src/gc/system.c
"src/gc/system.c", line 99: error #2070: incomplete type is not allowed
          struct ucontext ucp;
                          ^

"src/gc/system.c", line 113: warning #2069-D: integer conversion resulted in
          truncation
          trace_mem_block(interp, 0x80000fff80000000,
                                  ^

"src/gc/system.c", line 207: warning #2940-D: missing return statement at end
          of non-void function "find_common_mask"
  }
  ^

1 error detected in the compilation of "src/gc/system.c".
make: *** [src/gc/system.o] Error 2

[Infinoid]

Hi,

I have lots of comments and questions on this patch. Sorry if it's a little disorganized.

This code is rather confusing, because it plays various tricks in order to get direct access to the register set and processor stack(s) so it can scan them for PMC pointers and do mark & sweep GC.

I'm not a GC expert, but I think it calls getcontext() in order to flush the system registers onto the stack. (IA64/PA-RISC has register windows, so they can't all be scanned directly.) Then the stack is scanned for pointers to PMCs/PObjs and thus begins the mark & sweep process. So the result *is* used, even if there's no more visible use of the variable itself. (I do worry about the call being optimized out, it's hard to tell either way.)

Testing the GC isn't terribly easy. If you can build and get through the test suite, I'd say that's a good start. Other parroteers might have more specific testing ideas.

Are the sys/pstat.h and ia64/sys/inline.h headers available (and useful) on all platforms HPUX runs on?

I think you're right, the IA64 code in that file must have been specifically for linux. I can update the comments accordingly, when this patch goes in.

We have fairly poor testing on non-core platforms, and are always looking for ways to extend our test coverage. Once you're able to build and test parrot, would you be willing to set up a cron job to submit periodic test reports? Please see https://trac.parrot.org/parrot/wiki/SmokingParrot for some details.

Thanks!

Mark

[whiteknight]

Infinoid: I think you're right about your analysis of this function. At the least, it's the same conclusion that I came to. getcontext() should be returning the processor register set into a variable on the call stack for later scanning by the GC. I also share your concern that this variable is likely to get optimized away. I don't have an HPUX platform to test on, but I strongly suspect this is the case.

I'm in the middle of some GC-related cleanups and the mess in src/gc/system.c is high up on my priorities list. One potential quick-fix solution would be to mark the variable as "volatile", although the compiler might complain that marking a local variable like that as volatile is nonsensical. I've got some better, more in-depth ideas for fixing this permanently, but if this is giving a problem the "volatile" keyword might just make it go away.

Anyway, that's me going off on a tangent. The GC is getting all cleaned up soon so don't worry about these issues unless they are causing a major problem.

[rrauenza]

Yes, <sys/pstat.h> and <ia64/sys/inline.h> are standard on IA64 HPUX. PA wouldn't have the latter, but PA is obsolete, and the code is already in #if _ia64

I am indeed interested in smoking parrot (heh!), which is the whole point of my trying to get this to compile.

If getcontext() is a function call, I doubt the call is optimized away.

Should I try to smoke rakudo or parrot -- or both? Are started with rakudo.

I did run make test, btw, and only about 88 tests failed, if I recall!

Rich

[rrauenza]

When will my three patches be pushed into svn? I'll try a parrot smoke at that time (I thought my arch.pm was already committed?)

Also, I notice parrot smoke doesn't report build errors. What's the best way to deal with/report that?

Rich

[Infinoid]

Replying to whiteknight:

Anyway, that's me going off on a tangent. The GC is getting all cleaned up soon so don't worry about these issues unless they are causing a major problem.

Well, they're preventing builds on HPUX, so I'll apply the patch for now and let you fix things up this weekend as you see fit.

Replying to rrauenza:

When will my three patches be pushed into svn? I'll try a parrot smoke at that time (I thought my arch.pm was already committed?)

#647 should be worked around in a different way, I'm hoping that's no longer preventing you from building. I'll apply this #648 patch (with some rewritten comments) momentarily, as it sounds like they very likely improve the situation for you and looks like they won't break anything else. It looks like your #645 was applied by NotFound a couple of days ago (r38512). That's everything, right?

Also, I notice parrot smoke doesn't report build errors. What's the best way to deal with/report that?

The trac tickets you've been posting have been great, I'd say just keep that up. Thanks!

Mark

[Infinoid]

Thanks, applied (with minor changes) as r38613.

[rrauenza]

You're right -- the arch.pm did get done. But it still fails. I'll investigate. (Ah! The substitution is in the wrong place. I'll update the bug.. if I can. it is closed.)

The problem with not reporting compilation fails through smoke, is then I have to monitor the builds for the smokes. I saw some references to buildbots, so I can look into that.

Rich