In D45766#1076176, @dblaikie wrote:

Is there anything else in the "-w" namespace other than the literal "-w" so
far?

In D45766#1076090, @dblaikie wrote:

FWIW I don't fundamentalyl object to also having something like -wtest.
Probably needs a better name though (unfortunately the double-negative gets
confusing... - like you want to describe the set of diagnostics that should
not be used in test code, so that as a group might be "-Wnon-test" but then
"-Wno-non-test" is pretty awkward) - probably worth chatting to Richard
Smith about that, I reckon.

This does not address my review. My review is suggesting that we avoid this issue completely by fixing IRGen to use an external linkage for internal declarations in your emission mode. That would allow you to just emit the module ctors as truly internal in the first place, removing any need to mangle them.

These functions must predate the addition of CreateLifetimeStart and CreateLifetimeEnd methods on IRBuilder; we should just be calling those.

Yes, I'm sorry, I think you're right. I had misunderstood the language problem when I suggested going down this road.

Let me try to summarize where I think we are.

The goal of having a unified option is that you can uniformly suppress warnings that don't always make sense in unit tests. It's future-proofing against the addition of other warnings (probably C++ warnings) that might not make sense for unit tests, like extending the x &= x warning (which is not in -Wself-assign) to user-defined operators. You don't think you would be able to take advantage of that? Because -Wno-self-assign-overloaded-nonfield is rather, uh, pretty precisely targeted for exactly that use case; I can't imagine why someone would use -Wself-assign-overloaded-nofield *positively*, or even *negatively* for any purpose besides suppressing a unit-test problem.

I'm sorry, that warning *is* in self-assign, although I'm not sure it should be.

Thanks; LGTM.

Thank you. Minor request and then LGTM.

If so, LGTM.

LGTM.

I'd still prefer if someone with more driver-design expertise weighed in, but we might not have any specialists there.

I think there was a point when we weren't always creating CXXThisExprs eagerly for these accesses. Now that we are, yeah, this should be dead.

Okay, LGTM.

Well, that is a really silly bug. Fix LGTM.

Committed as r329508.

Hmm. Alright, I guess.

If that's the problem, then I think the right design is for CallArg to include an optional cleanup reference for a cleanup that can be deactivated at the instant of the call (we should assert that this exists for parameters that are destroyed in the callee), and then for forwarding it's just a matter of getting that cleanup from parameter emission to forwarding-argument emission.

Just a couple minor requests; if you accept them, feel free to commit.

This looks okay to me, but I think it would better if someone with more expertise in the design of the driver and frontend code could review this.

In D45384#1060369, @ahatanak wrote:

Yes. I intended it as a property that propagates to classes that contain or derive from the type.

Would it make it less confusing if I merged CXXRecordDecl::CanPassInRegisters and RecordDecl::CannotPassInRegisters into a single enum? For example, the enum could have three enumerators, "CanPass", "CannotPass", "CanNeverPass", or something. Both "CannotPass" and "CanNeverPass" would force the type to be passed indirectly, and the only difference is that "CanNeverPass" propagates its property outwards and "CannotPass" doesn't. C structs are either "CanPass" or "CanNeverPass" while C++ structs can take any of the three values.

In D45384#1060192, @ahatanak wrote:

In D45384#1060164, @rjmccall wrote:

Well, but I think CanPassInRegisters==false in the base class does always mean CanPassInRegisters==false in the subclass.

I think there are cases that is not true. If I compile the following code, S0 (base class) is passed indirectly and S1 (derived class) is passed directly.

Yes, this is fine.

Well, but I think CanPassInRegisters==false in the base class does always mean CanPassInRegisters==false in the subclass.

Hmm. I'm not actually sure *why* it's not okay to forward callee-cleanup arguments. Do we just not have the necessary logic to disable the cleanup in the caller?

In D42092#1058841, @rsmith wrote:

In D42092#1058772, @rjmccall wrote:

Issue #3 is tricky; it's arguably not okay to force a landing at a landing pad if we're not actually catching anything.

I think the only way this is observable (at least within the bounds of well-defined C++ code) is whether -- and how much -- the stack is unwound before a call to terminate() in the case where an exception is not caught.

[except.handle]p9 says: "If no matching handler is found, the function std::terminate() is called; whether or not the stack is unwound before this call to std::terminate() is implementation-defined (18.5.1)."
[except.terminate]p2 says: " In the situation where no matching handler is found, it is implementation-defined whether or not the stack is unwound before std::terminate() is called. In the situation where the search for a handler (18.3) encounters the outermost block of a function with a non-throwing exception specification (18.4), it is implementation-defined whether the stack is unwound, unwound partially, or not unwound at all before std::terminate() is called. In all other situations, the stack shall not be unwound before std::terminate() is called."

So I think you're right; this solution to issue #3 is non-conforming, because it will result in the stack being partially unwound on the path to a call to std::terminate() for an unhandled exception. Even if we changed the rule for terminate to allow partial unwinding when no handler is found, this patch would still give a QoI regression for that case (because the call to the terminate_handler would happen after some unwinding had been performed).

I'm not particularly interested in pushing this patch further if it can't actually make catch-by-reference work properly. Let's drop this for now and encourage interested parties to work on an alternative personality function.

Okay, LGTM.

Changing the requirements on the return-value slot would be an ABI break, no? And it would force to us to treat all complete-object constructions as nvsize-limited unless they're constructions of known sizeof-sized allocations.

In D42092#1057881, @rsmith wrote:

In D42092#983892, @rjmccall wrote:

This is definitely something that the personality function should handle. If we want to do heroic things in the absence of personality function support, we can, but the code should at least be written to be conditional on personality support.

I suppose the question is, do we want to do such heroic things?

Also, we don't have a personality function that can get this right, nor even a concrete design proposed for one. It doesn't seem too hard -- we'd need to extend the EH table to indicate catch-by-reference -- but there's nothing for this to be made conditional on right now. I can easily add a CodeGenOption that turns this all off, if you'd like.

The change LGTM, but please add a test.

It's not unreasonable to just add -fmerge-all-constants to the command line invocations for the individual tests, yeah. We can take those off as necessary later.

Thanks for splitting the patch up. LGTM.

The other changes I see here seem reasonable, but please do split the patch.

Okay, LGTM with the reduced set of changes to the functionality.

Okay. LGTM. Thank you for putting the effort into maintaining both rules simultaneously.

Yes, if we think that the committee is likely to include questionable functions on the [[nodiscard]] list which we don't want to warn about pre-C++17, then it makes sense to have two internal macros, one for functions like std::move that should be unconditionally warned about and one for the iffy-bu-standard-directed cases.

I think the appropriate place to do this is in IsStandardConversion, immediately after the call to ResolveAddressOfOverloadedFunction. You might want to add a general utility for getting the type-of-reference of a function decl.

Is Marshall arguing that the standard doesn't allow compilers to warn about failing to use these function results prior to C++17? Because I don't think that's true; warnings are thoroughly non-normative.

Trust me, I understand that this is an important function, but a lot of functions are important, and we're not going to hardcode knowledge about all of them in the compiler.

Well, we should feel welcome to submit patches to the C++ library implementations, I think. I can understand why Marshall is waiting to just do this until he gets a comprehensive committee paper, but he might consider taking a patch in the meantime. But I'm not sure what the rush is if it doesn't change what goes into any concrete release of the compiler + library.

In D44536#1054929, @rsmith wrote:

It seems that we have two options: either we valiantly try to support this:

• we keep a list of types for which we've tried to form a delete-expression, but found that the type was incomplete
• when such a type is completed, we mark the destructor as used, triggering its synthesis or instantiation if necessary, and likewise for the operator delete
• we change our CXXDeleteExpr representation so that we don't store any information that might be changed by completing the class on the expression, such as the selected OperatorDelete function and whether we're performing a sized delete

No, the analysis is intentionally syntactic; it should apply even on dependently-typed arguments (but not get re-checked on instantiation). But I agree that the warning ought to be disabled in unevaluated contexts.

That seems reasonable. And this summer would still be in time for the next release.

Yeah, actually, I'm second-guessing myself. Maybe this should just be a libc++ / libstdc++ bug.

LGTM. I think it wouldn't be unreasonable to ask standard library maintainers to add [[nodiscard]] to std::move, but it's also not unreasonable for us to special-case some functions.

LGTM, thanks.

Right. I think it's fair to acknowledge that many data structure unit tests will contain a legitimate use of a user-defined self-assignment without feeling that that disqualifies the warning.

Thanks. LGTM whenever you've cleared up the self-hosting problems.

LGTM.

In D45082#1053724, @lebedev.ri wrote:

• Rebase
• Drop comments
• Silence the warning via "*&" contraption, not by casting to the reference.
  
  The main question still stands, do we want to recommend everyone to use that in their codebases, or double the amount of diag flags and provide a way to silence this specific diagnostic?

Seems fine to me, but you might want someone with analyzer experience to review.

In D45101#1053246, @ahatanak wrote:

Note that CGObjCNonFragileABIMac::EmitClassRef also passes the class identifier to CGObjCNonFragileABIMac::EmitClassRefFromId, but it doesn't cause a problem. CGObjCNonFragileABIMac::EmitClassRefFromId uses the identifier only when the ObjCInterfaceDecl passed to it is null and that happens only when it is called from CGObjCNonFragileABIMac::EmitNSAutoreleasePoolClassRef.

What would the design for that warning be? C promotes all arithmetic on sub-int types to int, and if that's assigned back to a variable of the original type, there's a conversion. But you seem to only want to warn about truncating the result of multiplication and not, say, addition or negation. Is there a principle to this? Just the likelihood of escaping the range of the original type?

In D44985#1051840, @yaxunl wrote:

In D44985#1050876, @rjmccall wrote:

In D44985#1050674, @yaxunl wrote:

In D44985#1050670, @rjmccall wrote:

What exactly are you trying to express here? Are you just trying to make these external declarations when compiling for the device because __shared__ variables are actually defined on the host? That should be handled by the frontend by setting up the AST so that these declarations are not definitions.

No. These variables are not like external symbols defined on the host. They behave like global variables in the kernel code but never initialized. Currently no targets are able to initialize them and it is users' responsibility to initialize them explicitly.

Giving them an initial value will cause error in some backends since they cannot handle them, therefore put undef as initializer.

So undef is being used as a special marker to the backends that it's okay not to try to initialize these variables?

I think undef as the initializer tells the llvm passes and backend that this global variable contains undefined value. I am not sure if this is better than without an initializer. I saw code in CodeGenModule::getOrCreateStaticVarDecl

// Local address space cannot have an initializer.
llvm::Constant *Init = nullptr;
if (Ty.getAddressSpace() != LangAS::opencl_local)
  Init = EmitNullConstant(Ty);
else
  Init = llvm::UndefValue::get(LTy);

which means OpenCL static variable in local address space (equivalent to CUDA shared address space) gets an undef initializer.

For CUDA shared variable, in CodeGenFunction::EmitStaticVarDecl, it first goes through call of CodeGenModule::getOrCreateStaticVarDecl and gets a zeroinitializer, then it reaches line 400

// Whatever initializer such variable may have when it gets here is
  // a no-op and should not be emitted.
  bool isCudaSharedVar = getLangOpts().CUDA && getLangOpts().CUDAIsDevice &&
                         D.hasAttr<CUDASharedAttr>();
  // If this value has an initializer, emit it.
  if (D.getInit() && !isCudaSharedVar)
    var = AddInitializerToStaticVarDecl(D, var);

Although this disables adding initializer from D, var already has a zeroinitializer from CodeGenModule::getOrCreateStaticVarDecl, therefore its initializer needs to be overwritten by undef.

Probably a better solution would be do it in CodeGenModule::getOrCreateStaticVarDecl, side by side by the OpenCL code.

In D44883#1051878, @lebedev.ri wrote:

Ok, tried llvm stage2 build, and it failed as expected, in code that intentionally does self-assignment:

In D44536#1051181, @ahatanak wrote:

I see, so Sema::CheckCompletedCXXClass probably isn't the right place to call DeclareImplicitDestructor as that could significantly increase the size of the AST.

I think it's part of an effort to avoid creating implicit declarations for all the special members of every struct we parse from system headers.

LGTM!

Wow, the IR improvements here are amazing.

LGTM. I guess it just didn't matter for Objective-C because the variable always has to be an Objective-C pointer and the extra store of nil never hurt anything.

LGTM.

LGTM.

In D44985#1050674, @yaxunl wrote:

In D44985#1050670, @rjmccall wrote:

What exactly are you trying to express here? Are you just trying to make these external declarations when compiling for the device because __shared__ variables are actually defined on the host? That should be handled by the frontend by setting up the AST so that these declarations are not definitions.

No. These variables are not like external symbols defined on the host. They behave like global variables in the kernel code but never initialized. Currently no targets are able to initialize them and it is users' responsibility to initialize them explicitly.

Giving them an initial value will cause error in some backends since they cannot handle them, therefore put undef as initializer.

You should send an RFC to cfe-dev about adding this new language mode. I understand that it's very similar to an existing language mode that we already support, and that's definitely we'll consider, but we shouldn't just agree to add new language modes in patch review.

What exactly are you trying to express here? Are you just trying to make these external declarations when compiling for the device because __shared__ variables are actually defined on the host? That should be handled by the frontend by setting up the AST so that these declarations are not definitions.

If __global__ is supported in C++ structures, you might also need to make sure that member function constants (&A::kernel_function) drop the CC. And it might be a good idea to make sure that decltype(kernel_function) doesn't have a problem with it, either, since that does do some special-case work.

Is it possible to just do this for all structs? I don't think it hurts anything to do it for structs that are trivial and returned indirectly, and it would certainly be nice to construct C return values in-place even if they're guilty of nothing more than being, y'know, really really big.