This is an archive of the discontinued LLVM Phabricator instance.

Update Microsoft name mangling scheme for exception specifiers in the type system
ClosedPublic

Authored by zahen on Dec 13 2018, 6:19 PM.

Details

Summary

The msvc exception specifier for noexcept function types has changed from the prior default of "Z" to "_E" if the function cannot throw when compiling with /std:C++17.

Diff Detail

Event Timeline

zahen created this revision.Dec 13 2018, 6:19 PM
zturner added inline comments.Dec 13 2018, 6:38 PM
lib/AST/MicrosoftMangle.cpp
2311–2314

I knew that the mangling changed whenever a pointer to a noexcept function is passed as an argument, and we don't yet handle that, but I'm surprised to hear that they changed an existing mangling, since it's a hard ABI break.

Do you know the major and minor version numbers that this changed in? I'd like to test it out for starters, but also since this is an ABI break we would need to put it behind -fms-compatibility-version and only mangle using the new scheme when the compatibility version is sufficiently high.

Ahh I read the new test cases, and all of the test cases are about the case where noexcept function is a parameter, so maybe this is actually the case I was referring to. Can you add a test case for something like this:

void f() noexcept { }

I get this:

00A 00000000 SECT4  notype ()    External     | ?foo@@YAXXZ (void __cdecl foo(void))
00B 00000010 SECT4  notype ()    External     | ?bar@@YAXP6AXX_E@Z (void __cdecl bar(void (__cdecl*)(void) noexcept))

Showing that it is in fact only for function parameters. So we should have a test to confirm that the behavior when the noexcept function is not a parameter remains Z (from the code it looks like this might be broken under this patch).

Also we still need to put this behind -fms-compatibility-version. Finally, it would be nice if you could also update the demangler (llvm/lib/Demangle/MicrosoftDemangle.cpp)

zahen added a comment.Dec 14 2018, 2:46 PM

Also we still need to put this behind -fms-compatibility-version. Finally, it would be nice if you could also update the demangler (llvm/lib/Demangle/MicrosoftDemangle.cpp)

This was introduced in 15.5 (1912). What's the preferred way to represent that in clang code?

lib/AST/MicrosoftMangle.cpp
2311–2314

It's only when a function is used as a type. My original rathole was trying to enumerate all of the places where that could be, but instead I settled on "everywhere but the initial definition". It's why false is passed in the 4th parameter on line 516.

I've confirmed this changed in 15.5 so I'll use that as the compat version.

rnk added inline comments.Dec 14 2018, 2:51 PM
lib/AST/MicrosoftMangle.cpp
2311–2314

I see existing code that uses this pattern: getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015)

The MSVCMajorVersion enum is symbolic, so I think you might have to multiply it by a hundred and modify LangOptions::isCompatibleWithMSVC to multiply by two fewer places.

I guess to fit with the existing enums we'd say MSVC2017_5, even though that conflates VS and VC version numbers.

BTW, as far as updating the demangler, as long as it doesn't crash or generate an error on a valid _E mangling, that should be sufficient (with a test). If you want bonus points you can make it print out noexcept when you see the _E, but I won't require it as it's a bit of extra work. If you decide not to do that, I'll just file a bug for it so that we don't forget.

lib/AST/MicrosoftMangle.cpp
2311–2314

Ok, I see it now. That should be fine.

zahen added a comment.Dec 14 2018, 3:15 PM

BTW, as far as updating the demangler, as long as it doesn't crash or generate an error on a valid _E mangling, that should be sufficient (with a test). If you want bonus points you can make it print out noexcept when you see the _E, but I won't require it as it's a bit of extra work. If you decide not to do that, I'll just file a bug for it so that we don't forget.

I have the full set of demangler updates ready to go and will upload shortly (Monday at the latest due to travel)

zahen updated this revision to Diff 178490.Dec 17 2018, 10:21 AM

Added support for msvc minor version as requested. Tied the updated mangling scheme to C++17 & compatibility mode > 1912 (15.5). Added additional tests.

rnk accepted this revision.Dec 17 2018, 1:01 PM

lgtm, thanks!

This revision is now accepted and ready to land.Dec 17 2018, 1:01 PM
zahen added a comment.Dec 17 2018, 1:05 PM

I don't have check-in permission, so I'd appreciate if someone could handle the actual commit.

This revision was automatically updated to reflect the committed changes.