Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only mangle the capability qualifier for pointers in the hybrid ABI #390

Closed
wants to merge 1 commit into from

Conversation

arichardson
Copy link
Member

In the purecap ABI the default pointer representation is capabilities, so
there is no need to add the qualifier to the name mangling. In fact this
can result in difficult-to-debug failures since e.g. version scripts will
no longer export the correct symbols. This problem was discovered in
libcxxrt: the type_info objects for pointer types (e.g. "char*"/"char* const")
were not exported from the purecap library, resulting in linker failures
for C++ programs that use RTTI.

Having all pointers mangled was useful to catch incorrect linker paths
before I added CHERI support to LLD since C++ programs would fail to link
due to missing symbols. However, we now emit errors when attempting to
link purecap and non-purecap libraries so this is no longer useful.

Since the capability mangling is now only used in hybrid mode, I also
changed the mangling to U12__capability:
void test(void *__capability) now mangles as _Z4testU12__capabilityPv
instead of _Z4testU3capPv and llvm-cxxfilt demangles it to
test(void* __capability) instead of test(void* cap).

This should be a better fix than CTSRD-CHERI/cheribsd#421

This is a flag day for all purecap C++ code but we should't have too much of it. Not sure if we should combine that with other flag days?

In the purecap ABI the default pointer representation is capabilities, so
there is no need to add the qualifier to the name mangling. In fact this
can result in difficult-to-debug failures since e.g. version scripts will
no longer export the correct symbols. This problem was discovered in
libcxxrt: the type_info objects for pointer types (e.g. "char*"/"char* const")
were not exported from the purecap library, resulting in linker failures
for C++ programs that use RTTI.

Having all pointers mangled was useful to catch incorrect linker paths
before I added CHERI support to LLD since C++ programs would fail to link
due to missing symbols. However, we now emit errors when attempting to
link purecap and non-purecap libraries so this is no longer useful.

Since the capability mangling is now only used in hybrid mode, I also
changed the mangling to U12__capability:
`void test(void *__capability)` now mangles as _Z4testU12__capabilityPv
instead of _Z4testU3capPv and llvm-cxxfilt demangles it to
`test(void* __capability)` instead of `test(void* cap)`.
Copy link
Member

@brooksdavis brooksdavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally makes sense. I've not reviewed the contents of the change.

I've occasionally thought that it might be useful to be able to call pure-cap functions from hybrid code and this would break that, but I think I'm OK with that being a different ABI if that ever turns out to be useful.

@brooksdavis
Copy link
Member

I think it's fine to just have a flag day on the dev branch. We don't have that much C++ code.

@jrtc27
Copy link
Member

jrtc27 commented Apr 9, 2020

This generally makes sense. I've not reviewed the contents of the change.

I've occasionally thought that it might be useful to be able to call pure-cap functions from hybrid code and this would break that, but I think I'm OK with that being a different ABI if that ever turns out to be useful.

Some kind of attribute/pragma/extern (like extern "C") could be added to solve that problem without the need for a whole new ABI.

@arichardson
Copy link
Member Author

This has been merged to dev.

@arichardson arichardson deleted the new-cap-mangling branch April 23, 2020 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants