Re: [Sbcl-devel] [patch] hppa stepping-bugfix and aver-cleanup

[Larry V. <re...@us...>]

> > 0x514a390c:     stw r13,4(r4)
> > 0x514a3910:     copy r5,r26
> > 0x514a3914:     stw r26,3c(r4)  r6=0x4e00000b , $r6+91=0x4e000066
> > VOP save-dynamic-state
> > 0x514a3918:     ldw 91(r6),r14            r14=0
> > 0x514a391c:     break a,0     <--- sigill's, inserted in define-vop
> > to hook up gdb
> > 0x514a3920:     copy sp,r16
> > (end of vop, returning in register r14)
> > 0x514a3924:     stw r14,38(r4)
> > 0x514a3928:     stw r15,14(r4)
> > 0x514a392c:     stw r16,10(r4)
> > 0x514a3930:     copy r3,r26
> > 0x514a3934:     stw r26,40(r4)
> > 0x514a3938:     addi 20,r4,r25
> > 0x514a393c:     ldw a9(r6),r14
> > 0x514a3940:     break b,0
> > ...
> >
> > 0x4e000020:   0x0000053e    0x4e000027    0x79abf308    0x4e00000b
> > 0x4e000030:   0x50000017    0x4e00000b    0x0000053e    0x0000004a
> > 0x4e000040:   0x00000000    0x4e00000b    0x50000027    0x4e00000b
> > 0x4e000050: w 0x0000053e  v 0x0000004a  h 0x00000000  p 0x4e00000b
> > 0x4e000060: n 0x5000003f pa 0x4e00000b    0x0000053e    0x4b0005b0
> > 0x4e000070:   0x00000000    0x4e00000b    0x50000057    0x4e00000b
> > 0x4e000080:   0x0000053e    0x4e000530    0x00000000    0x4e00000b
> > 0x4e000090:   0x5000007f    0x4e00000b    0x0000053e    0x00000000
> >
> > w = widetag, 53e: symbol structure
> > v = value
> > h = hash
> > p = plist
> > n = name:  x/4xs 0x5000003f => 0x5000003f: "4*CORE-STRING*"
> > pa=package
> >
> > We load return value (reg r14) with 0x4e000066 that points to the
> > wrong symbol and slot (should be symbol *current-catch-block* and
> > value slot).
> >
> > It seems ppc, sparc, mips, alpha and hppa all computes
> > load-symbol-value the same way.. can they really all be wrong ?
> > sound more that I'm wrong then. But again hppa above showes clearly
> > that hppa does it wrongly.
> >
> > best regards,
> > /larry
>
> A new patch that fixes the bugs in nlx.lisp and cleansup call.lisp
> regz, /larry
>
>
> diff --git a/src/compiler/hppa/call.lisp
> b/src/compiler/hppa/call.lisp index 1edc572..db0771e 100644
> --- a/src/compiler/hppa/call.lisp
> +++ b/src/compiler/hppa/call.lisp
> @@ -774,13 +774,8 @@ default-value-8
>                  (insert-step-instrumenting (callable-tn)
>                    ;; Conditionally insert a conditional trap:
>                    (when step-instrumenting
> -                    ;; Get the symbol-value of SB!IMPL::*STEPPING*
> -                    (loadw stepping null-tn
> -                           (+ symbol-value-slot
> -                              (truncate (static-symbol-offset
> 'sb!impl::*stepping*)
> -                                        n-word-bytes))
> -                           other-pointer-lowtag)
> -                    ;; If it's not NIL, trap.
> +                    (load-symbol-value stepping sb!impl::*stepping*)
> +                    ;; If symbol-value is not NIL, trap.
>                      ;(inst comb := stepping null-tn step-done-label)
>                      (inst comb := null-tn null-tn step-done-label
>
> :nullify t)
>
>                      ;; CONTEXT-PC will be pointing here when the
> diff --git a/src/compiler/hppa/macros.lisp
> b/src/compiler/hppa/macros.lisp index 0a5e991..6349953 100644
> --- a/src/compiler/hppa/macros.lisp
> +++ b/src/compiler/hppa/macros.lisp
> @@ -50,11 +50,11 @@
>             (inst ldo offset null-tn ,reg :unsigned t))))))
>
>  (defmacro load-symbol-value (reg symbol)
> -  `(inst ldw
> -         (+ (static-symbol-offset ',symbol)
> -            (ash symbol-value-slot word-shift)
> -            (- other-pointer-lowtag))
> -         null-tn ,reg))
> +  `(loadw ,reg null-tn
> +          (+ symbol-value-slot
> +             (truncate (static-symbol-offset ',symbol)
> +                       n-word-bytes))
> +          other-pointer-lowtag))
>
>  (defmacro store-symbol-value (reg symbol)
>    `(inst stw ,reg (+ (static-symbol-offset ',symbol)

-As far as I can tell the two new load-symbol-value definition is
-equivalent. Let's look at the
-macroexpansion of (load-symbol-value stepping sb!impl:*stepping*):
-
-Old:
-(INST LW STEPPING NULL-TN
-        (+ (STATIC-SYMBOL-OFFSET '*STEPPING*) (ASH SYMBOL-VALUE-SLOT
-WORD-SHIFT)
-           (- OTHER-POINTER-LOWTAG)))
-
-New:
-(INST LW STEPPING NULL-TN
-        (-
-         (ASH
-          (+ SYMBOL-VALUE-SLOT
-             (TRUNCATE (STATIC-SYMBOL-OFFSET '*STEPPING*) -N-WORD-BYTES))
-          2)
-         OTHER-POINTER-LOWTAG))
-
-The old definition (not only the macroexpansion) is more similar to
-code for other
-architectures and seriously IMHO, cleaner. Otherwise, they are the
-same. Is there a failure
-related to NLXs that this patch fixes?


Strange this mail didn't reach my box, some problems with the maillist ?
 Saw it on gmane..

Gabor, thanks for taking a look at this.

You are correct there is no difference. That made me curious why I then
got that bug in nlx.lisp. Well it was really silly, i added "91" to $r6
where I should have added "0x99"  :)

That reduces the lower half of the patch to an reverse-cosmetic :)
The upper half still applies as a cosmetic cleanup.

Having that sorted out I'm still wondering if mips had a bug at
call.lisp:787. Unfortunately I haven't had time to bootup
qemu yet.

cheers,
/larry