Download Reference Manual
The Developer's Library for D
About Wiki Forums Source Search Contact

Ticket #1042 (new defect)

Opened 10 years ago

Last modified 10 years ago

Improve Layout argv handling with gdc

Reported by: fawzi Assigned to: larsivi
Priority: major Milestone: External Bugs
Component: Core Functionality Version: trunk
Keywords: Cc:

Description

tango.text.convert.Layout convert floating point types with gdc uses just some fixed sizes. I needed to output muarray that are structs of size 32, so I rewrote that part of the code in a more flexible way, and added a couple of tests more.

By the way does is the statement

                        static if (is(typeof(args.ptr)))
                            arglist[i] = args.ptr;
                        else ....

correct ? is args.ptr automatically updated without any action, without calling va_arg does the compiler do so much magic and keep arg in foreach synchronized with args??? (that branch is not executed on my architecture).

anyway here is my patch

Fawzi

===================================================================
--- Layout.d    (revision 3426)
+++ Layout.d    (working copy)
@@ -57,7 +57,50 @@
         equivalents of each argument.
 
 *******************************************************************************/
+version(GNU) {
+/+
+ + Template to get arbitrary structures out of va_arg
+ + assumes that all argument of the same size are treated in the same way
+ +/
+private const int maxStore=64;
+private struct StoreEl{byte[maxStore] st;};
 
+private void *dyn_get_argT(int s1,int s2)(in TypeInfo ti,inout va_list argptr,out StoreEl el){
+    const int m = (s1+s2)/2;
+    static if (s1 < s2){
+        if (ti.tsize > m) {
+            return dyn_get_argT!(m+1,s2)(ti,argptr,el);
+        } else {
+            return dyn_get_argT!(s1,m)(ti,argptr,el);
+        }
+    }
+    else{
+        if (ti.tsize == m)
+        {
+            struct ms{byte[m] mse;};
+            ms *st=cast(ms*)&el;
+               *st=va_arg!(ms)(argptr);
+               return cast(void *)st;
+        } else {
+            throw new Exception("hit max size of dyn_va_argT");
+            // 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;
+        }
+    }
+}
+private void *dyn_get_arg(in TypeInfo ti,inout va_list argptr,out StoreEl el){
+    return dyn_get_argT!(1,maxStore)(ti,argptr,el);
+}
+
+}
+
 class Layout(T)
 {
         public alias convert opCall;
@@ -184,15 +227,11 @@
         version (GNU)
                 {
                 Arg[64] arglist = void;
-                int[64] intargs = void;
-                byte[64] byteargs = void;
-                long[64] longargs = void;
-                short[64] shortargs = void;
-                void[][64] voidargs = void;
                 real[64] realargs = void;
                 float[64] floatargs = void;
                 double[64] doubleargs = void;
-
+                StoreEl[64] bigargs = void;
+                
                 foreach (i, arg; arguments)
                         {
                         static if (is(typeof(args.ptr)))
@@ -228,31 +267,8 @@
                                 }
                         if (! converted)
                            {
-                           switch (arg.tsize)
-                                  {
-                                  case 1:
-                                       byteargs[i] = va_arg!(byte)(args);
-                                       arglist[i] = &byteargs[i];
-                                       break;
-                                  case 2:
-                                       shortargs[i] = va_arg!(short)(args);
-                                       arglist[i] = &shortargs[i];
-                                       break;
-                                  case 4:
-                                       intargs[i] = va_arg!(int)(args);
-                                       arglist[i] = &intargs[i];
-                                       break;
-                                  case 8:
-                                       longargs[i] = va_arg!(long)(args);
-                                       arglist[i] = &longargs[i];
-                                       break;
-                                  case 16:
-                                       voidargs[i] = va_arg!(void[])(args);
-                                       arglist[i] = &voidargs[i];
-                                       break;
-                                  default:
-                                       assert (false, "Unknown size: " ~ Integer.toString (arg.tsize));
-                                  }
+                               dyn_get_arg(arg,args,bigargs[i]);
+                               arglist[i] = & bigargs[i];
                            }
                         }
                 }
@@ -814,6 +830,8 @@
         assert( Formatter( "d{{{0}d", "<string>" ) == "d{<string>d");
         assert( Formatter( "d{0}}d", "<string>" ) == "d<string>}d");
 
+        assert( Formatter( "{} {} {} {0:x}",1.3f,cast(bool)1,1.4,cast(bool)1, 0xafe0000 ) == "1.30 true 1.40 true afe0000" );
+        assert( Formatter( "{} {} {} {0:x}",0.0f,cast(bool)1,0.0,cast(bool)1, 0xafe0000 ) == "0.00 true 0.00 true afe0000" );
         assert( Formatter( "{0:x}", 0xafe0000 ) == "afe0000" );
         // todo: is it correct to print 7 instead of 6 chars???
         assert( Formatter( "{0:x7}", 0xafe0000 ) == "afe0000" );
@@ -886,6 +904,8 @@
         f[ 1.0 ] = "one".dup;
         f[ 3.14 ] = "PI".dup;
         assert( Formatter( "{}", f ) == "{ 1.00=>one, 3.14=>PI }" );
+        
+        
         }
 }
 

Attachments

patch.1 (8.9 kB) - added by fawzi on 04/21/08 15:34:58.
slightly cleaned up patch

Change History

04/11/08 17:25:23 changed by sean

  • owner changed from sean to kris.

04/11/08 18:13:06 changed by fvbommel

Doesn't compile on x86-64:

<...>/include/d/4.1.3/tango/text/convert/Layout.d:98: Error: cannot have out or ref parameter of type __va_list_tag [1LU]

(gdc (GCC) 4.1.3 20070831 (prerelease gdc 0.25, using dmd 1.021) (Ubuntu 0.25-4.1.2-16ubuntu1))

That "static if (is(typeof(args.ptr)))" was added on my suggestion. On x86-64 GDC typeof(args) is an array type, which is the reason for both that static if and for the error message I pasted above. (See also #1029 and the commits associated with it)

04/11/08 19:02:02 changed by kris

  • owner changed from kris to larsivi.

04/12/08 11:31:27 changed by fawzi

using "ref va_list" instead of "inout va_list" should fix the issue whan va_list is an array. I still strongly suspect that

                            arglist[i] = args.ptr;

is wrong for all but the first argument.

04/12/08 13:37:04 changed by larsivi

Some previous tests suggests that the static if may be relevant for most 64 bit architectures.

As for the patch, could you please explain the purpose better? Does your usecase work with DMD out of the box?

04/12/08 16:57:16 changed by fvbommel

fawzi: ref and inout are, AFAIK, equivalent in all cases. Either way, the compiler error is exactly the same if I change inout to ref.

And the old code (including arglist[i] = args.ptr;) works fine even for > 1 argument. If you suspect otherwise I suggest you obtain access to an x86_64 machine or emulator and try to prove it.

04/17/08 07:43:11 changed by fawzi

Indeed the code arglist[i] = args.ptr; is wrong for all arguments but the first (I tested on AMD64), the whole routine works because the code after the if is still executed and overwrites the value.

Here is patch that works on both X86-64 and darwin, the idea is to replace the fixed sizes that where checked in the previous version with a template that checks up to a easy changeable size, now set to 64 bytes.

This way I will be able to print again arrays of the multiarray library that have a size of 32 bytes (the old code accepted just 1,2,4,8,16 as sizes, and the size 16 was wrong depending of the architecture (void[]).sizeof is not 16 on all architectures, it can be also 8.

04/17/08 14:08:21 changed by fvbommel

That version seems to work better. And better struct support is pretty nice. And yes, after some more testing the current svn version does seem to be broken; structs with floating-point values in them don't work properly, for instance. (I guess I didn't notice this earlier because I hardly ever use floats)

However, the same holds for small structs in your version:

import tango.io.Stdout;
import tango.text.convert.Format;

struct T {
    double d = 2;
    char[] toString() {
        return Format.convert("{{{}}", d);
    }
}

void main() {
    T test;
    Stdout.formatln("{}", test.sizeof);
    Stdout.formatln("{}\n{}\n{}", test, test, test);
}

Output (with the new patch):

8
{0.24e-309}
{0.00}
{0.24e-309}

(The problem is that floating-point fields of such structs are passed in xmm* registers)

That said, your code is better for some otherwise unsupported struct types (specifically, > (void[]).sizeof without floating-point values). It's just that GDC varargs are a mess because it uses the C vararg ABI.

P.S.: You may want to reconsider putting your code between Layout and its doc-comment :P.

04/18/08 15:22:50 changed by larsivi

  • milestone changed from 0.99.6 to External Bugs.

04/21/08 11:43:02 changed by fawzi

I gave a look at variadic templates, as these don't have the bugs of var_arg. I put my findings here as it might be relevant for some possible future refactorings.

It is easy to create a wrapper that uses variadic templates, with something like this

template Nargs(){
    const int Nargs=0;
}
template Nargs(T,U...){
    const int Nargs=1+Nargs!(U);
}
template mf(U...){
void mf(U args){
    const int nargs=Nargs!(U);
    void *[nargs] aPtrs;
    TypeInfo[nargs] aTis;
    foreach(i,a;args){
        aPtrs[i]=cast(void*)&a;
        aTis[i]=typeid(U[i]);
    }
    return printArgs(aPtrs,aTis);
}
}

This is all stack based, and has no limit on the number of arguments. A convert call can be converted to this. What is more difficult is to have different functions based on the first arguments. Actually using static ifs I managed to handle the different inputs, but not the different return types.

I think that at the moment with the current interface (all public) it isn't possible. The heavy overloading makes it difficult, on should use at least two names: convert and convertSync.

Fawzi

04/21/08 15:34:58 changed by fawzi

  • attachment patch.1 added.

slightly cleaned up patch