Right, exactly.

No, because hasCustomRR is set whenever the method implementation isn't the standard -[NSObject autorelease] implementation. In other words, this is just inlining the standard implementation when it's dynamically known that the dispatch would end up there anyway.

I don't know what you mean. Currently, if the callee tail-calls autorelease (and necessarily this has to be MRR code), we perform an ordinary message send of autorelease. It happens to be the case that the standard implementation of autorelease will allow the autorelease to be reclaimed, but nothing about that choice is in any way non-standard on the MRR side.

Oh, yes, that's correct, at least under base ObjC rules.

In D61808#1505544, @ahatanak wrote:

In D61808#1505023, @rjmccall wrote:

The assumption that these methods return the receiver is not true without additional semantic assumptions about these methods, so the current behavior of unconditionally turning [x autorelease] into objc_autorelease(x), x seems quite broken.

Because it's possible to override an autorelease method and make the method return something other than the passed argument, for example?

If it's not possible to assume that the method returns the passed argument in MRR, the direction of this patch and https://reviews.llvm.org/D61970 is completely wrong.

Thanks! Down to minor stuff now.

The assumption that these methods return the receiver is not true without additional semantic assumptions about these methods, so the current behavior of unconditionally turning [x autorelease] into objc_autorelease(x), x seems quite broken.

Thanks, LGTM.

LGTM.

All of the IRGen changes in this patch are unnecessary according to the model we've worked out, right? The fix is just to mark the destructor as still used when we're constructing an array.

In D61165#1494001, @rsmith wrote:

In D61165#1492830, @rjmccall wrote:

In D61165#1492781, @rsmith wrote:

For the purposes of this patch, I think that means we never need a destructor for the type of a [[no_destroy]] variable.

Arrays and other subobjects of an aggregate initializaton, unless applying the standard "literally" implies the obviously perverse result that we don't destroy subobjects in such cases.

Yes, but you need the destructors for those subobjects as a side-condition of the initialization, irrespective of what kind of object that initialization is initializing. I don't think that's got anything to do with [[no_destroy]]. I think it remains the case that you never need a destructor for the type of a [[no_destroy]] variable.

In D61165#1492781, @rsmith wrote:

In D61165#1490417, @erik.pilkington wrote:

In D61165#1490168, @rjmccall wrote:

I think the intuitive rule is that initialization is complete when the full-expression performing the initialization is complete because that's the normal unit of sequencing. Note that the standard does use both "constructed" and "initialized" in different contexts, although this might just be an editorial choice.

I think it would be quite subtle if the standard was deliberately splitting this particular hair by implicitly creating a "constructed, but not initialized" state of an object, without calling that out anywhere :)

If by "constructed" you mean "a constructor has finished", then we need to split this hair. Consider:

struct B { ~B(); };
struct A {
  A() {}
  A(B &&) : A() { throw 0; }
};
void f() {
  static A a = B();
}

At the point when the exception is thrown, a constructor for a has completed, but its initialization is not complete.

[except.ctor]/3 and /4 lay out the rules:

"""
If the initialization or destruction of an object other than by delegating constructor is terminated by an
exception, the destructor is invoked for each of the object’s direct subobjects and, for a complete object,
virtual base class subobjects, whose initialization has completed (9.3) and whose destructor has not yet begun
execution, except that in the case of destruction, the variant members of a union-like class are not destroyed.
[Note: If such an object has a reference member that extends the lifetime of a temporary object, this ends
the lifetime of the reference member, so the lifetime of the temporary object is effectively not extended.
— end note] The subobjects are destroyed in the reverse order of the completion of their construction. Such
destruction is sequenced before entering a handler of the function-try-block of the constructor or destructor,
if any.

If the compound-statement of the function-body of a delegating constructor for an object exits via an
exception, the object’s destructor is invoked. Such destruction is sequenced before entering a handler of the
function-try-block of a delegating constructor for that object, if any.
"""

The above wording seems to suggest that the initialization of an object of class type completes when its outermost constructor finishes (at least for the case of initialization by constructor). And indeed all the other wording I can find that has some bearing on when an object is deemed initialized suggests that option 1 is correct, and in general that the initialization of a variable is complete when the initialization full-expression ends, which is before the destructors for any temporaries run. (Those destructor calls are separate full-expressions that happen afterwards; see [intro.execution]/5.)

In D61165#1492780, @dexonsmith wrote:

In D61165#1492724, @rjmccall wrote:

In D61165#1492582, @erik.pilkington wrote:

Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which appears to claim that initialization ends with the execution of the constructor, excluding temporary destructors.

(13) In a non-delegating constructor, initialization proceeds in the following order:
/* list of steps... */
(13.4) Finally, the compound-statement of the constructor body is executed.

John: do you agree with this analysis?

That's talking about constructor bodies.

Sure, but that's all that dcl.int suggests happens during initialization. Skipping over paragraphs that don't apply:

http://eel.is/c++draft/dcl.init#17: The semantics of initializers are as follows.

• http://eel.is/c++draft/dcl.init#17.6: [I]f the destination type is a (possibly cv-qualified) class type:
  • http://eel.is/c++draft/dcl.init#17.6.2: [I]f the initialization is direct-initialization, or if it is copy-initialization where the cv-unqualified version of the source type is the same class as, or a derived class of, the class of the destination, constructors are considered. The applicable constructors are enumerated ([over.match.ctor]), and the best one is chosen through overload resolution ([over.match]). Then:
    • http://eel.is/c++draft/dcl.init#17.6.2.1: If overload resolution is successful, the selected constructor is called to initialize the object, with the initializer expression or expression-list as its argument(s).

In D61165#1492582, @erik.pilkington wrote:

Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which appears to claim that initialization ends with the execution of the constructor, excluding temporary destructors.

(13) In a non-delegating constructor, initialization proceeds in the following order:
/* list of steps... */
(13.4) Finally, the compound-statement of the constructor body is executed.

John: do you agree with this analysis?

The flip side of that argument is that (1) there aren't very many users right now and (2) it's much easier to start conservative and then weaken the rule than it will be to strengthen it later. It really isn't acceptable to just turn off access/use-checking for the destructor, so if we get trapped by the choice we make here, we'll end up having to either leak or call std::terminate.

In D61165#1490417, @erik.pilkington wrote:

In D61165#1490168, @rjmccall wrote:

I think the intuitive rule is that initialization is complete when the full-expression performing the initialization is complete because that's the normal unit of sequencing. Note that the standard does use both "constructed" and "initialized" in different contexts, although this might just be an editorial choice.

I think it would be quite subtle if the standard was deliberately splitting this particular hair by implicitly creating a "constructed, but not initialized" state of an object, without calling that out anywhere :)

That's only true for subobjects of an enclosing aggregate before that aggregate's initialization is complete though, right? So it doesn't seem like that much of an inconsistency, just mimicking what we would be doing if an exception was thrown in, say, the body of the ctor before the object's initialization is completed.

Conceptually yes, but formally no. The standard *could* write this rule as "all currently-initialized subobjects must be separately destroyed when an exception aborts initialization of the containing aggregate, including constructor bodies and aggregate initialization", but it doesn't actually do so; instead it has specific rules covering the behavior when an exception is thrown out of the body of a constructor, and those rules simply do not apply to aggregate initialization.

Right, I was just trying to draw an analogy. Can you be more specific about the inconsistency you mentioned above? What objects with static storage duration have to be destroyed when an exception is thrown? Just subobjects of static aggregates that had their initialization aborted by an exception? If so, that obviously doesn't seem inconsistent.

LGTM.

LGTM.

In D61458#1488972, @hfinkel wrote:

In D61458#1488970, @jlebar wrote:

Here's one for you:

__host__ float bar();
__device__ int bar();
__host__ __device__ auto foo() -> decltype(bar()) {}

What is the return type of foo? :)

I don't believe the right answer is, "float when compiling for host, int when compiling for device."

So, actually, I wonder if that's not the right answer. We generally allow different overloads to have different return types.

In D61165#1488553, @erik.pilkington wrote:

In D61165#1487328, @rjmccall wrote:

In D61165#1487100, @erik.pilkington wrote:

It seems like the most common sense interpretation here is to just treat the initialization of G as completed at the point when the constructor finishes (this appears to be what GCC implements: https://wandbox.org/permlink/R3C9pPhoT4efAdL1).

Your example doesn't actually demonstrate that that's what GCC implements because it doesn't try to execute the declaration twice. But you're right, GCC does appear to consider the initialization complete as soon as the static object's constructor returns normally. On the other hand, GCC gets the array case here wrong: if a static local has array type, and a destructor for a temporary required by the first element initializer throws, then it does not destroy the element but also (correctly) does not mark the variable as fully initialized, and so a second attempt to run the initializer will simply construct a new object on top of an already-constructed one. This is arguably correct under the standard — the first array element is not a previously-constructed object of automatic duration — but I hope it's obvious that that's a defect.

So if it was static it would just get destroyed at exit-time, and therefore should be disable-able with no_destroy. If the standard implies that we should be doing something else, then we should do that, but I can't seem to find any reference to the rule you're describing.

Like I said, this is a poorly-specified part of the standard, because at least *some* objects with static storage duration have to be destroyed when an exception is thrown (because of aggregate initialization), but the standard wants to pretend that this isn't true. I think that allowing temporary destructors to cancel initialization uniformly across all kinds of objects is the most consistent rule,

That's only true for subobjects of an enclosing aggregate before that aggregate's initialization is complete though, right? So it doesn't seem like that much of an inconsistency, just mimicking what we would be doing if an exception was thrown in, say, the body of the ctor before the object's initialization is completed.

I think it would be more reasonable to just change getDeclLanguageLinkage to check for a kernel function.

In D61165#1487100, @erik.pilkington wrote:

It seems like the most common sense interpretation here is to just treat the initialization of G as completed at the point when the constructor finishes (this appears to be what GCC implements: https://wandbox.org/permlink/R3C9pPhoT4efAdL1).

Okay. This seems reasonable to me, then.

@chandlerc, I hate to do this to you, but licensing question.

Hmm. You know, there's another case where the destructor can be called even for a non-array: if constructing the object requires a temporary, I believe an exception thrown from that temporary's destructor is supposed to go back and destroy the variable. (This is, sadly, not entirely clear under the standard since the object is not automatic.) Now, Clang does not actually get this correct — we abort the construction, but we don't destroy the variable — but (1) we should fix that someday and (2) in the meantime, we shouldn't implement something which will be a problem when we go to fix that.

Okay. So it sounds like this should either be a device-only rule, with no warning in mixed-mode languages like CUDA, or we should take a different approach.

Presumably a similar rule would apply to thread-locals if you supported them.

I don't think the implication is supposed to be that padding is zero-initialized or not depending on where in the aggregate it appears, but it doesn't really matter, I don't think we're arguing about the goal of this patch.

In Swift we basically shove *everything* through our corresponding linkage-entity structure, so I'm not opposed to the basic idea.

IIRC, C says *members* are initialized as if they were in static storage, which might mean that their interior padding should be zeroed, but I don't think says anything about the padding in the enclosing aggregate. But I think zero-initializing padding is probably the right thing to do in general under this flag.

It seems to fine just forbid hidden. Again, I suspect other targets do not care because they are not using a standard dynamic loader to load the code containing kernel functions.

Okay, SGTM.

I believe at least one of the goals of nodestroy is to allow the type to potentially not provide a destructor at all, so if we're going to implicitly require the destructor anyway in certain situations, we should clearly document that, and we should be aware that we may be making the attribute less useful.

In D61147#1479940, @erik.pilkington wrote:

In D61147#1479932, @rjmccall wrote:

I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being.

Why? It seems to me like the vast majority of methods only declared in an @implementation are used as local helpers (even if people probably should be using functions for this), where this warning would be useful. Maybe I'm missing something? Still trying to learn more about Objective-C.

Are you sure these are the right semantics for nodestroy? I think there's a reasonable argument that we should not destroy previous elements of a nodestroy array just because an element constructor throws. It's basically academic for true globals because the exception will terminate the process anyway, and even for thread_locals and static locals (where I believe the exception is theoretically recoverable) it's at least arguable that we should either decline to destroy (possibly causing leaks) or simply call std::terminate.

I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being.

Sorry, yes, I meant in Objective-C methods, of course, not unconditionally even in C-like code whenever Objective-C is enabled.

Yeah, I tend to think we should just suppress this unconditionally in Objective-C.

Yeah, that seems like a missing warning.

Yeah, that's fine.

I suspect that other OpenCL and CUDA implementations don't care at all about symbol visibility for device-side code generation, and giving kernel functions default visibility seems like the right thing to do for the (relatively few) things at the AST level that are sensitive to that, like template visibility. Would you mind reaching out to other implementors about that?

In D60967#1476057, @scott.linder wrote:

In D60967#1476029, @rjmccall wrote:

Shouldn't it be an error if the user tries to give it hidden visibility?

We effectively consider the user explicitly specifying that a symbol is e.g. a kernel to also carry with it visibility information. We don't want to require the user to redundantly specify that a kernel is not hidden, when it is never meaningful for it to be hidden.

Shouldn't it be an error if the user tries to give it hidden visibility?

It seems reasonable to me for target hooks to run after global hooks, but can I ask why AMDGPU specifically relies on this?

I suspect Darwin also doesn't want to take this. We care very little about 32-bit Intel, and part of caring very little is not wanting to spend any effort dealing with the ramifications of ABI breaks. That would apply both to the rule in general and to the vector rule specifically. @dexonsmith, agreed?

LGTM.

LGTM.

Alright, LGTM.

Alright, this seems reasonable to me.

In D60573#1463641, @riccibruno wrote:

In D60573#1463569, @rjmccall wrote:

Hmm. Does this never impact code that's just using a locally-defined type within its scope? I guess if ADL is involved, unqualified lookup must have reached the scope of the innermost namespace, and so it would be searching that namespace anyway.

In that case, I think I'm mollified that the source-compatibility risk is low and we should just unconditionally apply the new rule. LGTM.

I am not sure about what you mean. It is certainly possible to construct a piece of C++11 code which breaks with this patch.

Hmm. Does this never impact code that's just using a locally-defined type within its scope? I guess if ADL is involved, unqualified lookup must have reached the scope of the innermost namespace, and so it would be searching that namespace anyway.

LGTM.

Great, SGTM.

Should we have a special has_feature test to check that this attribute is allowed on implementations? We've done that in the past when expanding the set of subjects for an attribute. But that's not necessary if we haven't made this attribute part of a public Xcode release yet, because then the attribute is just understood to always apply to implementations; I can't remember when exactly this was added and what releases it's made it into.

LGTM.

Committed as r358132.

Committed as r358115.