Page MenuHomePhabricator

Add AddressSpace mangling to MS mode
ClosedPublic

Authored by erichkeane on Dec 14 2018, 11:10 AM.

Details

Summary

All of the symbols demangle on llvm-undname and demangler.com. This
address space qualifier is useful for when we want to use opencl C++ in
Windows mode. Additionally, C++ address-space using functions will now
be usable on windows.

Diff Detail

Repository
rL LLVM

Event Timeline

erichkeane created this revision.Dec 14 2018, 11:10 AM

It would be interesting to teach llvm-undname to demangle this more naturally. Right for this mangling: ?f1@@YAXPEAU?$AS_@$00$$CBD@__clang@@@Z I see this demangling: void __cdecl f1(struct __clang::AS_<1, char const> *). That's an accurate representation of the underlying structure, but it would be cool if we had special cases for things in the __clang namespace, so that we could display this as void __cdecl f1(char __attribute__((address_space(1))) const *) {}
`

rnk added inline comments.Dec 14 2018, 11:27 AM
test/CodeGenCXX/mangle-address-space.cpp
7 ↗(On Diff #178256)

You know, now that we have a demangler, I wonder if we shouldn't add a RUN line like this to test that these names really do demangle properly:
// RUN: %clang_cc1 %s ... -emit-bitcode -o - | llvm-nm -demangle - | FileCheck %s --check-prefix=DEMANGLED

test/CodeGenOpenCL/address-spaces-mangling.cl
7 ↗(On Diff #178256)

I would replace -triple x86_64-windows-pc here and everywhere with -triple x86_64-windows-itanium (or gnu instead of itanium) to test the Itanium mangling on Windows. I don't think x86_64-windows-pc parses into a real triple. After all, "pc" is parsed as the vendor, which is supposed to be second.

24 ↗(On Diff #178256)

Why "MS_"? This isn't the MSVC environment, this is using the Itanium C++ ABI mangling. Maybe WIN_ instead would be better?

erichkeane marked 4 inline comments as done.Dec 14 2018, 11:31 AM
erichkeane added inline comments.
test/CodeGenCXX/mangle-address-space.cpp
7 ↗(On Diff #178256)

I'm not sure how that would work, but I can take a look.

test/CodeGenOpenCL/address-spaces-mangling.cl
7 ↗(On Diff #178256)

Woops! I didn't mean to check this file in. It has some other nasty problems that need to be fixed up, so I tested this feature in the mangle-address-space. I'll still do this above.

erichkeane marked 2 inline comments as done.Dec 14 2018, 11:36 AM
erichkeane added inline comments.
test/CodeGenOpenCL/address-spaces-mangling.cl
7 ↗(On Diff #178256)

@rnk I tried this, but it changes off of the MicrosoftMangler and switches back to the Itanium mangling. Is there a better triple for testing microsoft mangling?

majnemer added inline comments.Dec 14 2018, 11:44 AM
lib/AST/MicrosoftMangle.cpp
1806–1836 ↗(On Diff #178256)

Don't these need to be _AS_Foobar to avoid collisions with normal code?

rnk added inline comments.Dec 14 2018, 11:53 AM
lib/AST/MicrosoftMangle.cpp
1806–1836 ↗(On Diff #178256)

I think we're in __clang::, so it's OK? In any case, it's what's done for Itanium, IIUC. It's also not like we have to worry about conflicts with user macros.

test/CodeGenOpenCL/address-spaces-mangling.cl
7 ↗(On Diff #178256)

Tacking on "-msvc" or "-itanium" or "-gnu" is the more explicit way to the C++ ABI. I guess I was confused, it looks like this file expects Itanium manglings from the -pc triple.

erichkeane marked 2 inline comments as done.Dec 14 2018, 1:57 PM
erichkeane added inline comments.
lib/AST/MicrosoftMangle.cpp
1806–1836 ↗(On Diff #178256)

Right, we're in __clang (thus cannot conflict), and cannot be replaced with macros or anything (since this is a mangling name), so I opted to save the char instead.

However, I'll make it _AS_CU<whatever> just cause.

test/CodeGenOpenCL/address-spaces-mangling.cl
7 ↗(On Diff #178256)

Ah, yeah, I should have figured that out. I was originally looking to do the opencl-c++ test in this file but it has some bigger problems that cause issues (so I never finished it).

Somehow it stayed in my workspace when I stashed.

I'll update this whole patch as soon as my build passes.

Should catch me up on all comments except @zturner's llvm-undname feature request :)

rnk accepted this revision.Dec 14 2018, 3:02 PM

lgtm

test/CodeGenCXX/mangle-address-space.cpp
7 ↗(On Diff #178256)

Any thoughts on this? I see, llvm-nm -demangle doesn't call the microsoft demangler.

This revision is now accepted and ready to land.Dec 14 2018, 3:02 PM
erichkeane marked an inline comment as done.Dec 14 2018, 3:15 PM
erichkeane added inline comments.
test/CodeGenCXX/mangle-address-space.cpp
7 ↗(On Diff #178256)

I wasn't able to find a way that would call this. Also, some of my test machines don't seem to default build llvm-undname always (which would be required for the windows ones).

This revision was automatically updated to reflect the committed changes.