This is an archive of the discontinued LLVM Phabricator instance.

libc++/win: Make once_flag have the same size as a pointer
ClosedPublic

Authored by thakis on Mar 20 2019, 11:29 AM.

Details

Summary

unsigned long is 32-bit on 32-bit systems and 64-bit on 64-bit systems
on LP64 systems -- which most Unix systems are, but Windows isn't.
Windows is LLP64, which means unsigned long is 32-bit even on 64-bit
systems.

pplwin.h contains

static_assert(alignof(void *) == alignof(::std::once_flag), ...)

which fails due to this problem.

Instead of unsigned long, use uintptr_t, which consistently is 32-bit
on 32-bit systems and 64-bit on 64-bit systems.

Only do this when targeting the Windows ABI or when ABI
stability isn't required, since the type of once_flag::state_ gets
mangled into the exported symbol
call_once.

No functional change except on 64-bit Windows.

Diff Detail

Event Timeline

thakis created this revision.Mar 20 2019, 11:29 AM
EricWF requested changes to this revision.Mar 20 2019, 12:49 PM

Can you add a description of why this is a problem on Windows?

libcxx/include/mutex
583

Because this change is ABI breaking and can't be done unconditionally, I would create a typedef _StateType that's uintptr_t on Windows and unsigned long everywhere else.

653

This change is ABI breaking because uintptr_t isn't always unsigned long. On 32 bit platforms it may be unsigned int, which mangles differently.

This revision now requires changes to proceed.Mar 20 2019, 12:49 PM
thakis updated this revision to Diff 191565.Mar 20 2019, 1:15 PM
thakis marked an inline comment as done.
thakis edited the summary of this revision. (Show Details)

update

thakis marked an inline comment as done.Mar 20 2019, 1:16 PM

Done, thanks for catching that. As discussed on chat, the patch description already had a description on why this is a problem :)

EricWF added inline comments.Mar 20 2019, 1:41 PM
libcxx/include/mutex
579

I don't see value changing this in the unstable ABI.

EricWF accepted this revision.Mar 20 2019, 2:00 PM

Otherwise, after addressing inline comments, this LGTM.

This revision is now accepted and ready to land.Mar 20 2019, 2:00 PM
thakis marked 2 inline comments as done.Mar 20 2019, 3:22 PM

Thanks, landing with UNSTABLE part removed.

libcxx/include/mutex
579

I figured then it'll use the same type everywhere eventually, but removed ABI_UNSTABLE again.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 20 2019, 3:54 PM
ruiu added a subscriber: ruiu.Mar 21 2019, 1:54 PM
ruiu added inline comments.
libcxx/trunk/include/mutex
579–583 ↗(On Diff #191597)

Did you have to use if defined? Looks like you can always use uintptr_t.

EricWF added inline comments.Mar 21 2019, 6:57 PM
libcxx/trunk/include/mutex
579–583 ↗(On Diff #191597)

Yes. Because although uintptr_t may always have the same size and alignment as unsigned long on non-windows platforms, it may not produce the same mangling. And we have symbols in the dylib that depend on the mangling.

We (MSVC++) could just fix pplwin.h to not have this assumption, since clang doesn't need the workaround that forces us to reinterperet_cast void*, but that fix probably wouldn't make it shipped until VS 2019 Update 1 at the earliest (I'd have to go through ship room) and Update 2 more likely.

Comments in the other reivew about "it's odd that you're trying to use libc++ but linking with msvc++'s STL DLL, msvcp140.dll", still apply here :)

thakis marked an inline comment as done.Mar 22 2019, 6:30 AM

We (MSVC++) could just fix pplwin.h to not have this assumption, since clang doesn't need the workaround that forces us to reinterperet_cast void*, but that fix probably wouldn't make it shipped until VS 2019 Update 1 at the earliest (I'd have to go through ship room) and Update 2 more likely.

Comments in the other reivew about "it's odd that you're trying to use libc++ but linking with msvc++'s STL DLL, msvcp140.dll", still apply here :)

I think this is a good change regardless: it didn't look intentional that once_flag::__state_ was 64-bit on 64-bit systems everywhere except on Windows. It seems someone just wasn't thinking of LLP64 / LP64.

I think this is a good change regardless:

But it looks like it's used as a single bit here, so saving 4 bytes is a win, right?