aaron.ballman added a comment.

In D116966#3260571 <https://reviews.llvm.org/D116966#3260571>, @ArcsinX wrote:

> Friendly ping.
>
> I am not sure that this patch is clear, so I will try to explain it again:
>
> - clang static analyzer plugins are *NOT* a typical clang plugins: instead of 
> `llvm::Registry<>::Add` usage, these plugins provides 
> `сlang_registerCheckers` and `clang_analyzerAPIVersionString` functions which 
> are called from static analyzer. So, these plugins work ok without anything 
> special on Windows (but typical clang plugins are not and D18826 
> <https://reviews.llvm.org/D18826> was a try to fix it).
> - `PLUGIN_TOOL` together with `LLVM_EXPORT_SYMBOLS_FOR_PLUGINS` (introduced 
> in D18826 <https://reviews.llvm.org/D18826>) is a try to add support for a 
> typical clang plugins as a DLL on Windows. Thus, `PLUGIN_TOOL` does nothing 
> on non-Windows OS.
> - `PLUGIN_TOOL` does nothing without  `LLVM_EXPORT_SYMBOLS_FOR_PLUGINS=On`, 
> but in clang static analyzer plugins `LLVM_EXPORTED_SYMBOL_FILE`  file is 
> used to specify symbols we want to export (`сlang_registerCheckers` and 
> `clang_analyzerAPIVersionString`)
>
> Thus, for now the only thing `PLUGIN_TOOL` specification for clang static 
> analyzer plugins does is breaking MSVC build when 
> `LLVM_EXPORT_SYMBOLS_FOR_PLUGINS=On` (and plugins support enabled with 
> `-DCLANG_PLUGINS_SUPPORT=On -DLLVM_ENABLE_PLUGINS=On`).

Thanks for this! FWIW, I've been spending some time trying to figure out 
whether support is intended or not, and I've confirmed that we don't support 
plugins in general, but we do have some base level of support for them on 
Windows, depending on what the plugin does (it sounds like if you only call 
shared functions, you're likely okay, but once you try touching shared data 
objects, you're likely not okay). It sounds like you're in one of the 
situations that works well enough because it's only calling through function 
interfaces (and C interfaces at that). In light of that, this patch seems 
reasonable to me as it's another step towards better plugin support.

However, the changes you've made don't look to be specific to building on 
Windows; this removes `PLUGIN_TOOL` for all targets. I presume it's still 
needed for non-Windows targets, isn't it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116966/new/

https://reviews.llvm.org/D116966

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to