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

Add missing prefixes when checking if function is in std namespace #1222

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

nilsfriess
Copy link
Collaborator

According to the Itanium ABI spec certain classes in the std namespace have their own abbreviations and do not start with St so the current check would not mark them as std functions. However, I am not sure if these abbreviations are actually used by the LLVM Itanium mangler. I haven't looked at the source code yet but I looked at the symbols of some test applications using nm and all mangled names of std:: functions started with _ZSt, _ZNSt or _ZNKSt (this last one is for const member functions and was missing previously), so maybe it is enough to just check for these three.

Opening this as a draft for now until I'm sure which is the correct approach (hoping that someone who knows more about this sees this and can comment on it).

According to
https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling-compression
certain classes in the `std` namespace have their own abbreviations and
do not start with `St`.
Copy link
Collaborator

@illuhad illuhad left a comment

Choose a reason for hiding this comment

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

Good catch. Even if it is a draft now, I wonder if this should go into the 23.10 release as a fix. What do you think?

src/compiler/stdpar/MallocToUSM.cpp Outdated Show resolved Hide resolved
@nilsfriess
Copy link
Collaborator Author

I opened #1224 which only contains the additional check for const member functions. I'm quite sure that this is actually missing, so I think we can safely include #1224 in the release but should do more research about the other checks included in this PR.

Co-authored-by: Aksel Alpay <aksel.alpay@uni-heidelberg.de>
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

2 participants