psionic12 added a comment. > Right, I understand (a little bit, at least!) what plugins *can* do. What I'm > asking specifically is: this feature has a cost, how important is supporting > it? Are there codebases where these attributes are widely used, and > enforcement at development time is particularly valuable?
Well, I can't tell you how widely plugin used, because they often used in third-party projects. Firefox team seems use them a lot, the `ParsedAttrInfo` plugin implementation seems write by them if I remember right. And our team use both plugins and clangd a lot, that's why I'm willing to contribute to make them work together. As about the importance, my personal opinion is that since there's a feature, we should try to polish it... > Sorry, I probably should have said "can plugins be thread-hostile". > Clangd runs several parsing actions in parallel, each on a single separate > thread. > If a plugin assumes there's only a single instance of it running and uses > static data accordingly, we're going to hit problems in clangd. > The problem is that precisely because clang uses a single thread, (some) > plugins may think this is a reasonable assumption. And it's not possible to > tell "from the outside" whether a given plugin is unsafe in this way. That seems a serious problem, but I got a question, what if the clang itself declared some static data (I'll investigate later, just ask for now)? > It looks like this patch moves `LoadLibraryPermanently` to a different > location, per the patch description clangd does not currently load plugins. > That's why I'm concerned this patch may introduce unsafe loading of untrusted > .sos. Well, since it called plugin, it's users choose to use it or not, I personally think we can't check if an `.so` is trusted, and neither should we care... > SkipFunctionBodies is an option in FrontendOpts that produces a different AST > that clang itself (which does not set this option) can't produce. Basically > I'm saying "can we tell if a plugin is going to work with our weird ASTs, or > do we just have to try and maybe crash". I'll test this. > - the filesystem access to the libraries isn't virtualized (vfs) and I don't > know if it really can be. Sorry I still can't understand this, could you help to explain more? What I mean before is that since this part of code is used in clang driver as well, it should be no problem get called by clangd. >>> - consequences of bad behavior/crash are worse, because clangd is >>> long-running >> >> Does clangd shares the same process with an `FrontendAction`? Because in >> clang, if a plugin carshes, the whole process does terminated, clang didn't >> handle this yet(just a core dump). > > Yes. And I'm not convinced it's possible to handle this safely without large > architectural changes. Well, I guess currently I can do nothing to improve this, but if a user uses a plugin, it crashed, that's should be the plugin's problem... > Absent that, I think we probably shouldn't enable them (beyond an experiment). How about capsule the loading functionalities to a function, and call it from clangd if experimental feature is checked? In this way nothing changed if the clangd's experimental feature is off. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92155/new/ https://reviews.llvm.org/D92155 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
