[clang] [clang] Fix loss of `dllexport` for exported template specialization (PR #93302)
Andrew Ng via cfe-commits
cfe-commits at lists.llvm.org
Tue May 28 09:16:19 PDT 2024
nga888 wrote:
> I'm not sure this is the right place for the fix though. Note that even though my patch added handling for dropping dllexport, we still do export `f` in this case:
>
> ```
> __declspec(dllexport) int f(int x);
> int user(int x) {
> return f(x);
> }
> int f(int x) { return 1; }
> ```
In your example, the "early" declaration of `f` already has `dllexport`. For the test case in this patch:
```
struct s {
template <bool b = true> static bool f();
};
template <typename T> bool template_using_f(T) { return s::f(); }
bool use_template_using_f() { return template_using_f(0); }
template<>
bool __declspec(dllexport) s::f<true>() { return true; }
```
It is the "late" specialization of the template that has `dllexport` and I think this is the main reason why `s::f<true>()` isn't consistently marked as `dllexport` in the AST. This in turn, then triggers the `dllexport` to be dropped by the code in your patch.
I understand that removing the code that drops `dllexport` may not be the "best" place to fix this issue but if this code is not actually required, which appears to be the case (please let me know if this is not), then removing the code to achieve "better" behaviour feels like a "win". The alternative would be to add more code/complexity to the AST handling. It already seems that there is some level of "entanglement" between the AST and codegen given the need for the code to remove `dllimport`.
One existing scenario where this patch's test case does result in the expected export is with the option `-fdelayed-template-parsing` which appears to still be the default for `clang-cl`. However, this is non-standard MSVC behaviour which I believe even MSVC is moving away from. Compiling with `clang-cl` using the `-permissive-` option, which disables delayed template parsing amongst other things, results in the same issue, i.e. the missing export. This patch also fixes this use case.
https://github.com/llvm/llvm-project/pull/93302
More information about the cfe-commits
mailing list