"next-property" reloaded !

[Olivier Danet]

Hello !
I have tried to recreate the 3 known variants of next-property:

Standard :         a) true "string" b) false   c) false
Sun Ultra10 :      a) -1 "string"   b) nothing c) nothing
PowerPC iBook :    a) -1 "string"   b) -1 0 0  c) 0

Here is a tentative patch :

Index: forth/admin/devices.fs
===================================================================
--- forth/admin/devices.fs    (révision 1269)
+++ forth/admin/devices.fs    (copie de travail)
@@ -451,7 +451,7 @@
    ?active-package dup >r if
      0 0
      begin
-      r@ next-property
+      r@ next-property-std
      while
        cr 2dup dup -rot type
        begin ."  " 1+ dup d# 26 >= until drop
Index: forth/device/property.fs
===================================================================
--- forth/device/property.fs    (révision 1269)
+++ forth/device/property.fs    (copie de travail)
@@ -63,14 +63,14 @@
    ;

\ From package (5.3.4.1)
-: next-property
+: next-property-std
  ( previous-str previous-len phandle -- false | name-str name-len true )
    >r
    2dup 0= swap 0= or if
      2drop r> >dn.properties @
    else
      r> find-property dup if @ then
-    ?dup if >prop.next @ then
+    dup if >prop.next @ then
    then

?dup if
@@ -81,7 +81,57 @@
    then
  ;

+: next-property-ppc
+( previous-str previous-len phandle -- false | name-str name-len true | 
0 0 true )
+  >r
+  2dup 0= swap 0= or if
+    2drop r> >dn.properties @ true
+  else
+    r> find-property dup if @ then
+    dup if >prop.next @ true then
+  then

+  if
+    dup if
+      >prop.name @ dup cstrlen
+      ( phandle name-str name-len )
+    else
+      0
+      ( 0 0 )
+    then
+    true
+  else
+    false
+  then
+;
+
+: next-property-std-bis
+  next-property-ppc
+  dup
+  if over 0=
+    if
+      2drop
+    then
+  then
+;
+
+: next-property-s64
+  next-property-std
+  ?dup drop
+;
+
+: next-property
+  [IFDEF] CONFIG_PPC
+  next-property-ppc
+  [ELSE]
+  [IFDEF] CONFIG_SPARC64
+  next-property-s64
+  [ELSE]
+  next-property-std
+  [THEN]
+  [THEN]
+;
+
  \
  \ 5.3.5.4 Property value access
  \
Index: forth/system/ciface.fs
===================================================================
--- forth/system/ciface.fs    (révision 1269)
+++ forth/system/ciface.fs    (copie de travail)
@@ -121,7 +121,7 @@

( buf prev prev_len )

-  r> next-property if
+  r> next-property-std if
      ( buf name name_len )
      dup 1+ -rot ci-strcpy drop 1
    else
===================================================================

there are :
next-property-std = standard, used by Sparc32 "romvec" C code and by 
forth code, like ".properties"
next-property-s64 = Sparc64 version, built from next-property-std
next-property-ppc = PowerPC version
next-property-std-bis = Alternate standard version, built from the PPC 
one (e.g. when compiling for PPC target)

Tests :

showstack
cd screen

\ Sparc32
" "  ?active-package next-property-std cr . type cr
" name" ?active-package next-property-std cr . type cr
" interrupts" ?active-package next-property-std cr . cr
" ttttt" ?active-package next-property-std cr . cr

\ PowerPC
" "  ?active-package next-property-ppc cr . type cr
" name" ?active-package next-property-ppc cr . type cr
" interrupts" ?active-package next-property-ppc cr .  . . cr
" ttttt" ?active-package next-property-ppc cr . cr

\ Sparc64
" "  ?active-package next-property-s64 cr . type cr
" name" ?active-package next-property-s64 cr . type cr
" interrupts" ?active-package next-property-s64 cr
" ttttt" ?active-package next-property-s64 cr

" "  ?active-package next-property-std-bis cr . type cr
" name" ?active-package next-property-std-bis cr . type cr
" interrupts" ?active-package next-property-std-bis cr . cr
" ttttt" ?active-package next-property-std-bis cr . cr

Currently all variants are available simultaneously.
Additional [IFDEF] can be used to hide some variants according
to compilation options, generate the -std from the -ppc,..

One more oddity:
NetBSD is not the only one to do strange things with properties.
In OpenBSD, one can find :
arch/sparc/dev/sbus.c:int sbus_testdma(sc, ca)
...
   getpropint(0, "slave-only", 0)
...
arch/sparc/dev/sbus.c:int sbus_print(args, sbus)
...
   getpropint(0, sl, 0)
...
Here, 0 should select the 'root' node.
It really looks like a bug in OpenBSD.
It does not prevent the OS from running though.

Olivier

[Mark Cave-Ayland]

On 18/02/14 01:08, Olivier Danet wrote:
> Hello !
> I have tried to recreate the 3 known variants of next-property:
>
> Standard : a) true "string" b) false c) false
> Sun Ultra10 : a) -1 "string" b) nothing c) nothing
> PowerPC iBook : a) -1 "string" b) -1 0 0 c) 0
>
> Here is a tentative patch :
> Index: forth/admin/devices.fs
>
> --- forth/admin/devices.fs (révision 1269)
> +++ forth/admin/devices.fs (copie de travail)
> @@ -451,7 +451,7 @@
> ?active-package dup >r if
> 0 0
> begin
> r@ next-property
> r@ next-property-std
>
> while
> cr 2dup dup -rot type
> begin ." " 1+ dup d# 26 >= until drop
> Index: forth/device/property.fs
> ===================================================================
> --- forth/device/property.fs (révision 1269)
> +++ forth/device/property.fs (copie de travail)
> @@ -63,14 +63,14 @@
> ;
>
> \ From package (5.3.4.1)
> -: next-property
> +: next-property-std
> ( previous-str previous-len phandle -- false | name-str name-len true )
> > r
>
> 2dup 0= swap 0= or if
> 2drop r> >dn.properties @
> else
> r> find-property dup if @ then
> ?dup if >prop.next @ then
> dup if >prop.next @ then
>
> then
>
> ?dup if
> @@ -81,7 +81,57 @@
> then
> ;
>
> +: next-property-ppc
> +( previous-str previous-len phandle -- false | name-str name-len true |
> 0 0 true )
> > r
>
> 2dup 0= swap 0= or if
>
> 2drop r> >dn.properties @ true
>
> else
>
> r> find-property dup if @ then
>
> dup if >prop.next @ true then
>
> then
>
> if
>
> dup if
> > prop.name @ dup cstrlen
>
> ( phandle name-str name-len )
>
> else
>
> 0
>
> ( 0 0 )
>
> then
>
> true
>
> else
>
> false
>
> then
>
> +;
>
> +: next-property-std-bis
> next-property-ppc
> dup
> if over 0=
> if
> 2drop
> then
> then
>
> +;
>
> +: next-property-s64
> next-property-std
> ?dup drop
>
> +;
>
> +: next-property
> [IFDEF] CONFIG_PPC
> next-property-ppc
> [ELSE]
> [IFDEF] CONFIG_SPARC64
> next-property-s64
> [ELSE]
> next-property-std
> [THEN]
> [THEN]
>
> +;
>
> \
> \ 5.3.5.4 Property value access
> \
> Index: forth/system/ciface.fs
> ===================================================================
> --- forth/system/ciface.fs (révision 1269)
> +++ forth/system/ciface.fs (copie de travail)
> @@ -121,7 +121,7 @@
>
> ( buf prev prev_len )
> r> next-property if
> r> next-property-std if
>
> ( buf name name_len )
> dup 1+ -rot ci-strcpy drop 1
> else
> ===================================================================
>
> there are :
> next-property-std = standard, used by Sparc32 "romvec" C code and by
> forth code, like ".properties"
> next-property-s64 = Sparc64 version, built from next-property-std
> next-property-ppc = PowerPC version
> next-property-std-bis = Alternate standard version, built from the PPC
> one (e.g. when compiling for PPC target)
>
> Tests :
>
> showstack
> cd screen
>
> \ Sparc32
> " " ?active-package next-property-std cr . type cr
> " name" ?active-package next-property-std cr . type cr
> " interrupts" ?active-package next-property-std cr . cr
> " ttttt" ?active-package next-property-std cr . cr
>
> \ PowerPC
> " " ?active-package next-property-ppc cr . type cr
> " name" ?active-package next-property-ppc cr . type cr
> " interrupts" ?active-package next-property-ppc cr . . . cr
> " ttttt" ?active-package next-property-ppc cr . cr
>
> \ Sparc64
> " " ?active-package next-property-s64 cr . type cr
> " name" ?active-package next-property-s64 cr . type cr
> " interrupts" ?active-package next-property-s64 cr
> " ttttt" ?active-package next-property-s64 cr
>
> " " ?active-package next-property-std-bis cr . type cr
> " name" ?active-package next-property-std-bis cr . type cr
> " interrupts" ?active-package next-property-std-bis cr . cr
> " ttttt" ?active-package next-property-std-bis cr . cr
>
> Currently all variants are available simultaneously.
> Additional [IFDEF] can be used to hide some variants according
> to compilation options, generate the -std from the -ppc,..
>
> One more oddity:
> NetBSD is not the only one to do strange things with properties.
> In OpenBSD, one can find :
> arch/sparc/dev/sbus.c:int sbus_testdma(sc, ca)
> ...
> getpropint(0, "slave-only", 0)
> ...
> arch/sparc/dev/sbus.c:int sbus_print(args, sbus)
> ...
> getpropint(0, sl, 0)
> ...
> Here, 0 should select the 'root' node.
> It really looks like a bug in OpenBSD.
> It does not prevent the OS from running though.
>
> Olivier

Hi Olivier,

Thanks for doing this patch, and apologies for the delay in getting back 
to you as it has given me quite a few things to think about!

My current thoughts at the moment are that the reason these particular 
differences in next-property have only just been discovered is because 
it is extremely unlikely that anyone would call it directly. So as we 
discovered from Zoltan, PPC/SPARC64 would call next-property via the 
client interface, and we know that SPARC32 would call next-property via 
the romvec block.

I guess the only way these bugs could be encountered would be if someone 
evaluated a Forth string containing next-property directly, and why 
would they bother if OpenBIOS already provides a method to do this via 
CIF/romvec?

Therefore what I would suggest is that we hold on this patch for the 
moment and instead could you provide an alternative that fixes the 
return from next-property in obp_nextprop instead? That way we have the 
results of your research preserved in the archives in case we need to 
rethink this later, plus the patch itself is much simpler making it much 
less likely to affect other platforms.

ATB,

Mark.

[Olivier Danet]

On 03/03/2014 22:12, Mark Cave-Ayland wrote:
> On 18/02/14 01:08, Olivier Danet wrote:
> > Hello !
> > I have tried to recreate the 3 known variants of next-property:
> >
> > Standard : a) true "string" b) false c) false
> > Sun Ultra10 : a) -1 "string" b) nothing c) nothing
> > PowerPC iBook : a) -1 "string" b) -1 0 0 c) 0
> >
> > Here is a tentative patch :
> > Index: forth/admin/devices.fs
> >
> > --- forth/admin/devices.fs (révision 1269)
> > +++ forth/admin/devices.fs (copie de travail)
> > @@ -451,7 +451,7 @@
> > ?active-package dup >r if
> > 0 0
> > begin
> > r@ next-property
> > r@ next-property-std
> >
> > while
> > cr 2dup dup -rot type
> > begin ." " 1+ dup d# 26 >= until drop
> > Index: forth/device/property.fs
> > ===================================================================
> > --- forth/device/property.fs (révision 1269)
> > +++ forth/device/property.fs (copie de travail)
> > @@ -63,14 +63,14 @@
> > ;
> >
> > \ From package (5.3.4.1)
> > -: next-property
> > +: next-property-std
> > ( previous-str previous-len phandle -- false | name-str name-len true )
> > > r
> >
> > 2dup 0= swap 0= or if
> > 2drop r> >dn.properties @
> > else
> > r> find-property dup if @ then
> > ?dup if >prop.next @ then
> > dup if >prop.next @ then
> >
> > then
> >
> > ?dup if
> > @@ -81,7 +81,57 @@
> > then
> > ;
> >
> > +: next-property-ppc
> > +( previous-str previous-len phandle -- false | name-str name-len true |
> > 0 0 true )
> > > r
> >
> > 2dup 0= swap 0= or if
> >
> > 2drop r> >dn.properties @ true
> >
> > else
> >
> > r> find-property dup if @ then
> >
> > dup if >prop.next @ true then
> >
> > then
> >
> > if
> >
> > dup if
> > > prop.name @ dup cstrlen
> >
> > ( phandle name-str name-len )
> >
> > else
> >
> > 0
> >
> > ( 0 0 )
> >
> > then
> >
> > true
> >
> > else
> >
> > false
> >
> > then
> >
> > +;
> >
> > +: next-property-std-bis
> > next-property-ppc
> > dup
> > if over 0=
> > if
> > 2drop
> > then
> > then
> >
> > +;
> >
> > +: next-property-s64
> > next-property-std
> > ?dup drop
> >
> > +;
> >
> > +: next-property
> > [IFDEF] CONFIG_PPC
> > next-property-ppc
> > [ELSE]
> > [IFDEF] CONFIG_SPARC64
> > next-property-s64
> > [ELSE]
> > next-property-std
> > [THEN]
> > [THEN]
> >
> > +;
> >
> > \
> > \ 5.3.5.4 Property value access
> > \
> > Index: forth/system/ciface.fs
> > ===================================================================
> > --- forth/system/ciface.fs (révision 1269)
> > +++ forth/system/ciface.fs (copie de travail)
> > @@ -121,7 +121,7 @@
> >
> > ( buf prev prev_len )
> > r> next-property if
> > r> next-property-std if
> >
> > ( buf name name_len )
> > dup 1+ -rot ci-strcpy drop 1
> > else
> > ===================================================================
> >
> > there are :
> > next-property-std = standard, used by Sparc32 "romvec" C code and by
> > forth code, like ".properties"
> > next-property-s64 = Sparc64 version, built from next-property-std
> > next-property-ppc = PowerPC version
> > next-property-std-bis = Alternate standard version, built from the PPC
> > one (e.g. when compiling for PPC target)
> >
> > Tests :
> >
> > showstack
> > cd screen
> >
> > \ Sparc32
> > " " ?active-package next-property-std cr . type cr
> > " name" ?active-package next-property-std cr . type cr
> > " interrupts" ?active-package next-property-std cr . cr
> > " ttttt" ?active-package next-property-std cr . cr
> >
> > \ PowerPC
> > " " ?active-package next-property-ppc cr . type cr
> > " name" ?active-package next-property-ppc cr . type cr
> > " interrupts" ?active-package next-property-ppc cr . . . cr
> > " ttttt" ?active-package next-property-ppc cr . cr
> >
> > \ Sparc64
> > " " ?active-package next-property-s64 cr . type cr
> > " name" ?active-package next-property-s64 cr . type cr
> > " interrupts" ?active-package next-property-s64 cr
> > " ttttt" ?active-package next-property-s64 cr
> >
> > " " ?active-package next-property-std-bis cr . type cr
> > " name" ?active-package next-property-std-bis cr . type cr
> > " interrupts" ?active-package next-property-std-bis cr . cr
> > " ttttt" ?active-package next-property-std-bis cr . cr
> >
> > Currently all variants are available simultaneously.
> > Additional [IFDEF] can be used to hide some variants according
> > to compilation options, generate the -std from the -ppc,..
> >
> > One more oddity:
> > NetBSD is not the only one to do strange things with properties.
> > In OpenBSD, one can find :
> > arch/sparc/dev/sbus.c:int sbus_testdma(sc, ca)
> > ...
> > getpropint(0, "slave-only", 0)
> > ...
> > arch/sparc/dev/sbus.c:int sbus_print(args, sbus)
> > ...
> > getpropint(0, sl, 0)
> > ...
> > Here, 0 should select the 'root' node.
> > It really looks like a bug in OpenBSD.
> > It does not prevent the OS from running though.
> >
> > Olivier
>
> Hi Olivier,
>
> Thanks for doing this patch, and apologies for the delay in getting 
> back to you as it has given me quite a few things to think about!
>
> My current thoughts at the moment are that the reason these particular 
> differences in next-property have only just been discovered is because 
> it is extremely unlikely that anyone would call it directly. So as we 
> discovered from Zoltan, PPC/SPARC64 would call next-property via the 
> client interface, and we know that SPARC32 would call next-property 
> via the romvec block.
>
> I guess the only way these bugs could be encountered would be if 
> someone evaluated a Forth string containing next-property directly, 
> and why would they bother if OpenBIOS already provides a method to do 
> this via CIF/romvec?
>
> Therefore what I would suggest is that we hold on this patch for the 
> moment and instead could you provide an alternative that fixes the 
> return from next-property in obp_nextprop instead? That way we have 
> the results of your research preserved in the archives in case we need 
> to rethink this later, plus the patch itself is much simpler making it 
> much less likely to affect other platforms.
>
> ATB,
>
> Mark.

Just push the one-liner Forth patch I submitted two months ago...

Regards.
Olivier

[Mark Cave-Ayland]

On 04/03/14 20:57, Olivier Danet wrote:
> Just push the one-liner Forth patch I submitted two months ago...

Okay... I think I understand this a bit better now. According to grep 
there are only 2 Forth references to next-property in the source tree:

forth/admin/devices.fs
   forth/system/ciface.fs

Now we know next-property in ciface.fs can never be called with a 
non-existent property, since the check further up the code will detect 
this first and exit.

Similarly the loop in devices.fs (.properties) starts from the first 
property in the package and loops through them in turn, so again it is 
impossible to call next-property with a non-existent property here.

On this basis, given that the patch causes no regressions in my SPARC32 
test images I've applied it to SVN trunk - thanks for the patch, and 
apologies for the delay in getting it applied.

ATB,

Mark.

[Olivier Danet]

On 05/03/2014 01:03, Mark Cave-Ayland wrote:
> On 04/03/14 20:57, Olivier Danet wrote:
> > Just push the one-liner Forth patch I submitted two months ago...
>
> Okay... I think I understand this a bit better now. According to grep 
> there are only 2 Forth references to next-property in the source tree:
>
> forth/admin/devices.fs
>   forth/system/ciface.fs
>
> Now we know next-property in ciface.fs can never be called with a 
> non-existent property, since the check further up the code will detect 
> this first and exit.
>
> Similarly the loop in devices.fs (.properties) starts from the first 
> property in the package and loops through them in turn, so again it is 
> impossible to call next-property with a non-existent property here.
>
> On this basis, given that the patch causes no regressions in my 
> SPARC32 test images I've applied it to SVN trunk - thanks for the 
> patch, and apologies for the delay in getting it applied.
>
> ATB,
>
> Mark.

Never mind.
And thank you for your hard work !

Olivier.