[webkit-dev] Smart Pointer Analysis Tool for WebKit

[Ryosuke Niwa]

On Wed, Sep 23, 2020 at 10:32 AM Darin Adler <darin at apple.com> wrote:

> On Sep 16, 2020, at 11:32 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>
>    1. Every data member to a ref counted object must use either Ref,
>    RefPtr, or WeakPtr. webkit.NoUncountedMemberChecker
>    <https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checkers.rst#id17>
>
> My only worry here is performance. Do we know yet if we can afford it?
>

We still need to find that out. So far, our deployment of smart pointers in
various DOM objects haven't caused perf regressions yet.

The worst case here is Ref, which is much worse than a reference since we
> end up having to use -> instead of . everywhere and you can’t see that
> there is no null involved.
>

In practice, this may not matter much because in many of our codebases,
most of references are used as function arguments, in which case, they're
still allowed without having to store in a local Ref / RefPtr. There are
quite a few cases where data members are references but then those can also
be replaced by a simple member function which retrieves the value of the
smart pointer member variable and returns a reference.

>
>    1. Every ref counted base class, if it has a derived class, must
>    define a virtual destructor. webkit.RefCntblBaseVirtualDtor
>    <https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checkers.rst#id16>
>
> The style system has an optimization that intentionally violates this for
> performance reasons (StyleRuleBase).
>

Interesting. I wasn't aware of this example. We're planning to add some
kind of compiler-level annotations to exempt these warnings so we may need
to apply that here if it's important for performance.

>
>    1. Every ref counted object passed to a non-trivial function as an
>    argument (including "this" pointer) should be stored as a Ref or RefPtr in
>    the caller’s local scope unless it's an argument to the caller itself by
>    transitive property [1]. alpha.webkit.UncountedCallArgsChecker
>    <https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checkers.rst#id21>
>
> What is a non-trivial function?
>

For now, a trivial function is defined as a member function defined in the
class declaration whose definition simply returns a member variable (the
result of get() or a copy if the member variable is a smart pointer).

>
>    1. Every ref counted object must be captured using Ref or RefPtr for
>    lambda. webkit.UncountedLambdaCapturesChecker
>    <https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checkers.rst#id18>
>
> Ref, RefPtr, or WeakPtr, right?
>
> Same concern about Ref vs references.
>

This is an interesting point. We should probably amend the rule to allow
WeakPtr as well.

Jan: do we currently allow WeakPtr or just Ref / RefPtr?

We probably also need to figure out a way to exempt all lambda functions
that never get stored anywhere. We have a bunch of helper functions like
WTF::map which just calls lambdas on each item while iterating over an
array, etc... and there is no need to create a separate Ref / RefPtr in
those cases since lambdas are never stored and re-used later.

I wonder if there is a way for your tool to automatically figure that out?
e.g. notice that a function never stores lambda anywhere, and then
propagate that information as some kind of function attribute. Then any
function that doesn't store lambda anywhere and only calls those functions
that also don't store lambda anywhere can marked as "safe".

>
>    1. Local variables - we’re still working on this (
>    https://reviews.llvm.org/D83259)
>
> I am looking forward to learning more about the proposal here.
>
> Same concern about Ref vs. references.
>
> I really want to see before/after for some non-trivial source files with
> significant problems; where will this drive the most change and what will
> things look like after?
>

Right, for both performance measurements and code changes, we probably want
to fix warnings in a large quality in some important files and see
before/after.

- R. Niwa
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20200923/bc9d55d1/attachment.htm>