Skip to content

[C++20] [Modules] The '#pragma comment(lib, ...)' and '#pragma detect_mismatch' directives in named modules shouldn't leak #61783

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

Closed
ChuanqiXu9 opened this issue Mar 29, 2023 · 7 comments
Labels
clang:modules C++20 modules and Clang Header Modules wontfix Issue is real, but we can't or won't fix it. Not invalid

Comments

@ChuanqiXu9
Copy link
Member

ChuanqiXu9 commented Mar 29, 2023

Reproducer:

// mod.cppm
module;

#pragma comment(lib, "msvcprt.lib")

export module mod;

and

// user.cpp
import mod;

now the IR for user.cpp will contain the metadata !0 = !{!"msvcprt.lib"} which is leaked from another translation unit mod.cppm. This is incorrect. The root cause of the problem is that EagerlyDeserializedDecls is designed for headers and it didn't take care for named modules. We need to redesign a new mechanism for named modules.

@ChuanqiXu9 ChuanqiXu9 added the clang:modules C++20 modules and Clang Header Modules label Mar 29, 2023
@llvmbot
Copy link
Member

llvmbot commented Mar 29, 2023

@llvm/issue-subscribers-clang-modules

@dwblaikie
Copy link
Collaborator

dwblaikie commented Mar 29, 2023

now the IR for user.cpp will contain the metadata !0 = !{!"msvcprt.lib"} which is leaked from another translation unit mod.cppm.

I'm not sure that's incorrect, though - you could have the object implementation of the module in that .lib file & this would let you auto-link to the library implementation?

@ChuanqiXu9
Copy link
Member Author

now the IR for user.cpp will contain the metadata !0 = !{!"msvcprt.lib"} which is leaked from another translation unit mod.cppm.

I'm not sure that's incorrect, though - you could have the object implementation of the module in that .lib file & this would let you auto-link to the library implementation?

I think it is super weird that the pragma's semantics leak to other TUs. And it just works surprisingly for the #pragma comment example due to the properties of the linkers.

@Ivan171
Copy link

Ivan171 commented Mar 31, 2023

It is like the pragma is being exported somehow. That shouldn't happen in my opinion. Only the module that specified the #pragma comment(lib, "msvcprt.lib") should have the /DEFAULTLIB metadata.

@dwblaikie
Copy link
Collaborator

now the IR for user.cpp will contain the metadata !0 = !{!"msvcprt.lib"} which is leaked from another translation unit mod.cppm.

I'm not sure that's incorrect, though - you could have the object implementation of the module in that .lib file & this would let you auto-link to the library implementation?

I think it is super weird that the pragma's semantics leak to other TUs. And it just works surprisingly for the #pragma comment example due to the properties of the linkers.

Is there something in my example that doesn't hold/make sense though?

Like I /think/ you can build an ABI compatible implementation of a C++ module, so clients don't have to build object files from modules themselves, maybe? If you can do that, I think it makes a lot of sense for a pragma link directive in a module to impact clients of the module to cause the library to be linked in.

I guess: when would you /not/ want this behavior? (like, you used the module, if the module has the pragma link, well your use of the module links in the implementation object file which then causes the pragma link to happen... )

@ChuanqiXu9
Copy link
Member Author

I guess: when would you /not/ want this behavior? (like, you used the module, if the module has the pragma link, well your use of the module links in the implementation object file which then causes the pragma link to happen... )

I don't feel good about the behavior since it looks like an implicit linking information, which smells bad. We should prefer explicit information.

And a bad example in my mind is that we have two modules (a and b) and two corresponding library ('A' and 'B'). And we have #pragma comment(lib, "C") in a. Then let's import 'a' in 'b'. And libA is a dynamic library. So ideally library 'B' should only link library A. But with the current behavior, the library 'B' may add library 'C' to its dependencies.

Also according to the doc (https://learn.microsoft.com/en-us/cpp/preprocessor/comment-c-cpp?view=msvc-170), it looks like there are other types of comments. So may be there are other worse examples. So I think we shouldn't export things which is not explicitly exported by the named modules.

How do you think about this?

@ChuanqiXu9
Copy link
Member Author

ChuanqiXu9 commented Apr 4, 2023

Just now I had a chat with a MSVC's developer. He tells me that the #pragmas in MSVC will leak the #pragma to the users. Also it is still very odd to me. I think it may be better to be consistent with MSVC for the issue since the #pragma comment is a MSVC's extension. Also the #pragma is not a standard thing. So the compilers have some space for this.

@EugeneZelenko EugeneZelenko added the wontfix Issue is real, but we can't or won't fix it. Not invalid label Apr 4, 2023
@EugeneZelenko EugeneZelenko closed this as not planned Won't fix, can't repro, duplicate, stale Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:modules C++20 modules and Clang Header Modules wontfix Issue is real, but we can't or won't fix it. Not invalid
Projects
None yet
Development

No branches or pull requests

5 participants