stdlib: remove some static assertions which may not hold #21606

compnerd · 2019-01-03T16:36:46Z

These are based around the idea that std::atomic is trivially
constructible, which is not a guarantee that the standard fully
provides. The default initialization of the std::atomic type
may leave it in an undetermined state. These were caught using
the Visual C++ preview runtime.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

compnerd · 2019-01-03T16:37:04Z

@lorentey @moiseev @gparker42

lorentey · 2019-01-03T16:42:24Z

cc @mikeash

stdlib/public/SwiftShims/GlobalObjects.h

compnerd · 2019-02-08T03:49:37Z

@swift-ci please test Linux platform

swift-ci · 2019-02-08T03:52:49Z

Build failed
Swift Test OS X Platform
Git Sha - e0e3b499336e138bad9e4ba81ad668d06c5cc7a6

jrose-apple · 2019-02-08T17:25:19Z

I don't think we can remove these assertions. We rely on a HeapObject coming out of calloc ready to go.

compnerd · 2019-02-08T20:04:53Z

@jrose-apple - that is not a guarantee that the C++ standard makes. The std::atomic needs to be initialized. This was the discussions that @lorentey and I had above, and he also seemed to agree that this was the correct thing to do.

mikeash · 2019-02-08T20:37:08Z

From the random copy of C++11 that turned up in a search:

The atomic integral specializations and the specialization atomic<bool> shall have standard layout. They shall each have a trivial default constructor and a trivial destructor.

Which I think means it has to have the same representation as the integer type, and not do any initialization. I'm bad at standardese though so draw your own conclusions.

compnerd · 2019-02-08T21:59:42Z

Yes, it has the same representation, but the constructor is needed to initialize it to 0 (otherwise it is in an indeterminate state). The zero-ing of the member is sufficient to make it fail the trivial constructability test. See LWG Issue 2334. LWG P0883 is designed to address this which will make the std::is_trivially_constructible no longer hold.

jrose-apple · 2019-02-08T22:13:33Z

So we can still put them in constant sections because of the newly-added constexpr, but we can't just calloc a HeapObject or HeapObject-containing thing anymore. Maybe that's okay if we actually rewrite the stdlib to do that, but let's do that first and then remove the static_asserts rather than the other way around.

compnerd · 2019-02-08T23:02:27Z

@jrose-apple - hmm, perhaps you can file a radar for that? I think that this would be something that Apple might want to track more closely (but, this was found by building with the next version of the C++ runtime from Microsoft which does actually implement parts of the future specification), so this will become a problem for everyone.

jrose-apple · 2019-02-09T00:54:30Z

@mikeash, what do you think?

mikeash · 2019-02-11T17:56:23Z

It looks like we don't actually rely on the memory being zero-filled at allocation as I was thinking. We call through to malloc, not calloc, and then initialize the header manually by setting the metadata pointer to the metadata, and setting the refcounts to whatever RefCountBits(0, 1) ends up being.

That initialization has an interesting comment:

// FIXME: this should be a placement new but that adds a null check
object->metadata = metadata;
object->refCounts.init();

Can we find a way to use placement new without penalty? Seems like that would be the best for everything.

compnerd · 2019-02-11T21:12:02Z

@mikeash - yes and no. It is "removable" by switching to C++17. As of C++17, a null pointer to the placement new is UB, which means that the nullptr check should not be emitted by the compiler.

compnerd · 2019-02-14T17:06:47Z

@mikeash - until we can move to C++17, what do you think is the best way to move forward here?

compnerd · 2019-02-25T16:32:43Z

@mikeash - ping

mikeash · 2019-02-27T19:41:35Z

Sorry to leave you hanging! I don't want to introduce an unnecessary NULL check. If placement new means we have to have the check for now, can we use placement new when compiling with VC++ and keep the existing version for other compilers?

Alternately, is it possible to avoid the NULL check with an annotation to inform the compiler that the pointer is definitely not ever NULL?

Either one would be good by me.

compnerd · 2019-02-27T21:17:09Z

I'm happy with a C++17/Windows path through this codepath. Interestingly enough, I do not see a null pointer check even with C++14:

	.def	 "?_swift_allocObject_@@YAPEAUHeapObject@swift@@PEBU?$TargetHeapMetadata@UInProcess@swift@@@2@_K1@Z";
	.scl	3;
	.type	32;
	.endef
	.section	.text,"xr",one_only,"?_swift_allocObject_@@YAPEAUHeapObject@swift@@PEBU?$TargetHeapMetadata@UInProcess@swift@@@2@_K1@Z"
	.p2align	4, 0x90         # -- Begin function ?_swift_allocObject_@@YAPEAUHeapObject@swift@@PEBU?$TargetHeapMetadata@UInProcess@swift@@@2@_K1@Z
"?_swift_allocObject_@@YAPEAUHeapObject@swift@@PEBU?$TargetHeapMetadata@UInProcess@swift@@@2@_K1@Z": # @"?_swift_allocObject_@@YAPEAUHeapObject@swift@@PEBU?$TargetHeapMetadata@UInProcess@swift@@@2@_K1@Z"
.seh_proc "?_swift_allocObject_@@YAPEAUHeapObject@swift@@PEBU?$TargetHeapMetadata@UInProcess@swift@@@2@_K1@Z"
# %bb.0:                                # %entry
	push	rsi
	.seh_pushreg 6
	sub	rsp, 32
	.seh_stackalloc 32
	.seh_endprologue
	mov	rsi, rcx
	mov	rcx, rdx
	mov	rdx, r8
	call	swift_slowAlloc
	mov	qword ptr [rax], rsi
	mov	qword ptr [rax + 8], 2
	add	rsp, 32
	pop	rsi
	ret
	.seh_handlerdata
	.section	.text,"xr",one_only,"?_swift_allocObject_@@YAPEAUHeapObject@swift@@PEBU?$TargetHeapMetadata@UInProcess@swift@@@2@_K1@Z"
	.seh_endproc

mikeash · 2019-02-27T21:19:57Z

Maybe the compiler has already figured out that it can't be NULL? Let me know and send me a diff if you'd like me to try with clang and see if it produces good code.

compnerd · 2019-02-27T21:28:57Z

@mikeash - uploaded a new version of the change that enables it under C++17 and Windows, I'll double check this on Linux as well.

swift-ci · 2019-02-27T21:31:04Z

Build failed
Swift Test Linux Platform
Git Sha - 377a5e8dee7172dd929a20c28291af98efbeca33

mikeash · 2019-02-27T21:31:49Z

I'll try the placement new path on Mac and see how it does. If we can be confident that it'll generate good code then maybe we can just use that everywhere.

compnerd · 2019-02-27T22:03:07Z

@mikeash - please do, since I just ran this on Linux (with some "hacks" - namely, added -O2 -g0 -S -o - - -DNDEBUG -U_DEBUG -USWIFT_RUNTIME_ENABLE_LEAK_CHECKER -USWIFT_ENABLE_RUNTIME_FUNCTION_COUNTERS and get this:

	.p2align	4, 0x90         # -- Begin function _ZL19_swift_allocObject_PKN5swift18TargetHeapMetadataINS_9InProcessEEEmm
	.type	_ZL19_swift_allocObject_PKN5swift18TargetHeapMetadataINS_9InProcessEEEmm,@function
_ZL19_swift_allocObject_PKN5swift18TargetHeapMetadataINS_9InProcessEEEmm: # @_ZL19_swift_allocObject_PKN5swift18TargetHeapMetadataINS_9InProcessEEEmm
	.cfi_startproc
# %bb.0:                                # %entry
	pushq	%rbx
	.cfi_def_cfa_offset 16
	.cfi_offset %rbx, -16
	movq	%rdi, %rbx
	movq	%rsi, %rdi
	movq	%rdx, %rsi
	callq	swift_slowAlloc
	movq	%rbx, (%rax)
	movq	$2, 8(%rax)
	popq	%rbx
	.cfi_def_cfa_offset 8
	retq
.Lfunc_end1:
	.size	_ZL19_swift_allocObject_PKN5swift18TargetHeapMetadataINS_9InProcessEEEmm, .Lfunc_end1-_ZL19_swift_allocObject_PKN5swift18TargetHeapMetadataINS_9InProcessEEEmm
	.cfi_endproc

mikeash · 2019-02-27T22:36:48Z

Here's what I get with placement new:

__ZL19_swift_allocObject_PKN5swift18TargetHeapMetadataINS_9InProcessEEEmm:
000000000033db80	pushq	%rbp
000000000033db81	movq	%rsp, %rbp
000000000033db84	pushq	%rbx
000000000033db85	pushq	%rax
000000000033db86	movq	%rdi, %rbx
000000000033db89	movq	%rsi, %rdi
000000000033db8c	movq	%rdx, %rsi
000000000033db8f	callq	_swift_slowAlloc
000000000033db94	movq	%rbx, (%rax)
000000000033db97	movq	$0x2, 0x8(%rax)
000000000033db9f	addq	$0x8, %rsp
000000000033dba3	popq	%rbx
000000000033dba4	popq	%rbp
000000000033dba5	retq

Looks great! Seems like we might as well use it everywhere now. I guess that comment must be obsolete.

swift-ci · 2019-02-27T23:02:35Z

Build failed
Swift Test OS X Platform
Git Sha - 377a5e8dee7172dd929a20c28291af98efbeca33

compnerd · 2019-02-27T23:25:46Z

Awesome! I'll upload a new version with that enabled globally!

The assertions here are based around the idea that `std::atomic` is trivially constructible which is not a guarantee that the standard fully provides. The default initialization of the `std::atomic` type may leave it in an undetermined state. These were caught using the Visual C++ preview runtime. Ideally, the object constructor would use a placement new operator. However, prior to C++17, the C++ standard mandated that there be a NULL pointer check in the placement new operator. This is something which is no longer the case with C++17. Switch to the placement new operator for C++17 and newer and enable that codepath for Windows as well (which seemingly elides the null-pointer check with clang-cl).

compnerd · 2019-02-27T23:29:40Z

@swift-ci please test

swift-ci · 2019-02-28T04:25:47Z

Build failed
Swift Test OS X Platform
Git Sha - c2af8f9437c6f18f2898f8d60e3460a94682a9e1

mikeash · 2019-02-28T16:41:40Z

Woohoo!

compnerd · 2019-02-28T17:15:18Z

@mikeash - okay to merge?

mikeash · 2019-02-28T17:16:40Z

Go for it!

lorentey reviewed Jan 3, 2019

View reviewed changes

stdlib/public/SwiftShims/GlobalObjects.h Show resolved Hide resolved

compnerd force-pushed the you-may-not-assert branch from e0e3b49 to 377a5e8 Compare February 8, 2019 03:48

compnerd force-pushed the you-may-not-assert branch from 377a5e8 to c2af8f9 Compare February 27, 2019 21:27

compnerd force-pushed the you-may-not-assert branch from c2af8f9 to 0983ea6 Compare February 27, 2019 23:29

compnerd merged commit 48d8ebd into swiftlang:master Feb 28, 2019

compnerd deleted the you-may-not-assert branch February 28, 2019 17:49

stdlib: remove some static assertions which may not hold #21606

stdlib: remove some static assertions which may not hold #21606

Uh oh!

Conversation

compnerd commented Jan 3, 2019

Uh oh!

compnerd commented Jan 3, 2019

Uh oh!

lorentey commented Jan 3, 2019

Uh oh!

Uh oh!

compnerd commented Feb 8, 2019

Uh oh!

swift-ci commented Feb 8, 2019

Uh oh!

jrose-apple commented Feb 8, 2019

Uh oh!

compnerd commented Feb 8, 2019

Uh oh!

mikeash commented Feb 8, 2019

Uh oh!

compnerd commented Feb 8, 2019

Uh oh!

jrose-apple commented Feb 8, 2019

Uh oh!

compnerd commented Feb 8, 2019

Uh oh!

jrose-apple commented Feb 9, 2019

Uh oh!

mikeash commented Feb 11, 2019

Uh oh!

compnerd commented Feb 11, 2019

Uh oh!

compnerd commented Feb 14, 2019

Uh oh!

compnerd commented Feb 25, 2019

Uh oh!

mikeash commented Feb 27, 2019

Uh oh!

compnerd commented Feb 27, 2019

Uh oh!

mikeash commented Feb 27, 2019

Uh oh!

compnerd commented Feb 27, 2019

Uh oh!

swift-ci commented Feb 27, 2019

Uh oh!

mikeash commented Feb 27, 2019

Uh oh!

compnerd commented Feb 27, 2019

Uh oh!

mikeash commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-ci commented Feb 27, 2019

Uh oh!

compnerd commented Feb 27, 2019

Uh oh!

compnerd commented Feb 27, 2019

Uh oh!

swift-ci commented Feb 28, 2019

Uh oh!

mikeash commented Feb 28, 2019

Uh oh!

compnerd commented Feb 28, 2019

Uh oh!

mikeash commented Feb 28, 2019

Uh oh!

Uh oh!

mikeash commented Feb 27, 2019 •

edited

Loading