• d-bugmail puremagic.com Apr 01 2008
• d-bugmail puremagic.com Apr 01 2008
• Fawzi Mohamed <fmohamed mac.com> Apr 06 2008

http://d.puremagic.com/issues/show_bug.cgi?id=1968

           Summary: boxer.d does not work
           Product: DGCC aka GDC
           Version: unspecified
          Platform: Macintosh
        OS/Version: Mac OS X
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Phobos
        AssignedTo: dvdfrdmn users.sf.net
        ReportedBy: fawzi gmx.ch


Doing my tangobos merge I came across a bug in std.boxer.
Actually in phobos it passes the unit tests, but is still broken.
The issue can be seen with the following program
==========
import std.stdio;
import std.boxer;
void main()
{
   auto array=boxArray("ab","cd","ef");
   assert(unbox!(char[])(array[0])=="ab");
   assert(unbox!(char[])(array[1])=="cd");
   assert(unbox!(char[])(array[2])=="ef");
   writefln(unbox!(char[])(array[2]));
}
========

The main issue is that the generic handling does not update _argptr.
 doing a modification like this

-               array[index] = box(ti_orig, cast(void*) _argptr);
+            void *v=cast(void*)_argptr;
+            array[index] = box(ti_orig, v);
+            // update _argptr
+            v+=ti.tsize;
+            void **vv=cast(void**)&_argptr;
+            *vv=v;

fixes the problem, at least on MacOsX.
Still this is a dangerous operation because it makes assumptions on _argptr
(that from now on the arguments are on the stack and a pointer to them is the
first element in va_list).
So I also added to the function a lot of special cases (all basic types, arrays
of them and all pointer types) in the dumb way (by typing them out).
I think that this way it should be rather safe, and work in most occasions.

Here is the patch wit a slight change in the import and an extra cast.
--- boxer.d     (revision 209)
+++ boxer.d     (working copy)
   -68,10 +68,9   
 module std.boxer;

 private import std.format;
+import std.stdarg;
 private import std.string;
 private import std.utf;
-version (GNU)
-    private import std.stdarg;

  /* These functions and types allow packing objects into generic containers
   * and recovering them later.  This comes into play in a wide spectrum of
   -502,7 +501,7   

  /**
   * Box each argument passed to the function, returning an array of boxes.
-  */    
+  */
 Box[] boxArray(...)
 {
     version (GNU)
   -517,28 +516,128   
            while ( (ttd = cast(TypeInfo_Typedef) ti) !is null )
                ti = ttd.base;

-           if (ti is typeid(float))
+           if (ti is typeid(bool))
            {
-               float f = va_arg!(float)(_argptr);
-               array[index] = box(ti_orig, cast(void *) & f);
+               auto a = va_arg!(bool)(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
            }
            else if (ti is typeid(char) || ti is typeid(byte) || ti is
typeid(ubyte))
            {
-               byte b = va_arg!(byte)(_argptr);
-               array[index] = box(ti_orig, cast(void *) & b);
+               auto a = va_arg!(byte)(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
            }
-           else if (ti is typeid(wchar) || ti is typeid(short) || ti is
typeid(ushort))
+           else if (ti is typeid(wchar))
            {
-               short s = va_arg!(short)(_argptr);
-               array[index] = box(ti_orig, cast(void *) & s);
+               auto a = va_arg!(wchar)(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
            }
-           else if (ti is typeid(bool))
+           else if (ti is typeid(short) || ti is typeid(ushort))
            {
-               bool b = va_arg!(bool)(_argptr);
-               array[index] = box(ti_orig, cast(void *) & b);
+               auto a = va_arg!(short)(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
            }
-           else
-               array[index] = box(ti_orig, cast(void*) _argptr);
+           else if (ti is typeid(int) || ti is typeid(uint))
+           {
+               auto a = va_arg!(int)(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(long) || ti is typeid(ulong))
+           {
+               auto a = va_arg!(long)(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(float) || ti is typeid(ifloat))
+           {
+               auto a = va_arg!(float)(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(double) || ti is typeid(idouble))
+           {
+               auto a = va_arg!(double)(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(real) || ti is typeid(ireal))
+           {
+               auto a = va_arg!(real)(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(bool[]))
+           {
+               auto a = va_arg!(bool[])(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(char[]) || ti is typeid(byte[]) || ti is
typeid(ubyte[]))
+           {
+               auto a = va_arg!(byte[])(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(wchar[]))
+           {
+               auto a = va_arg!(wchar[])(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(short[]) || ti is typeid(ushort[]))
+           {
+               auto a = va_arg!(short[])(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(int[]) || ti is typeid(uint[]))
+           {
+               auto a = va_arg!(int[])(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(long[]) || ti is typeid(ulong[]))
+           {
+               auto a = va_arg!(long[])(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(float[]) || ti is typeid(ifloat[]))
+           {
+               auto a = va_arg!(float[])(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(double[]) || ti is typeid(idouble[]))
+           {
+               auto a = va_arg!(double[])(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(real[]) || ti is typeid(ireal[]))
+           {
+               auto a = va_arg!(real[])(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(cfloat[]))
+           {
+               auto a = va_arg!(cfloat[])(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(cdouble[]))
+           {
+               auto a = va_arg!(cdouble[])(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti is typeid(creal[]))
+           {
+               auto a = va_arg!(creal[])(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+           }
+           else if (ti.tsize == (void *).sizeof)
+           { // treats all pointer based cases
+               void *a = va_arg!(void *)(_argptr);
+               array[index] = box(ti_orig, cast(void *) & a);
+        }
+        else
+        {
+            // structures passed on the stack
+            // assumes a stack based storage, and that the pointer to it
+            // can be retrived via cast...
+            void *v=cast(void*)_argptr;
+            array[index] = box(ti_orig, v);
+            // update _argptr
+            v+=ti.tsize;
+            void **vv=cast(void**)&_argptr; // this is dangerous...
+            *vv=v; // ... very dangerous... but seems to work ;)
+           }
        }

        return array;
   -797,7 +896,7   
         if (isArrayTypeInfo(value.type))
             return (*cast(void[]*) value.data).ptr;
         if (cast(TypeInfo_Class) value.type)
-            return *cast(Object*) value.data;
+            return cast(T)(*cast(Object*) value.data);

         throw new UnboxException(value, typeid(T));
     }


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=1968





------- Comment #1 from fawzi gmx.ch  2008-04-01 10:46 -------
I do not know if the update of _argptr
+            v+=ti.tsize;
should take into account alignement issues.
Probably it should...

Fawzi


-- 

I had posted into digitalmars.D the proposal to introduce the function

  void *dyn_va_arg(in TypeInfo ti,inout va_list argptr)

along with the template va_arg.
Such function would clear up the mess in boxer.d, and I think that such 
a function would be nice also for other things.
After having read the gcc ABI on AMD64 where it is made clear that the 
treatment of the va_arg depends only on the size, I implemented 
dyn_va_arg for sizes up to 42 bytes (easily increasable) using that 
assumption.
I think that such a function (maybe extended to all possible cases) 
should go in std.stdarg, but for the moment I have modified std.boxer 
and put it there.

Indeed the code now is much better, I think that one could also avoid 
the transformation to the base type, but for now I have left it in.

I also modified the unit test with a more stringent test (one that 
would fail with the previous version).

===================================================================
--- boxer.d     (revision 209)
+++ boxer.d     (working copy)
   -68,10 +68,9   
 module std.boxer;

 private import std.format;
+import std.stdarg;
 private import std.string;
 private import std.utf;
-version (GNU)
-    private import std.stdarg;

  /* These functions and types allow packing objects into generic containers
   * and recovering them later.  This comes into play in a wide spectrum of
   -500,9 +499,47   
     return array;
 }

+/+
+ + Template to get arbitrary structures out of va_arg
+ + assumes that all argument of the same size are treated in the same way
+ +/
+void *dyn_va_argT(int s1,int s2)(in TypeInfo ti,inout va_list argptr){
+    static if (s1 <= s2){
+        if (ti.tsize == s1)
+        {
+            struct ms{byte[s1] mse;};
+               return cast(void *)&va_arg!(ms)(argptr);
+        }
+        return dyn_va_argT!(s1+1,s2)(ti,argptr);
+    }
+    else{
+        char[10] str,str2;
+        throw new Exception("hit max size of dyn_va_argT ("~format(str,s2)~
+            "), ti.tsize="~format(str2,ti.tsize));
+        // if you hit this then either increase the getSizes s2 limit or
+        // hope that the follwing hack works
+        //
+        // assumes a stack based storage, and that the pointer to it
+        // can be retrived via cast...
+        void *v=cast(void*)argptr;
+        void **vv=cast(void**)&argptr;
+        *vv=v+ti.tsize;
+        return v;
+    }
+}
+
+/+
+ + Function that return a pointer to the current argument of the va_list
+ + and updates the va_list.
+ + assumes that all argument of the same size are treated in the same way
+ +/
+void *dyn_va_arg(in TypeInfo ti,inout va_list argptr){
+    return dyn_va_argT!(1,42)(ti,argptr);
+}
+
  /**
   * Box each argument passed to the function, returning an array of boxes.
-  */
+  */
 Box[] boxArray(...)
 {
     version (GNU)
   -516,29 +553,9   

            while ( (ttd = cast(TypeInfo_Typedef) ti) !is null )
                ti = ttd.base;
-
-           if (ti is typeid(float))
-           {
-               float f = va_arg!(float)(_argptr);
-               array[index] = box(ti_orig, cast(void *) & f);
-           }
-           else if (ti is typeid(char) || ti is typeid(byte) || ti is 
typeid(ubyte))
-           {
-               byte b = va_arg!(byte)(_argptr);
-               array[index] = box(ti_orig, cast(void *) & b);
-           }
-           else if (ti is typeid(wchar) || ti is typeid(short) || ti 
is typeid(ushort))
-           {
-               short s = va_arg!(short)(_argptr);
-               array[index] = box(ti_orig, cast(void *) & s);
-           }
-           else if (ti is typeid(bool))
-           {
-               bool b = va_arg!(bool)(_argptr);
-               array[index] = box(ti_orig, cast(void *) & b);
-           }
-           else
-               array[index] = box(ti_orig, cast(void*) _argptr);
+
+           void *p=dyn_va_arg(ti,_argptr);
+           array[index] = box(ti, p);
        }

        return array;
   -797,7 +814,7   
         if (isArrayTypeInfo(value.type))
             return (*cast(void[]*) value.data).ptr;
         if (cast(TypeInfo_Class) value.type)
-            return *cast(Object*) value.data;
+            return cast(T)(*cast(Object*) value.data);

         throw new UnboxException(value, typeid(T));
     }
   -891,13 +908,13   
     assert(unboxTest!(ireal)(box(45i)) == 45i);

     /* Create an array of boxes from arguments. */
-    Box[] array = boxArray(16, "foobar", new Object);
+    Box[] array = boxArray(new Object, "foobar",16);

     assert(array.length == 3);
-    assert(unboxTest!(int)(array[0]) == 16);
+    assert(unboxTest!(Object)(array[0]) !is null);
     assert(unboxTest!(char[])(array[1]) == "foobar");
-    assert(unboxTest!(Object)(array[2]) !is null);
-
+    assert(unboxTest!(int)(array[2]) == 16);
+
     /* Convert the box array back into arguments. */
     TypeInfo[] array_types;
     void* array_data;
===================================================================