DMD 1.047

Daniel Green avatarDaniel Green created an issue

Here are a series of patches to update GDC to DMD 1.047. Phobos has been compiled but no executables have been ran/generated.

dmd1047_rev88.patch - Line ending patch, optional but I'm not sure the others will merge without the patch or the line endings being unified.

dmd1047_rev89.patch - Patches the frontend.

dmd1047_rev90.patch - Patches phobos

dmd1047_rev91.patch - DMD changes some variables to use const. This patch will change method signatures to be const void* and then drop it when saving into the dt_t structure.

There were 4 files that had no GDC equivalent; glue.c, s2ir.c, tocvdebug.c, dwarf.c. The license was not listed as being for personal use but I believe they are mostly backend related.

I also commented out some code in statement.c that should be reviewed by someone who knows more. There are two sections containing "#if 0 // DMD v1.047 needs reviewing".

Comments (16)

  1. Daniel Green

    Sorry for not being clear. I simply meant that I haven't tested the compiler outside of building phobos.

    Although there is a bug with foreach statements when using ref/inout attributes.

    The following code will cause a segfault in the compiler.

    foreach(ref entry; internalUnicodeData) 
        unicodeData[entry.code] = &entry; 
    

    and

    foreach(ref value; array)
        printf("%d\n", value);
    

    will cause a segfault in when attempting to print value.

  2. Daniel Green

    The following bit of code will print out the the value of the array as the address of var.

    int array[] = [0,1,2,3,4,5,6,7,8,9];
    foreach(ref var; array)
        printf("%d\n", &var);
    
  3. Daniel Green

    About the ref bug I mentioned earlier, what I believe the problem to be is that GDC will not correctly generate the address of a ref variable in a foreach statement.

    I think the best way to explain it is with some D code.

    // This is what it does.
    int array[] = [0,1,2,3];
    int* var = array[i];
    
    // This is what is should do.
    int array[] = [0,1,2,3];
    int* var = &array[i];
    

    I spent some time on that conclusion and tried to trace down the code path used by GDC to generate a foreach statement. If I knew when this bug appeared, I would like to try to trace down why it happens but that might just be a side effect of the new front end turning foreach into for.

    In statement.d around line 1508 you'll see how it assigns values to foreach arguments. Later on the ExpInitializer is used in d-codegen.cc:78 to initialize a value to be stored inside a variable. I will attach a simple patch that fixes the issue, but I doubt is the best way to go about it but it successfully solved the issue. I will do a full recompile to see if it breaks anything.

    Here is a list of my ideas as to why this issue occurs. Since I do not know much about the internals of GDC this list are my best guesses based the work I did today.

    • GDC does not mark reference types. typeof(ref int) == typeof(int).
    • It's also possible that the front end will not correctly change a ref into a pointer to make GDC notice that it is a reference type, but such a change should be noticeable when comparing to DMD code as I believe type setting is all done in the frontend.

    PS, I'd like to look into some form of continuous integration system so that GDC changes can be verified automatically and was wondering if anyone else had ideas on that.

  4. Iain Buclaw
    class PrefData
    {
      GradeData[] gradeData;
    
      public this() {
        gradeData = new GradeData[1];
        foreach (inout GradeData gd; gradeData)
          gd = new GradeData;  // segfault happens here.
      }
    }
    
    class GradeData {}
    
    void main()
    {
        PrefData p = new PrefData();
    }
    

    I'm going to rest now, but can you test this code too once you have recompiled.

    This is another regression that has cropped up, another segfault during runtime.

    Regards

  5. Iain Buclaw

    OK, managed to have a look, and the cause is in statement.c, around line 1475.

    If the comments aren't obvious enough, what the frontend now does is convert foreach statements into for statements, so when the GDC glue calls ::toIR to generate the tree, it is infact calling ForStatement::toIR, not ForeachStatement::toIR.

    The fix you've got doesn't look too wrong to me. I might be picky and say that you should instead use:

    if (init_val && v->isRef() || v->isOut())
    

    as the condition.

    What could be a better idea though is to hash out a boolean value to tell the GDC backend that the ForStatement it is compiling infact is a remixed ForeachStatement.

    Regards

  6. Daniel Green

    Picky is good, I only used the variable directly because I wasn't sure on the method calls. Although I think you forgot a set of parentheses.

    if (init_val && (v->isRef() || v->isOut()))
    

    I worked under the assumption that I didn't want to deviate the frontend from DMD and since GDC and DMD did the same thing I tried to see what GDC was doing and why it was failing to work. The conclusion I came to was that GDC only identifies a ref or out type by it's methods isRef() and isOut() and not on the type but as part of the declaration. I do not know if at some point GDC diverged from DMD on that point, I couldn't find any indication of such.

    That is the path that lead me to the fix I implemented. When GDC's backend initializes a variable of "TYPE" but the destination is "ref TYPE" since there is no indication within the type variable that it is a ref type GDC will not return a reference. Hence my check after the value to determine if it is a reference we should be dealing with and return that instead.

    Now my opinion is fairly naive when dealing with GDC but I felt that this particular issue was a backend issue and that if a statement that assigns a value to a ref came into the backend that it was legitimate and should be dealt with as such. The only question that remains for me is did GDC diverge from DMD on handling type references or does DMD do a similar thing in it's backend.

  7. Daniel Green

    Awesome, thanks for looking into that. Definitely looks like a better place for it. Also a test I did get tango-0.99.9 to compile. The only issue I came across was related to templates and implicit casting of arguments which I think is likely to be a front end issue that is fixed in a later DMD.

    Once all this is added to the repository, do you think it would be safe to tag it as 1.047 and move on to 1.048?

  8. Daniel Green

    For your patch, do you think it would also be good to include a test of STCout. inout could also be used instead of ref and I"m not sure if the front end treats it the same as ref or seperately.

  9. Iain Buclaw

    Just got back to test my patch, and looks like that did not go as well as I imagined it. Oh well, your way is as good as it can get then Daniel. Attaching a working revision of my patch that should be made for inclusion.

    And it looks like the front end treats inout as a ref to me. (Using my code example).

    (gdb) p s->ident.string 
    $1 = 0x81c70cc "gd"
    (gdb) p s->storage_class & STCref
    $2 = 2097152
    (gdb) p s->storage_class & STCout
    $3 = 0
    

    Oh, and discovered that there is a STCforeach afterall... Learn something new every day. :-)

    (gdb) p s->storage_class & STCforeach 
    $4 = 16384
    

    As for moving onto 1.048, I'd like to test building several projects first (there are quite a number of D games I help look after), try a few libraries (ie: gtkD), and perhaps run dstress on the compiler too to check for any other regressions that may have cropped up.

    Also, it looks like there are only 7 changesets between 1.047 and 1.049, just my 2 cents, but it might be worth just jumping there. http://www.dsource.org/projects/dmd/log/?action=stop_on_copy&rev=203&stop_rev=196

    But, don't listen to me, I just maintain the GDC package in Debian/Ubuntu, do what you want, and thanks for your contribution. :-)

    Michael Charlton, can you remove patches from bug reports in bitbucket? Or should you be OK with sorting through these attachments.

  10. Iain Buclaw

    OK, gtkD does not compile (it doesn't work with DMD-1.047 either).

    Tried 5 random games, and all compiled and ran OK. Good-good. :-)

    Dstress will take a while...

    Oh, and noticed an ICE bug when compiling this:

    void main()
    {
        const int[] a = [1,2,3,4,5];
        int[]b = a;
    }
    

    Not sure if the upgrade to 1.047 has anything to do with it...

  11. Log in to comment
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.