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

Backport fix for mangling of aliases #54103

Closed
nikic opened this issue Feb 27, 2022 · 8 comments
Closed

Backport fix for mangling of aliases #54103

nikic opened this issue Feb 27, 2022 · 8 comments

Comments

@nikic
Copy link
Contributor

nikic commented Feb 27, 2022

/cherry-pick 54b909d

@llvmbot
Copy link
Member

llvmbot commented Feb 27, 2022

/branch llvmbot/llvm-project/issue54103

@llvmbot
Copy link
Member

llvmbot commented Feb 27, 2022

/pull-request llvmbot#104

@tstellar
Copy link
Collaborator

@rnk What do you think about backporting this? 54b909d

@rnk
Copy link
Collaborator

rnk commented Feb 28, 2022

I actually think this is somewhat risky to backport. It has the potential to really change behavior, it changes alias symbols in object files.

I see the motivation is to use this in Rust, which I presume enables the similar function merging pass, which generates these kinds of aliases. Instead of backporting this, should Rust pre-adopt the strategy outlined in #54090 for getting rid of IR mangling? Rust could create functions, and then, for those three annoying calling conventions on i86-windows, rename them using Mangler::getNameWithPrefix, using the special escape.

Or we could go forward with the backporting. The impact here is really very limited to papering over a Windows oddity, and there isn't nearly as much exotic usage of aliases on Windows as there are for example in glibc for Linux and other OSs.

@nikic
Copy link
Contributor Author

nikic commented Mar 1, 2022

@Amanieu Thoughts on that? I see that you are already calling into the mangler for inline asm sym support, so I guess it wouldn't be much of a stretch to also explicitly mangle the function names themselves on our side?

@Amanieu
Copy link
Contributor

Amanieu commented Mar 1, 2022

Here is a reduced example of the Rust code that led to this bug. I encountered this in practice while working with generic fastcall functions which end up being called from a different codegen unit.

I think this should be backported since this is a real bug in LLVM that needs to be fixed either with my patch or by fixing MergeFunctions when it creates the alias.

Mangling the function names on our side would be awkward: we would need to first create the LLVM function with the unmangled name, call LLVMRustGetMangledName on that function and then change its name to \01 + the mangled name.

@rnk
Copy link
Collaborator

rnk commented Mar 1, 2022

Mangling the function names on our side would be awkward: we would need to first create the LLVM function with the unmangled name, call LLVMRustGetMangledName on that function and then change its name to \01 + the mangled name.

Yes, I agree, having to construct and rename the function is very awkward, but the proposal in #54090 is pretty close to that, it requires building the FunctionType and AttributeList and passing those to the IR Mangler to produce the function name. Would you find that too onerous?

I think, on reflection, we should just take this as internal LLVM tech debt, rather than having Rust adopt a temporary solution, and then change Rust again to call these new, yet-to-be-written APIs. We can revert 54b909d from LLVM if and when we get rid of IR calling convention mangling.

So, let's do the backporting.

@tstellar
Copy link
Collaborator

tstellar commented Mar 1, 2022

Merged: d245bcf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants