Skip to content

Commit

Permalink
Only destroy static locals if they have non-trivial destructors.
Browse files Browse the repository at this point in the history 
This fixes a regression introduced in
2b4fa53 that caused us to emit
shutdown-time destruction for variables with ARC ownership, using
C++-specific functions that don't exist in C implementations.
  • Loading branch information
@zygoloid
zygoloid committed Jan 10, 2020
1 parent d3ba1e0 commit 7a38468
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 1 deletion.
3 changes: 2 additions & 1 deletion clang/lib/CodeGen/CGDecl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,8 @@ CodeGenFunction::AddInitializerToStaticVarDecl(const VarDecl &D,

emitter.finalize(GV);

if (D.needsDestruction(getContext()) && HaveInsertPoint()) {
if (D.needsDestruction(getContext()) == QualType::DK_cxx_destructor &&

This comment has been minimized.

Sign in to view

Copy link
@rjmccall

rjmccall Jan 11, 2020

Contributor

I feel like handling this in VarDecl::needsDestruction is probably the better fix.

This comment has been minimized.

Sign in to view

Copy link
@zygoloid

zygoloid Jan 11, 2020

Author Collaborator

I think the right mental model is: this variable does need destruction, but we never happen to actually destroy these kinds of static locals on program shutdown. Acid test: if the initialization of the variable exits by exception after we finish initializing it (in a destructor for a temporary), should we call a destructor for the variable? We should ask needsDestruction that question, and for the other forms of destruction, the answer should be yes.

This comment has been minimized.

Sign in to view

Copy link
@rjmccall

rjmccall Jan 11, 2020

Contributor

Hmm. That makes sense, except my understanding is that we're actually supposed to consider the variable to be fully initialized if a temporary's destructor throws.

This comment has been minimized.

Sign in to view

Copy link
@zygoloid

zygoloid Jan 11, 2020

Author Collaborator

Oh. But it looks like we miscompile such cases anyway. Testcase:

struct X { X() noexcept; ~X() noexcept(false); };
struct Y { Y(X) noexcept; ~Y(); };
void f() {
  static Y y = X();
}

The exceptional path out of this function calls __cxa_guard_abort but not Y::~Y(). (Same thing if the static variable is an ObjC pointer with ownership, which should presumably release as necessary on the exceptional path, but doesn't.)

This comment has been minimized.

Sign in to view

Copy link
@zygoloid

zygoloid Jan 11, 2020 •
edited
Loading

Author Collaborator

I'm not really sure what the intended rule here is; [stmt.dcl]p4 says "If the initialization exits by throwing an
exception, the initialization is not complete, [...]" which could be read both ways. But we should either call both of __cxa_guard_abort and Y::~Y() or neither of them (and instead call __cxx_guard_release), so our current codegen is definitely not right one way or the other.

This comment has been minimized.

Sign in to view

Copy link
@zygoloid

zygoloid Jan 11, 2020

Author Collaborator

(This was discussed on the core reflector last year -- https://lists.isocpp.org/core/2019/05/6135.php -- but without a definitive conclusion.)

This comment has been minimized.

Sign in to view

Copy link
@rjmccall

rjmccall Jan 11, 2020

Contributor

Yeah, I think this might have come up in a discussion of whether our behavior was reasonable, and I agree that it isn't. At some point in the next few months, I'm hoping to have some time to go implement some of these things properly, including the return / initialization logic, where I think there are currently major problems with throwing destructors. But that might be just unjustified optimism. :)
HaveInsertPoint()) {
// We have a constant initializer, but a nontrivial destructor. We still
// need to perform a guarded "initialization" in order to register the
// destructor.
Expand Down
12 changes: 12 additions & 0 deletions clang/test/CodeGenObjC/initialize-function-static.m
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// RUN: %clang_cc1 -triple x86_64-apple-macos10.15 -emit-llvm -fobjc-arc -o - %s | FileCheck %s

@interface I
@end

I *i() {
static I *i = ((void *)0);
return i;
}

// CHECK-NOT: __cxa_guard_acquire
// CHECK-NOT: __cxa_guard_release

0 comments on commit 7a38468

Please sign in to comment.