Page MenuHomePhabricator

Explicitly specify MSVC mangling of iostream globals
ClosedPublic

Authored by kastiglione on Aug 10 2016, 3:10 PM.

Details

Summary

With MS ABI, the char [] definitions of the iostream globals (iostream.cpp)
result in mangled symbol names that don't match the mangled symbol names of the
declarations (<iostream>).

For example, istream cin mangles to:

?cin@__1@std@@3V?$basic_istream@DU?$char_traits@D@__1@std@@@12@A

but its definition in iostream.cpp is char cin[sizeof(istream)], which
incorrectly mangles to:

?cin@__1@std@@3PADA

To work aound this issue, this change manually sets the symbol name using an
__asm__ label, for win32 only.

Event Timeline

kastiglione retitled this revision from to Explicitly specify MSVC mangling of iostream globals.
kastiglione updated this object.
kastiglione added a subscriber: llvm-commits.
kastiglione updated this object.Aug 10 2016, 4:02 PM

I don't think _WIN32 is the right guard because mingw defines that macro and it uses the Itanium mangling.

I'd use something like _MSC_VER.

If you want this to work with MSVC (instead of just clang-cl), you could use #pragma comment(linker, "/alternatename:foo=bar") to achieve similar results.

Thanks @majnemer, I'll update accordingly.

compnerd edited edge metadata.Jan 2 2017, 2:18 PM

What is the state of this ptach? @majnemer had a valid point, this should be updated with that.

kastiglione updated this revision to Diff 82835.Jan 2 2017, 8:44 PM
kastiglione edited edge metadata.

Change #ifdefs from _WIN32 to _MSC_VER

@majnemer, @compnerd: Using /alternatename will require the mangling of the definition be maintained in addition to the mangling of the declaration. I myself don't need MSVC compatibility for this, so if there are no proponents for a MSVC compatible change at this point, I'm inclined to stay with an __asm__ approach since /alternatename potentially makes this fix more fragile.

smeenai added a subscriber: smeenai.Jan 2 2017, 9:11 PM
EricWF edited edge metadata.Jan 4 2017, 10:44 PM

I don't think _WIN32 is the right guard because mingw defines that macro and it uses the Itanium mangling.

I'd use something like _MSC_VER.

Libc++ now has macros for the object format it's targeting. I think these changes should be guarded with #if defined(_LIBCPP_OBJECT_FORMAT_COFF) now.

src/iostream.cpp
18–19

Please guard these changes with #if defined(_LIBCPP_OBJECT_FORMAT_COFF) instead.

EricWF accepted this revision.Jan 4 2017, 10:50 PM
EricWF edited edge metadata.

LGTM minus inline coments.

@majnemer suggested another solution that would work with both MSVC and clang-cl but Libc++ is far from being able to support MSVC (due to #include_next), so this fix is good for now.

Unfortunately this is going to become more difficult to maintain once libc++ starts using different versioning namespaces. However we can cross that bridge when we come to it.

This revision is now accepted and ready to land.Jan 4 2017, 10:50 PM
smeenai added inline comments.Jan 4 2017, 11:01 PM
src/iostream.cpp
18–19

I don't think that'll be correct, since it's an ABI issue (Itanium vs Microsoft) not an object format issue. _MSC_VER isn't great either though, because for example you can compile with -target i686-windows-itanium -fmsc-version-1900 and still end up with _MSC_VER defined even though you're targeting the Itanium ABI. Idk how to check for the ABI specifically.

EricWF added inline comments.Jan 4 2017, 11:07 PM
src/iostream.cpp
18–19

Ah thanks for the correct. I added this was a limitation of the object format.

compnerd added inline comments.Jan 4 2017, 11:09 PM
src/iostream.cpp
18–19

A similar problem exists with the MS ABI RTTI implementation.

I think it may be high time to introduce a _LIBCPP_ABI_IA and _LIBCPP_ABI_MS. Thoughts?

EricWF added inline comments.Jan 4 2017, 11:20 PM
src/iostream.cpp
18–19

That makes sense to me. How do we detect what ABI we are targeting?

kastiglione edited edge metadata.

Switch from _MSC_VER to _LIBCPP_MSVC

smeenai added inline comments.Jan 6 2017, 10:10 AM
src/iostream.cpp
18

_LIBCPP_MSVC is definitely not correct. It's only defined for cl, not for clang (not even clang-cl), and cl doesn't even support __asm__.

kastiglione added inline comments.Jan 6 2017, 11:32 AM
src/iostream.cpp
18

Thanks, faulty misinterpretation of the macro logic.

Revert to checking _MSC_VER

This LGTM again. We're working on getting special builtin macros to detect which ABI is being used, but until then this patch seems correct.

@smeenai mentioned that cl doesn't support __asm__ so technically this patch is incorrect since cl defines _MSC_VER. However libc++ cannot use cl currently due to usages of other constructs it doesn't support, such as #include_next, so I'm not worried about that now.

@kastiglione do you have commit access?

@EricWF I don't have commit access. Would you mind committing?

cl doesn't support __asm__ so technically this patch is incorrect since cl defines _MSC_VER.

Is it worth adding && defined(__clang__)?

EricWF added a comment.Jan 6 2017, 2:03 PM

@EricWF I don't have commit access. Would you mind committing?

cl doesn't support __asm__ so technically this patch is incorrect since cl defines _MSC_VER.

Is it worth adding && defined(__clang__)?

Up to you. Once we update clang to provide builtin macros for ABI detection we'll have to change all of these guards anyway.

kastiglione updated this revision to Diff 83429.Jan 6 2017, 2:19 PM

Check for clang too

@EricWF I added the __clang__ check, why not. Thanks for the review.

EricWF added a comment.Jan 6 2017, 6:18 PM

@kastiglione do you need somebody to commit this for you?

@EricWF I do need someone to commit this, please and thanks

EricWF closed this revision.Jan 6 2017, 10:20 PM

r291337.