melver wrote:
> We had a discussion on https://reviews.llvm.org/D52395 that might be relevant
> here. To quote @delesley:
>
> > When you pass an object to a function, either by pointer or by reference,
> > no actual load from memory has yet occurred. Thus, there is a real risk of
> > false positives; what you are saying is that the function _might_ read or
> > write from protected memory, not that it actually will.
Right. Though in the absence of better pointer tracking / alias analysis, I
believe there's no way to avoid resulting false positives with pointers. It's
something that a user of Wthread-safety-addressof would opt-in explicitly -
documentation needs elaboration on this perhaps.
> Another aspect is that we don't check if the lock is still held when we
> dereference the pointer, so there are also false negatives:
>
> ```c++
> Mutex mu;
> int x GUARDED_BY(mu);
>
> void f() {
> mu.Lock();
> int *p = &x;
> mu.Unlock();
> *p = 0;
> }
> ```
Good point - though I'd prefer few false negatives + false positives over no
checks at all. It's a tradeoff, and therefore we definitely shouldn't enable
Wthread-safety-addressof by default.
> You use the analogy of C++ references. Interestingly, we don't warn on
> "taking a reference", partly because no such thing exists. (You can only
> initialize something of reference type with an expression.) We warn on
> passing a variable by reference _to a function_, or in other words,
> initializing a parameter of reference type with a guarded variable. (Since
> https://reviews.llvm.org/D153131, we also warn on returning a reference when
> the lock is no longer held after return. Note that the initialization of the
> reference might still happen under lock!)
>
> However, we also track references in a way that pointers are not tracked. The
> reason is probably that references are immutable (that is, `T&` is
> semantically the same thing as `T* const`).
>
> ```c++
> void g() {
> mu.Lock();
> int &ref = x;
> mu.Unlock();
> ref = 0; // warning: writing variable 'x' requires holding mutex 'mu'
> exclusively
> }
> ```
>
> If you "take the reference" outside of the lock and do the assignment inside,
> there is no warning. Because the assignment is what needs the lock, not
> taking the address.
Good points, and I missed that reference handling is much better in this regard.
> However, with functions it's somewhat reasonable to assume that the function
> accesses through the pointer, and that the pointer doesn't escape. (This is
> of course not always true, but often enough.) So I wonder whether we should
> restrict this to pointers passed as function arguments.
That's a reasonable option to make it more conservative, although I'm not sure
it's necessary (yet).
> But if the number of false positives is manageable, we can also leave it like
> you implemented it. Did you try it out on some code base that uses thread
> safety annotations?
I'm working on bringing Wthread-safety to the Linux kernel. The strategy chosen
is to convert one subsystem (or single TU) at a time, based on an opt-in basis.
Since Linux already has a rather special dialect of C, those that would choose
to opt in their TUs would opt into another variant of Linux's C dialect (one
with Wthread-safety enabled).
My experience in converting a subsystem I maintain is that applying
Wthread-safety already requires small refactors to avoid false positives, and
I'm not opposed to add additional constraints via Wthread-safety-addressof.
Without additional checking of "obtaining address of guarded variables", I find
that Wthread-safety barely provides benefits, as passing pointers to
datastructures is so common.
Resolving this will improve the usefulness, and ultimately the chances of us
succeeding to upstream this to the Linux kernel.
I don't think we will ever convert 100% of the kernel to use Wthread-safety,
but the plan is that willing subsystem maintainers opt in and deal with
warnings produced by Wthread-safety. Having Wthread-safety-addressof will add
additional coverage. For cases where it produces too many false positives, I
intend to add a simple option that can enable/disable addressof checking for
individual TUs. Or if almost all places where obtaining the address of a
particular lock-guarded variable is _not_ actually under a lock (but still
correct), leaving off `guarded_by` might simply be the right choice.
https://github.com/llvm/llvm-project/pull/123063
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
