This is an archive of the discontinued LLVM Phabricator instance.

Allow both MSVC and Itanium mangling schemes.
ClosedPublic

Authored by chaoren on May 26 2015, 2:35 PM.

Details

Summary

LLDB on Windows should now be able to demangle Linux/Android symbols.

Also updated CxaDemangle.cpp to be compatible with MSVC.

Depends on D9949, D9954, D10048.

Diff Detail

Repository
rL LLVM

Event Timeline

chaoren updated this revision to Diff 26538.May 26 2015, 2:35 PM
chaoren retitled this revision from to Allow both MSVC and Itanium mangling schemes..
chaoren updated this object.
chaoren edited the test plan for this revision. (Show Details)
chaoren added reviewers: zturner, clayborg.
chaoren added a subscriber: Unknown Object (MLST).
zturner added inline comments.
source/Core/CxaDemangle.inc
1–17 ↗(On Diff #26538)

Unless we have a specific reason not to (which nobody has spoken up about yet), I would prefer for this to be a standalone .cpp file which compiles to an object file, instead of a .inc file. I can't think of any reason not to do this, but +greg and ed maste in case there is something about their platforms which would necessitate it being an inline file.

source/Core/Mangled.cpp
16–20 ↗(On Diff #26538)

Is there a reason why we didn't need these defines before but we need them now?

In any case, llvm has LLVM_NOEXCEPT, LLVM_CONSTEXPR, and LLVM_ALIGNAS, which all just do the right thing depending on the platform. Seems like we should use those instead.

59–66 ↗(On Diff #26538)

This function is going to get a "Not all control paths return a value" warning. Can you fix that?

chaoren added inline comments.May 26 2015, 3:10 PM
source/Core/CxaDemangle.inc
1–17 ↗(On Diff #26538)

I think it's for the same reason that we're putting everything here in an anonymous namespace. To prevent collision with a system defined cxa_demangle. I guess we could also solve that problem by putting everything under lldb_private.

4849 ↗(On Diff #26538)

Do you know the rebind, construct, and destroy are required for allocators in the STL standard, or it's just a MSVC quirk? If it's the former I think we should try to upstream it to libcxxabi, and avoid changing it here.

source/Core/Mangled.cpp
16–20 ↗(On Diff #26538)

What do you mean by "before"? The builtin demangler wasn't enabled for MSVC. I'll try to upstream the LLVM_* change to libcxxabi, so we don't have to change them in our copy.

zturner edited edge metadata.May 26 2015, 3:15 PM

I mean I see
+#define noexcept
+#define constexpr
+#define snprintf _snprintf
+#define alignas(X) __declspec(align(X))

but I dont' see corresponding -'s from anywhere else in the diff. So it
looks like this is new code, not moved from somewhere else. Unless I'm
just missing something. But now that I think about it, it looks like it's
just because all this was previously compiled out on Windows, and now it's
being compiled on Windows.

Since we've inlined a copy of the code already anyway, personally I don't
see anything wrong with making small changes like this just to get it to
compile on all platforms. I mean I wouldn't think it wise to change some
of the actual logic of the demangling, but just adding decorators on
functions to get it to compile would be ok I would think? Others may
disagree though :)

Yeah, the the entire thing didn't compile on Windows before. Virgile Bello
tried to add these in r205769 but was reverted by emaste in r205776 since
it broke FreeBSD. I put these here because I think it's nicer to have
minimal changes to the cxa_demangler copy.

Well the thing is that in VS 2015 some of those features will start
working, like constexpr. It would be nice to have those C++ features start
working automatically once the LLVM macros are changed to reflect the new
compiler versions.

clayborg requested changes to this revision.May 26 2015, 3:50 PM
clayborg edited edge metadata.

We should make a .cpp + .h file instead of a .inc file.

This revision now requires changes to proceed.May 26 2015, 3:50 PM
chaoren updated this revision to Diff 26616.May 27 2015, 11:10 AM
chaoren edited edge metadata.

Addressed review comments.

chaoren updated this object.May 27 2015, 11:11 AM
chaoren edited edge metadata.
emaste edited edge metadata.May 27 2015, 12:47 PM

Well the thing is that in VS 2015 some of those features will start
working, like constexpr. It would be nice to have those C++ features start
working automatically once the LLVM macros are changed to reflect the new
compiler versions.

Sounds good to me. It wasn't just FreeBSD broken originally but probably anyone using clang (or any non-MSVC perhaps).
I think I was just the first to encounter the build failure.

source/Core/Mangled.cpp
64 ↗(On Diff #26616)

I'm personally not a fan of the implicit bool cast (here and below); do we already use this approach elsewhere?

chaoren updated this revision to Diff 26639.May 27 2015, 3:07 PM
chaoren edited edge metadata.
  • Address review & remove illegal keyword redefinition.
clayborg accepted this revision.May 28 2015, 9:39 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.May 28 2015, 9:39 AM
This revision was automatically updated to reflect the committed changes.