Thread: [RFC] Clang plugin for catching suspicious typedef casting
Hi,
In the commit [1] one side change was to fix mixed up usage of
BlockNumber and Buffer variables. This was a one-off manual effort, and
indeed it hardly seems possible to get compilation warnings for such
code, as long as the underlying types could be converted in a standard
conforming way. As far as I see such conversions are acceptable and
used every now and then in the project, but they may hide some subtle
issues.
One interesting way I found to address this was to benefit from clang
plugin system [2]. A clang plugin allows you to run some frontend
actions when compiling code with clang, and this approach is used in
some large open source projects (e.g. I've got my inspiration largely
from libreoffice [3]). After a little experimenting I could teach such a
plugin to warn about situations similar to the one described above. What
it does is:
* It visits all the function call expressions
* Verify if the function arguments are defined via typedef
* Compare the actual argument with the function declaration
* Consult with the suppression rules to minimize false positives
In the end the plugin catches the error mentioned above, and we get
something like this:
ginget.c:143:41: warning: Typedef check: Expected 'BlockNumber' (aka 'unsigned int'),
got 'Buffer' (aka 'int') in PredicateLockPage PredicateLockPage(btree->index, stack->buffer, snapshot);
Of course the plugin produces more warning, and I haven't checked all of
them yet -- some probably would have to be ignored as well. But in the
long run I'm pretty confident it's possible to fine tune this logic and
suppression rules to get minimum noise.
As I see it, there are advantages of using plugins in such way:
* Helps automatically detect some issues
* Other type of plugins could be useful to support large undertakings,
where a lot of code transformation is involved.
And of course disadvantages as well:
* It has to be fine-tuned to be useful
* It's compiler dependent, not clear how to always exercise it
I would like to get your opinion, folks. Does it sound interesting
enough for the community, is it worth it to pursue the idea?
Some final notes about infrastructure bits. Never mind cmake in there --
it was just for a quick start, I'm going to convert it to something
else. There are some changes needed to tell the compiler to actually
load the plugin, those of course could be done much better as well. I
didn't do anything with meson here, because it turns out meson tends to
enclose the plugin file with '-Wl,--start-group ... -Wl,--end-group' and
it breaks the plugin discovery.
[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=126552c85c1cfb6ce6445159b8024cfa5631f33e
[2]: https://clang.llvm.org/docs/ClangPlugins.html
[3]: https://git.libreoffice.org/core/+/refs/heads/master/compilerplugins/clang/
Attachment
This is the first I am learning about clang plugins. Interesting concept. Did you give any thought to using libclang instead of a compiler plugin? I am kind of doing similar work, but I went with libclang instead of a plugin. -- Tristan Partin Neon (https://neon.tech)
> On Thu, Aug 03, 2023 at 12:23:52PM -0500, Tristan Partin wrote:
>
> This is the first I am learning about clang plugins. Interesting concept.
> Did you give any thought to using libclang instead of a compiler plugin? I
> am kind of doing similar work, but I went with libclang instead of a plugin.
Nope, never thought about trying libclang. From the clang documentation
it seems a plugin is a suitable interface if one wants to:
special lint-style warnings or errors for your project
Which is what I was trying to achieve. Are there any advantages of
libclang that you see?
On 03.08.23 18:56, Dmitry Dolgov wrote: > I would like to get your opinion, folks. Does it sound interesting > enough for the community, is it worth it to pursue the idea? I think it's interesting, and doesn't look too invasive. Maybe we can come up with three or four interesting use cases and try to implement them. BlockNumber vs. Buffer type checking is obviously a bit marginal to get anyone super-excited, but it's a reasonable demo. Also, how stable is the plugin API? Would we need to keep updating these plugins for each new clang version?
> On Wed, Aug 09, 2023 at 03:23:32PM +0200, Peter Eisentraut wrote: > On 03.08.23 18:56, Dmitry Dolgov wrote: > > I would like to get your opinion, folks. Does it sound interesting > > enough for the community, is it worth it to pursue the idea? > > I think it's interesting, and doesn't look too invasive. > > Maybe we can come up with three or four interesting use cases and try to > implement them. BlockNumber vs. Buffer type checking is obviously a bit > marginal to get anyone super-excited, but it's a reasonable demo. > > Also, how stable is the plugin API? Would we need to keep updating these > plugins for each new clang version? From the first glance the API itself seems to be stable -- I didn't find many breaking changes for plugins in the release notes over the last five releases. The git history for such definitions as ASTConsumer or RecursiveASTVisitor also doesn't seem to be very convoluted. But libreoffice example shows, that some updating would be necessary -- they've got about a dozen of places in the code that depend on the clang version. From what I see it's usually not directly related to the plugin API, and looks more like our opaque pointers issue.
Hi, On 8/10/23, Dmitry Dolgov <9erthalion6@gmail.com> wrote: >> On Wed, Aug 09, 2023 at 03:23:32PM +0200, Peter Eisentraut wrote: >> On 03.08.23 18:56, Dmitry Dolgov wrote: >> > I would like to get your opinion, folks. Does it sound interesting >> > enough for the community, is it worth it to pursue the idea? >> >> I think it's interesting, and doesn't look too invasive. +1. >> Maybe we can come up with three or four interesting use cases and try to >> implement them. BlockNumber vs. Buffer type checking is obviously a bit >> marginal to get anyone super-excited, but it's a reasonable demo. Several months ago, I implemented a clang plugin[1] for checking suspicious return/goto/break/continue in PG_TRY/PG_CATCH blocks and it found unsafe codes in Postgres[2]. It's implemented using clang's AST matcher API. I would like to contribute it well if this RFC can be accepted. [1] https://github.com/higuoxing/clang-plugins/blob/main/lib/ReturnInPgTryBlockChecker.cpp [2] https://www.postgresql.org/message-id/flat/CACpMh%2BCMsGMRKFzFMm3bYTzQmMU5nfEEoEDU2apJcc4hid36AQ%40mail.gmail.com -- Best Regards, Xing
On Fri Aug 4, 2023 at 5:47 AM CDT, Dmitry Dolgov wrote: > > On Thu, Aug 03, 2023 at 12:23:52PM -0500, Tristan Partin wrote: > > > > This is the first I am learning about clang plugins. Interesting concept. > > Did you give any thought to using libclang instead of a compiler plugin? I > > am kind of doing similar work, but I went with libclang instead of a plugin. > > Nope, never thought about trying libclang. From the clang documentation > it seems a plugin is a suitable interface if one wants to: > > special lint-style warnings or errors for your project > > Which is what I was trying to achieve. Are there any advantages of > libclang that you see? Only advantage I see to using libclang is that you can run programs built with libclang regardless of what your C compiler is. I typically use GCC. I think your idea of automating this kind of thing is great no matter how it is implemented. -- Tristan Partin Neon (https://neon.tech)
> On Fri, Dec 01, 2023 at 04:01:05PM -0600, Tristan Partin wrote: > On Fri Aug 4, 2023 at 5:47 AM CDT, Dmitry Dolgov wrote: > > > On Thu, Aug 03, 2023 at 12:23:52PM -0500, Tristan Partin wrote: > > > > > > This is the first I am learning about clang plugins. Interesting concept. > > > Did you give any thought to using libclang instead of a compiler plugin? I > > > am kind of doing similar work, but I went with libclang instead of a plugin. > > > > Nope, never thought about trying libclang. From the clang documentation > > it seems a plugin is a suitable interface if one wants to: > > > > special lint-style warnings or errors for your project > > > > Which is what I was trying to achieve. Are there any advantages of > > libclang that you see? > > Only advantage I see to using libclang is that you can run programs built > with libclang regardless of what your C compiler is. I typically use GCC. > > I think your idea of automating this kind of thing is great no matter how it > is implemented. Yeah, absolutely. Sorry, haven't had a chance to follow up on the patch, but I'm planing to do so. I guess the important part, as Peter mentioned above in the thread, is to figure out more use cases which could be usefully augmented with such compile time checks. At the moment I don't have any other except BlockNumber vs Buffer, so I'm going to search through the hackers looking for more potential targets. If anyone got a great idea right away about where compile-time checks could be useful, feel free to share!
On Sun, Dec 3, 2023 at 6:31 AM Dmitry Dolgov <9erthalion6@gmail.com> wrote: > > Only advantage I see to using libclang is that you can run programs built > > with libclang regardless of what your C compiler is. I typically use GCC. > > > > I think your idea of automating this kind of thing is great no matter how it > > is implemented. > > Yeah, absolutely. What is the difference between a clang plugin (or whatever this is), and a custom clang-tidy check? Are they the same thing? I myself use clang-tidy through clangd. It has a huge number of checks, plus some custom checks that are only used by certain open source projects. An example of a check that seems like it would be interesting to Postgres hackers: https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone/signal-handler.html An example of a custom clang-tidy check, used for the Linux Kernel: https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/linuxkernel/must-use-errs.html Your idea of starting with a check that identifies when BlockNumber and Buffer variables were confused seems like the right general idea to me. It's just about impossible for this to be a false positive in practice, which is important. But, I wonder, is there a way of accomplishing the same thing without any custom code? Seems like the general idea of not confusing two types like BlockNumber and Buffer might be a generic one? Some random ideas in this area: * A custom check that enforces coding standards within signal handlers -- the generic one that I linked to might need to be customized, in whatever way (don't use it myself). * A custom check that enforces a coding standard that applies within critical sections. We already have an assertion that catches memory allocations made within a critical section. It might make sense to have tooling that can detect other kinds of definitely-unsafe things in critical sections. For example, maybe it would be possible to detect when XLogRegisterBufData() has been passed a pointer to something on the stack that will go out of scope, in a way that leaves open the possibility of reading random stuff from the stack once inside XLogInsert(). AddressSanitizer's use-after-scope can detect that sort of thing, though not reliably. Not at all sure about how feasible any of this is... -- Peter Geoghegan
> On Sun, Dec 03, 2023 at 07:02:55PM -0800, Peter Geoghegan wrote:
> On Sun, Dec 3, 2023 at 6:31 AM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> > > Only advantage I see to using libclang is that you can run programs built
> > > with libclang regardless of what your C compiler is. I typically use GCC.
> > >
> > > I think your idea of automating this kind of thing is great no matter how it
> > > is implemented.
> >
> > Yeah, absolutely.
>
> What is the difference between a clang plugin (or whatever this is),
> and a custom clang-tidy check? Are they the same thing?
Good question. Obviously one difference is that a clang plugin is more
tightly integrated into the build process, where a custom clang-tidy
check could be used independently. IIUC they also use different API, AST
Consumer vs AST Matchers, but so far I have no idea what consequences
does it have.
Clang tooling page has this to say about choosing the right interface
[1], where clang-tidy seems to fall into LibTooling cathegory:
Clang Plugins allow you to run additional actions on the AST as part
of a compilation. Plugins are dynamic libraries that are loaded at
runtime by the compiler, and they’re easy to integrate into your
build environment.
Canonical examples of when to use Clang Plugins:
* special lint-style warnings or errors for your project creating
* additional build artifacts from a single compile step
[...]
LibTooling is a C++ interface aimed at writing standalone tools, as
well as integrating into services that run clang tools. Canonical
examples of when to use LibTooling:
* a simple syntax checker
* refactoring tools
> I myself use clang-tidy through clangd. It has a huge number of
> checks, plus some custom checks that are only used by certain open
> source projects.
>
> An example of a check that seems like it would be interesting to
> Postgres hackers:
>
> https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone/signal-handler.html
>
> An example of a custom clang-tidy check, used for the Linux Kernel:
>
> https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/linuxkernel/must-use-errs.html
Yeah, those look interesting, and might be even worth including
independently of the topic in this thread.
> Your idea of starting with a check that identifies when BlockNumber
> and Buffer variables were confused seems like the right general idea
> to me. It's just about impossible for this to be a false positive in practice,
> which is important. But, I wonder, is there a way of accomplishing the
> same thing without any custom code? Seems like the general idea of not
> confusing two types like BlockNumber and Buffer might be a generic
> one?
Agree, the example in this patch could be generalized.
> * A custom check that enforces coding standards within signal handlers
> -- the generic one that I linked to might need to be customized, in
> whatever way (don't use it myself).
>
> * A custom check that enforces a coding standard that applies within
> critical sections.
>
> We already have an assertion that catches memory allocations made
> within a critical section. It might make sense to have tooling that
> can detect other kinds of definitely-unsafe things in critical
> sections. For example, maybe it would be possible to detect when
> XLogRegisterBufData() has been passed a pointer to something on the
> stack that will go out of scope, in a way that leaves open the
> possibility of reading random stuff from the stack once inside
> XLogInsert(). AddressSanitizer's use-after-scope can detect that sort
> of thing, though not reliably.
Interesting ideas, thanks for sharing. I'm going to try implementing
some of those.
[1]: https://clang.llvm.org/docs/Tooling.html