Trivial constructors should allow the struct to be passed as POD

[ibuclaw]

https://d.puremagic.com/issues/show_bug.cgi?id=11740

Improvements to be done:

1. isPOD does not check for assignment operators at all.
2. isPOD does not cache its return value.
3. Can not infer whether the ctor is nothrow as fbody might not have fully ran its semantic.

[andralex]

This changes the definition of isPOD. The purpose as I figure is to make the struct as good as C, which would definitely mean no dtor. What is the motivation behind the proposed definition of isPOD?

[dnadlinger]

@andralex: isPOD is used to decide whether to pass a struct in registers under the x86_64 ABI, and should match what is specified for C++ structs there (3.2.3): "If a C++ object has either a non-trivial copy constructor or a non-trivial destructor, it is passed by invisible reference (the object is replaced in the parameter list by a pointer that has class INTEGER)". The name is arguably misleading, something among the lines of canPassInRegisters would be clearer.

@ibuclaw: Why would you allow destructors?

[yebblies]

Why can't we support throwing constructors?

[ibuclaw]

@yebblies - see point 3 in my initial message.

@klickverbot - The point of passing in memory is that you can catch the object in EH. If the dtor is guaranteed not to throw, then we don't need to worry about this.

[ibuclaw]

Forgot to include link to bug report

[yebblies]

@ibuclaw I can see it, but I don't think it answers my question. We can't support throwing constructors because we can't tell if they might throw?

[dnadlinger]

@ibuclaw: Could you sketch a situation where throwing constructors would be a problem, and how constructors relate to parameter passing in the first place? It seems to me that we would always be able to construct objects on the stack and then move them to registers. Assignment (copy constructors in C++) is a different story, of course.

[ibuclaw]

@klickverbot when calling the constructor, the 'this' parameter for the would be passed via registers rather than on the stack.

@yebblies - change of direction, ignoring nothrow altogether and a basterdised pass over the ctor body to ensure is only made up of simple assignment statements.

[yebblies]

idpod == ISPODyes?

[ibuclaw]

I've looked at it again, and I honestly can't remember what I was thinking using bitwise operators... :o)

[ibuclaw]

Must have thought that I was programming in B

[ibuclaw]

@klickverbot - https://d.puremagic.com/issues/show_bug.cgi?id=11224 is an example which shows 'this' being passed on the stack. Incidentally this pull will now break that because it now passes the struct through registers in that test.

[yebblies]

when calling the constructor, the 'this' parameter for the would be passed via registers rather than on the stack.

Huh? 'this' is a reference. It doesn't matter where it's passed. The difference between POD and non-POD is only for pass-by-value...

[yebblies]

Just return from here, no need for goto.

[yebblies]

I still don't get it. By definition, constructors are called on an initialized chunk of memory. If the inliner is screwing with that then it's an inliner bug, right?

[ibuclaw]

Because toArgTypes is returning a TypeTuple(int) - meaning this struct passed via the first available integer register, it's no longer being passed by reference on the stack. Hence ptr11224 != &s

[yebblies]

How on earth is toArgTypes getting TypeTuple(int) for ref Struct?

[dnadlinger]

[incorrect reply to Daniel removed]

@ibuclaw: I fail to see how bug 11224 is related to my question:

First of all, I asked for an example where a throwing constructor is a problem, but there are no exceptions in sight. :P

Regarding the test case, I don't think it is related to constructors or the low-level ABI at all. First, note that the this for structs is always a reference in D2, so your implication that the this parameter should be passed on the stack is certainly wrong on x86_64 (unless DMD horribly messed up its implementation of the C ABI; it is just a pointer). In the absence of NRVO, there might be different copies of the struct (which then triggers the address identity assertion), but this is only tangentially related to parameter passing.

Second, I'd doubt that the failing test case is fundamentally related to constructors at all – ptr11224 just happens to be set in the constructor, but the assignment could be in foo11224 just as well. Sure, the presence of a constructor right now disables passing structs in registers, but as far as this example is concerned, this is merely a peculiarity of the current implementation.

Now, regarding the test case failure: Either, the language guarantees that NRVO happens if possible, regardless of whether it makes any sense, or the test case just makes the flawed assumption that it will always happen, because this actually was the case prior to the patch. Now, given that the entire point of NRVO is to be an optimization, and storing small structs in registers is pretty much always going to beat a memory access, I don't think the former would be a reasonable decision. But if it was the case, then structs would have to be always returned in memory, and this rewrite to a hidden pointer parameter would take place before the ABI comes into play. Thus, I'm heavily leaning towards the second option – just fix that broken test case.

[yebblies]

@yebblies: IIRC DMD uses D integer types to represent the INTEGER from the System V x86_64 ABI, so that makes sense (as pointers are passed in registers, of course).

It seems to use tvoidptr for that, as it should.

TypeTuple *TypePointer::toArgTypes()
{
    return new TypeTuple(Type::tvoidptr);
}

[dnadlinger]

On Thu, Dec 19, 2013 at 4:20 PM, Iain Buclaw notifications@github.comwrote:

@klickverbot https://github.com/klickverbot when calling the
constructor, the 'this' parameter for the would be passed via registers
rather than on the stack.

I see that this comment is gone from the web interface – does this mean
that you agree? ;)

[ibuclaw]

I see that this comment is gone from the web interface – does this mean that you agree? ;)

@klickverbot comment is still there. ;)

As for NRVO, I despise that the testsuite treats it as if the language requires NRVO to happen anyway... :-)

[dnadlinger]

Err, okay, then the GitHub notifications were just behaving erratically... So, if you don't think NRVO is required to happen, then why did you cite that issue/test case in the first place?

By the way, I don't know of any part of the spec that would mandate NRVO, but it could have just slipped from my mind...

[ibuclaw]

@klickverbot - it's not in the spec, but phobos code (std.typecons.scoped) requires NRVO in order to implement its own semantic. Which is bad, very bad...

[ibuclaw]

This: https://d.puremagic.com/issues/show_bug.cgi?id=4500
Another example of reliance on NRVO (this was the fix): dlang/phobos#1757

[dnadlinger]

@ibuclaw: I agree that the unclear semantics of struct return values/NRVO are a problem. But the point I have been trying to make is that this doesn't have anything to do with constructors.

[ibuclaw]

I've spoken some more to @yebblies - and this is taking the wrong angle anyway. isPOD should be more strict, not laxed, and the dmd backend shouldn't be using isPOD to determine whether to pass the struct via invisible reference.

[dnadlinger]

@ibuclaw: Determining whether to keep a struct in registers is the entire point of isPOD, period. There is simply no other meaning ascribed to it. A trait based on the function was added in #1421, see the discussion there.

If you and @yebblies reached a different conclusion, then we are exactly in the situation I predicted back then - without appropriate documentation, apparently the purpose of it isn't even clear to compiler hackers.

[yebblies]

The only thing I care about is that we must match the System V ABI for passing structs to extern(C++) functions.

[ibuclaw]

@yebblies it's not just the System V ABI - it's at least the C++ ABI for GCC (not sure about other C++ implementations). All structs that define a user cpctor/postblit/dtor should be candidates for pass by reference.

[yebblies]

Doesn't g++ follow the System V ABI? I see the last set of commits still has a ctor marking a struct as non-POD...

[WalterBright]

The point of POD is so that it can be placed in registers. Since you cannot take the address of a register, a POD struct cannot have operations on it that require taking the address of it to make copies. This is why destructors and copy constructors are not allowed for POD.

[yebblies]

@WalterBright The issue here is just with normal constructors. Or at least, the issue in the bug report. I have no idea where this pull is supposed to be going.

Do you agree that a (non-copy) constructor should not prevent a struct from being passed in registers? (ie we follow the System V ABI)

[WalterBright]

Do you agree that a (non-copy) constructor should not prevent a struct from being passed in registers? (ie we follow the System V ABI)

I can't think of a reason why not.

But I also can't think of a reason why private fields should influence this.

[yebblies]

But I also can't think of a reason why private fields should influence this.

I'm with you there.

[ibuclaw]

(I've been away a while, so am just catching up). This started as one thing and ended up something completed different. So I'm closing this and re-opening each individual amendment separately.

The original problem with @yebblies is that D compiler and runtime (see argtypes.c) assumes that if a struct is non-POD, then it is passed in memory by reference (via hidden pointer). Which as far as I can tell is not the case in any other ABI except Microsoft's own cdecl (Ugh!).

The more common ABI (eg: all other flavours of *NIX) is that only non-trivially copyable structs are passed by reference. This can be determined by the following rules, albeit checked recursively:

bool
StructDeclaration::isTriviallyCopyable()
{
  return !(cpctor || postblit || dtor);
}

For linux, trivially copyable structs should be passed in registers. Which is not currently the case.

[ghost]

These might be useful to someone:

C++03 POD rules: http://stackoverflow.com/a/4178176/279684
C++11 POD rules: http://stackoverflow.com/a/7189821/279684

GCC has a simple macro to check if a symbol is a non-pod type via CLASSTYPE_NON_POD_P, I've used it before when exporting an AST to XML (via gccxml), for when I had to wrap some C++ code in D.

[yebblies]

Funny, on win32/dmc++ having a ctor is enough to make it non-pod.

[ibuclaw]

Probably an oversight by @WalterBright - or there is something funny going on in the implementation detail that makes it non-pod.

[yebblies]

It appears to be correct msvc behavior. win32 is just different.