This is an archive of the discontinued LLVM Phabricator instance.

[index] Generate class & metaclass manglings for objc
ClosedPublic

Authored by kastiglione on Sep 10 2017, 2:23 PM.

Details

Summary

ObjC classes have two associated symbols, one for the class and one for the
metaclass.

This change overloads CodegenNameGenerator::getAllManglings to produce both
class and metaclass symbols.

While this function is called by clang_Cursor_getCXXManglings, it's only
called for CXXRecordDecl and CXXMethodDecl, and so libclang's behavior is
unchanged.

Event Timeline

kastiglione created this revision.Sep 10 2017, 2:23 PM

Fix string type conversion

lib/Index/CodegenNameGenerator.cpp
80

this lambda returns a StringRef constructed from a SmallString variable (on stack) => StringRef outlives the owning variable - looks like a bug

to me this looks reasonable, although i'm probably not qualified to sign off, so i would wait for @abdulras and @arphaman.
The only comment i have on my mind - it would be good to add a test.

kastiglione marked an inline comment as done.Sep 10 2017, 3:12 PM
kastiglione added inline comments.
lib/Index/CodegenNameGenerator.cpp
80

thanks, I ran into this. I fixed it in an update.

kastiglione marked an inline comment as done.Sep 10 2017, 3:14 PM
kastiglione added inline comments.
lib/Index/CodegenNameGenerator.cpp
80

The SmallString is converted to a StringRef with .str(), and then that StringRef is implicitly converted to a std::string which of course allocates memory.

lib/Index/CodegenNameGenerator.cpp
80

yeah, my comment was referring to the very first revision, now this looks correct

For testing, I'm looking for suggestions. For test/Index, the tests use the c-index-test binary, which uses libclang, which in turn uses this file. However as mentioned in the description, this new functionality isn't exposed by libclang and so in turn isn't exposed by c-index-test. One way to solve this is to add a new function to libclang (clang_Cursor_getObjCManglings) but is that polluting an API when nobody is actually asking for that function?

lib/Index/CodegenNameGenerator.cpp
80

thanks for catching it!

arphaman edited edge metadata.Sep 11 2017, 3:28 AM

You should be able to test it using a unittest if this is not exposed in the C API.

compnerd requested changes to this revision.Sep 11 2017, 9:13 AM
compnerd added a subscriber: compnerd.
compnerd added inline comments.
lib/Index/CodegenNameGenerator.cpp
77–78

I think it could be nicer to split this into a separate check:

if (ClassName.empty())
  return {};
87–88

This isn't correct. This works for the macosx, macosx-fragile, ios, and watchos runtimes. However, it is incorrect for gcc, gnustep, and objfw. The latter use _OBJC_CLASS_ and _OBJC_METACLASS_ as the prefixes. Please consult the value for -fobjc-runtime when constructing the mangling.

This revision now requires changes to proceed.Sep 11 2017, 9:13 AM
kastiglione edited edge metadata.

Handle GNU symbols

kastiglione marked 2 inline comments as done.Sep 12 2017, 10:22 AM

Please add tests for the mangling.

lib/Index/CodegenNameGenerator.cpp
176

No need for the else.

Add tests via c-index-test/libclang

kastiglione marked an inline comment as done.Sep 21 2017, 2:39 PM
compnerd accepted this revision.Sep 21 2017, 4:57 PM
This revision is now accepted and ready to land.Sep 21 2017, 4:57 PM
This revision was automatically updated to reflect the committed changes.