melver wrote: Addressed comments so far.
> > The equivalent of passing a `pt_guarded_by` variable by value doesn't seem > > to warn. > > This inconsistency is probably my largest concern. If you have > > ```c > int x GUARDED_BY(mu); > int *p PT_GUARDED_BY(mu); > ``` > > then `&x` should basically behave like `p`, and `x` like `*p`. But with the > current implementation, that cannot be the case, because `p` is just a no-op. Right, PT_GUARDED_BY() coverage is lacking in this case. > That's why I think we should rather check function arguments, and warn on > passing (1) the address of a guarded variable, (2) a pointee-guarded variable. I agree this might be more conservative. > There is already functionality that makes sure (1) and (2) work together: > `checkAccess` and `checkPtAccess` call each other when necessary. This > doesn't seem to handle unary `&` at the moment, but I suppose this would be > easy to add. The check for passing arguments itself is in `examineArguments`. > We currently only check reference type arguments: > > ```c++ > if (Qt->isReferenceType()) > checkAccess(*Arg, AK_Read, POK_PassByRef); > ``` > > Here we might simply add a branch for pointer types with the appropriate > `POK` that you already introduced (though we might need to rename it > slightly). > > This still doesn't cover local pointer variables, but here I guess we need a > better understanding of the patterns and how much alias analysis we'd need to > unravel them. Local pointer variables as obfuscation device are hopefully not > common, so there is likely some more complex logic that would be intractable > for us anyway. So I'd be curious to see some interesting examples. The Linux kernel has odd idioms like this one: ``` #define __READ_ONCE(x) (*(const volatile __unqual_scalar_typeof(x) *)&(x)) ``` When passed a guarded variable (e.g. `READ_ONCE(my_guarded_var)`), this will no longer warn if we do not check addressof. My intent with Wthread-safety-addressof was to blindly warn on "addressof", so we get this coverage. But I suppose if we switch it to just cover cases when passing to functions, I'd propose: - introduce Wthread-safety-pointer as a counterpart to Wthread-safety-reference - enable it by default similar to Wthread-safety-reference (but initially under Wthread-safety-beta) I still wouldn't mind if Wthread-safety-addressof existed (for the plethora of weird macros the Linux kernel has), but I understand that maintaining more features might not be what we want. Preferences? 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
