Generate correct code for delegate .ptr property.

[ibuclaw]

Rather than forcing the type, which confuses gcc back-end type system. Generate correct code in the front-end. Not sure if ldc suffers from this too? @klickverbot will be able to confirm. :o)

[ibuclaw]

Note, could instead create a temporary like .funcptr, but doesn't seem worth it...

if (!e->isLvalue())
{
    Identifier *idtmp = Lexer::uniqueId("__dgtmp");
    VarDeclaration *tmp = new VarDeclaration(e->loc, this, idtmp, new ExpInitializer(Loc(), e));
    tmp->storage_class |= STCctfe;
    e = new DeclarationExp(e->loc, tmp);
    e = new CommaExp(e->loc, e, new VarExp(e->loc, tmp));
    e = e->semantic(sc);
}

[dnadlinger]

Yep, this is an issue for LDC as well. In the past, we simply directly generated a GEPExp for this case that directly mapped to the LLVM getelementptr instruction. During my recent cleanup pass, I reverted that piece of code to the upstream version, but I had to add a special case to the glue code just to make it work (we usually never paint from a struct type to a scalar).

[yebblies]

I'm not a huge fan of this, I think the correct solution would be to have a new ast class, similar in concept to ArrayLengthExp. Or possibly leave the DotExp intact. This is essentially doing glue-work in the frontend, which leads to bad error messages and complications in ctfe. That said, now might not be the time to fix this.

[ibuclaw]

It's not doing glue work in the front end. Its telling the back end the we want to take the base address of the delegate.

Type munging a { .ptr, .func } to a void* is not trivially understood at all. Or any non-scalar to pointer for that matter...

[yebblies]

It's not doing glue work in the front end. Its telling the back end the we want to take the base address of the delegate.

You are generating *cast(void **)(&dg) during semantic. My litmus test is - what is ctfe supposed to think when it sees this? What would a javascript backend do with this cast? Every case where an ast construct is prematurely lowered is a bug in the frontend, in my opinion. That's what I mean by glue work.

Type munging a { .ptr, .func } to a void* is not trivially understood at all. Or any non-scalar to pointer for that matter...

Yeah, the current hack is much worse.

[ibuclaw]

Well, delegates aren't CTFE -able, no? Much for the same reason why virtual methods aren't CTFE -able.

[donc]

Actually delegates are CTFEable, and so are virtual functions!
There is currently a problem with getting .ptr from a delegate (bug 7204) because it has been turned into a void* cast. What you're doing here is probably an improvement but what daniel suggests is probably the correct approach.

[ibuclaw]

Actually delegates are CTFEable, and so are virtual functions!

Well I should really give you a wizard hat then! :o)

[ibuclaw]

There is currently a problem with getting .ptr from a delegate (bug 7204)

@donc - I've had a look at the bug report, and that's for .funcptr, this change is for the object reference.

[yebblies]

This causes a regression on the following code:

static assert({ return (delegate () {}).ptr != null; }());

I also don't see the point of having both an rvalue and an lvalue case. If the rvalue code works, why have the lvalue code at all?

[yebblies]

I also don't see the point of having both an rvalue and an lvalue case. If the rvalue code works, why have the lvalue code at all?

Of course it makes sense now that I've had some sleep.

[ibuclaw]

static assert({ return (delegate () {}).ptr != null; }());

That would generate {.object = null, func = &__dgliteralxxx} - so the check that dg.object != null I would expect to be false in that case. If the delegate required a closure, then it would pass.

[ibuclaw]

then it would pass.

Would and should pass.

[yebblies]

Try it before and after applying this patch, you will see.

[ibuclaw]

Aye, I am also aware that CTFE now sees dg.ptr as returning the address of a local stack variable. :o)

[WalterBright]

I'm not sure what the state of this is. Is there a reason for the separate lvalue/rvalue path, and is the "you will see" mean it works correctly?

[ibuclaw]

There is a separate lvalue/rvalue path because cast's can't be used as lvalues, eg: cast(foo) x = y;. This is an error in D, but such codegen is also invalid gcc if not caught by the front-end.

[WalterBright]

There is a separate lvalue/rvalue path because cast's can't be used as lvalues, eg: cast(foo) x = y;. This is an error in D, but such codegen is also invalid gcc if not caught by the front-end.

There needs to be a comment to this effect in the source code. Also, what about @yebblies last comment?

[yebblies]

You need to fix the ctfe regression i posted earlier, and/or add the test case:
static assert({ return (delegate () {}).ptr != null; }());

[ibuclaw]

That test case doesn't make sense to me. Don't you mean:
static assert({ return (delegate () {}).funcptr != null; }());

[yebblies]

You need to be able to access the .ptr property at compile time.

[ibuclaw]

Because, to me .ptr != null; should imply that the delegate is nested, and uses a parent closure/frame. If either one isn't the case, then the object reference pointer should be nulll.

[yebblies]

Yeah, makes sense.

How about:
static assert({ int i; return (delegate () { return i; }).ptr != null; }());

I don't care about the value of .ptr so much as that it works at compile time.

[ibuclaw]

OK, I'm picking this up and giving it a rethink this week. Any ideas you want to throw at the drawing board in terms of representing this in the front-end implementation?

Thanks

[yebblies]

DelegatePtrExp? Like array.length, this is a special property, with special codegen and interpret code (although not as complicated). Giving it an ast class and TOK will make it very clear and remove all need for reverse-engineering the original expression.

[WalterBright]

@ibuclaw can you please add the comment I suggested and the test case @yebblies did, so I can merge this?

[yebblies]

@ibuclaw @donc @WalterBright

I have a patch that implements this, but it changes the behavior of .ptr and .funcptr.

https://github.com/yebblies/dmd/tree/dgfuncptr

This is a program that compiles without errors with my changes, current behavior is shown in comments where it differs.

void main()
{
    int n;
    auto p = { ++n; };
    enum q = delegate {};

    static assert(is(typeof(p.ptr) == void*));
    static assert(is(typeof(p.funcptr) == void*)); // (currently, void())

    // Can read dg.ptr at run time
    auto x = p.ptr;

    // Can read dg.funcptr at run time
    auto y = p.funcptr;

    // Cannot read dg.ptr at compile time (currently, can)
    static assert(!__traits(compiles, { enum a = q.ptr; }));

    // Cannot read dg.funcptr at compile time (currently, can)
    static assert(!__traits(compiles, { enum a = q.funcptr; }));

    // cannot assign dg.ptr (currently, can)
    static assert(!__traits(compiles, p.ptr = null));

    // cannot assign dg.funcptr (currently, can)
    static assert(!__traits(compiles, p.funcptr = null));
}

I'm not so sure about prohibiting them at compile time, I guess that you could use them to determine if two delegates reference the same function/data.

This patch has a side effect of fixing bug 2438

Thoughts?

[ibuclaw]

Where you say "currently can" at compile time - does it actually work ? I'd honestly be surprised if it did...

As for the other two, I would probably stress assignment to .ptr and .funcptr - unless of course you want to make them lvalue properties...

[ibuclaw]

And if the examples where you currently can at compile time, I would expect CTFE to translate them to (p.ptr) *(cast(void**) &p) and (p.funcptr) *(cast(void**)&p + size_t.sizeof)

[yebblies]

Well, you can do enum a = q.ptr is q.ptr; and the same with funcptr, so they are at least accepted if never dereferenced. What should we be able to do with them?

And yeah, I think .ptr and .funcptr should be rvalues - assigning them is always going to be pretty advanced and unsafe, so I don't think forcing casts is out of line here.

[yebblies]

edit: s/lvalues/rvalues

[ibuclaw]

edit: s/lvalues/rvalues

Whoops, my bad... (must have been looking into a mirror to work out which was my left hand).

[ibuclaw]

static assert(is(typeof(p.funcptr) == void*))

The typeof funcptr should be the function type, void* is just way off.

[dnadlinger]

The function pointer type would be so even further, as there is no way to
represent the context pointer in the parameter list.

[yebblies]

The function pointer type would be so even further, as there is no way to
represent the context pointer in the parameter list.

Exactly. It makes very little sense for it to be callable.

[WalterBright]

The code change is devoid of comments. Given all the comments here, surely there should be some explanation in the comments, and a test case to validate the changes.

[ibuclaw]

@WalterBright - I'm not sure what people are even asking to change here anymore.

For me, it helps if the frontend does an explicit cast from delegate -> pointer rather than the type stuffing that is currently going on.

For Daniel, he wants CTFE support, but I'm not sure that actually covers the scope of this pull.

[yebblies]

I want this not to cause a regression - the .ptr property is currently readable at compile time and after applying this patch it is not.

[ibuclaw]

OK - there are no current tests for this though so it not immediately evident what the regression is, and whether or not the regression is actually a bug fix. :-)

I honestly doubt that .ptr would be readable at compile time, because .ptr is not constructed until the codegen stages.

[ibuclaw]

btw @yebblies - where did you get to with your DelegatePtrExp bits?

[yebblies]

The regression is the code that currently compiles, but doesn't after applying this patch. #2200 (comment)

I might have another look at DelegatePtrExp today, and try not to get sidetracked by fixing the type again.

[yebblies]

The current patch I have does this:

• Readable at run time
• not readable at compile time
• not writable
• not lvalue

The rvalueness matches what arr.ptr does, and I think it's a good idea.

Should it be readable at compile time?
Should it be assignable/lvalue?

[yebblies]

It looks like accessing it from ctfe doesn't currently really work anyway, although IIRC this patch makes it ICE.

So if both properties being an rvalue is acceptable, maybe this will do it? #3181

[ibuclaw]

It looks like accessing it from ctfe doesn't currently really work anyway, although IIRC this patch makes it ICE.

I don't think so. Maybe more like turn something that questionably works into an 'error: can not evaluate delegate'

[yebblies]

Ok. Still, I think something along the lines of #3181 would be better.

[andralex]

ping on this