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

[arm64ec] Functions dllimport'ed from dlls have inconsistent address #93910

Open
efriedma-quic opened this issue May 31, 2024 · 8 comments
Open

Comments

@efriedma-quic
Copy link
Collaborator

See https://developercommunity.visualstudio.com/t/Functions-dllimported-from-arm64ec-dlls/10670642 ; the same issue reproduces compiling with clang-cl. My best explanation for what's happening is that original DLL gets the address of the actual function, but the dllimport is a hybrid_patchable stub the linker implicitly creates.

I'm not sure whether this is a compiler bug, or a linker bug. I'll update when I have more information.

CC @dpaoliello @cjacek

@cjacek
Copy link
Contributor

cjacek commented May 31, 2024

I think it's a linker bug, although I assume it's intentional in MSVC. I noticed that problem when implementing exports in LLD. The DLL f code will use the unmangled symbol for the function address, which is (by default) an alias to the mangled EC symbol. When linker exports such function, it creates a new "EXP+" symbol pointing to a generated x64 thunk, but leaves other symbols alone. A code importing such function will see the "EXP+" symbol for the function address, causing the mismatch.

Using __declspec(hybrid_patchable) for functions like that, where it matters, is a possible workaround. If f from the example was hybrid patchable, it would consistently use "EXP+" symbol (DLL-exported or not). The main downside is that it's an additional porting complication and those cases may be hard to identify. It also has performance implications. It should, however, work with existing MSVC versions and hopefully with LLVM soon.

A better solution would require a linker change. It could override unmangled alias in linker when creating the export thunk. Linker already has an extra step for EC exports where it needs to understand symbol mangling. It should be easy to extend it to detect unmangled->mangled aliases and replace the unmangled symbol, similar to how hybrid patchable functions do it in the compiler. I believe that this would transparently fix such cases. I experimented with something like that for LLD at some point, I will make a draft of such feature later to show what I mean.

@llvmbot
Copy link
Member

llvmbot commented May 31, 2024

@llvm/issue-subscribers-lld-coff

Author: Eli Friedman (efriedma-quic)

See https://developercommunity.visualstudio.com/t/Functions-dllimported-from-arm64ec-dlls/10670642 ; the same issue reproduces compiling with clang-cl. My best explanation for what's happening is that original DLL gets the address of the actual function, but the dllimport is a hybrid_patchable stub the linker implicitly creates.

I'm not sure whether this is a compiler bug, or a linker bug. I'll update when I have more information.

CC @dpaoliello @cjacek

@rnk
Copy link
Collaborator

rnk commented May 31, 2024

I clicked through and I see Qt has some practical reliance on this property, but while C++ requires the address of a function to be unique across the program, I generally recommend that portable applications avoid relying on this guarantee. MSVC ICF breaks it, making two distinct pointers compare equal, and this has been a recurring portability issue for CUDA kernel wrappers.

If, in your example, you remove the dllimport attributes and rely on the linker to synthesize thunks, you'll get the same behavior you observe with the dllimport attribute on ARM64EC. It is similarly easy to get into this situation on ELF with things like -fvisibility-inlines-hidden and -Bsymbolic-functions.

@efriedma-quic
Copy link
Collaborator Author

Using __declspec(hybrid_patchable) for functions like that, where it matters, is a possible workaround.

I tried that with the testcase... and it fails to link, at least with MSVC. Not sure if I'm making some silly mistake. (Haven't tried with your LLVM patch.)

while C++ requires the address of a function to be unique across the program, I generally recommend that portable applications avoid relying on this guarantee

I don't disagree that various compiler options can break function pointer uniqueness; but targets generally have some set of compiler options that preserves the guarantee, and I don't think Qt developers are going to be enthusiastic about rewriting their API.

@efriedma-quic
Copy link
Collaborator Author

Microsoft replied on the Developer Community report... they say this is behaving as intended.

@cjacek
Copy link
Contributor

cjacek commented Jun 3, 2024

I tried that with the testcase... and it fails to link, at least with MSVC. Not sure if I'm making some silly mistake. (Haven't tried with your LLVM patch.)

Ah, right, it's broken like that. It's specific to dllexport attribute, exporting via a command line or a .def file should work (but that makes the solution even less practical). FWIW, as I mentioned in !92965, we could make it work, at least with clang with something like cjacek@4febef2.

Microsoft replied on the Developer Community report... they say this is behaving as intended.

I was afraid it will be like that... I'd still call it a missed opportunity to make the EC more transparent than it is. I drafted a solution I mentioned earlier here: cjacek@82ed696 (on top of my other LLD patches). With that, linking your example unmodified outputs two the same pointers. We can't do that by default for compatibility reasons, but maybe as an opt-in feature if there will be a demand?

Another workaround is skipping x64 thunks for exports entirely. They are not crucial to make binaries work. If you skip declspec in your test case and export f with "-export:f,DATA", both MSVC and LLD will produce a working DLL (and a broken importlib). Maybe we could have an option in LLD that would just disable them.

@efriedma-quic
Copy link
Collaborator Author

Quick update for anyone following this:

@efriedma-quic
Copy link
Collaborator Author

The hybrid_patchable bug is supposedly fixed in the latest MSVC preview; see https://developercommunity.visualstudio.com/t/__declspechybrid_patchable-__declspec/10714809 .

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

5 participants