[cfe-dev] Can clang generate a warning for this?

[Richard Trieu via cfe-dev]

Your example doesn't compile.  However, I suspect what you are seeing from
gcc is due to it performing analysis from the backend after inlining has
taken place.  As a policy, Clang won't emit warnings from the backend since
it will cause different diagnostics depending on the optimization levels
and other options specified.

On Fri, Jun 24, 2016 at 2:16 PM, Riyaz Puthiyapurayil <
Riyaz.Puthiyapurayil at synopsys.com> wrote:

> Thanks. UndefinedBehaviorSanitizer looks interesting. Will take a look.
>
>
>
> I had replied to my original email with a re-code of FooLI and FooBI which
> will expose this issue with gcc (and with a debug build).
>
>
>
> Also note that my request for warning was for the specific case where two
> constructors exist in FooLI, one accepting a pointer and another accepting
> a reference, and the user of the class chose the reference version by
> dereferencing a pointer when he/she should have actually used the other
> version (because the pointer can sometimes be null). This is not an
> expensive check for clang. OTOH, I admit that a warning in this case can
> cause many false positives if the intent of the user was to avoid the null
> check because (s)he knows the pointer cannot be null (especially my recoded
> version of the classes no longer does a null check in the reference version
> even with gcc which wasn’t optimizing away the redundant nullcheck). But
> then I cannot think of any reasonable recoding to silence a false positive
> when it occurs. So I will withdraw my request for a warning.
>
>
>
> My recoding to expose this issue with gcc is as follows:
>
>
>
> class FooBI {
>
> public:
>
>     :
>
>     FooBI(const FooB* foo_) :
>
>         foo(foo_ && foo_->bar ? foo_ : NULL) {}   // [1] has a null check
>
>    FooBI(const FooB& foo_) :
>
>         foo(foo.bar ? &foo_ : NULL) {}   // There is no null-check here
>
>                                          // as this expects the reference
>
>                                          // to be non-NULL (gcc and
> unoptimized
>
>                                          // code will now generate crash
> for
>
>                                          // a null reference)
>
>     :
>
> };
>
> template<class T>class FooLI : public FooBI {
>
> public:
>
>     :
>
>     FooLI(const FooL<T>* foo) :
>
>         FooBI(foo)
>
>     {}
>
>     FooLI(const Foo<T>& foo) :
>
>         FooBI(foo)     // changed from FooBI(&foo)
>
>     {}
>
>     :
>
> };
>
>
>
>
>
>
>
> *-Riyaz*
>
>
>
> *From:* Richard Trieu [mailto:rtrieu at google.com]
> *Sent:* Thursday, June 23, 2016 2:47 PM
> *To:* Riyaz Puthiyapurayil <Riyaz.Puthiyapurayil at synopsys.com>
> *Cc:* cfe-dev at lists.llvm.org
> *Subject:* Re: [cfe-dev] Can clang generate a warning for this?
>
>
>
> There's two suggestions for warnings here:
>
> 1) Warn that pointer p is given a pointer from somewhere and then
> dereferenced without a null check.
>
> 2) Warn that two similar constructions only differing by pointer versus
> reference, and should suggest the appropriate constructor.
>
>
>
> 1 is hard because the assignment and dereference can be separated by some
> amount of intervening code that needs to be checked.  This requires at
> least a path-sensitive checker.  There's also the case that the check is
> performed by a different function, which further complicates the checking.
> Also, getFooL() may have guarantees not visible to Clang.  If it already
> returns a non-null pointer, then this warning would be a false positive.
>
>
>
> 2 requires the comparison of the structure of functions to determine that
> they are identical except for the pointer/reference difference.  That would
> be a very fragile check and even small changes to the constructors would
> render Clang unable to see the equivalence between the two functions.
>
>
>
> As you've noted, Clang does have specific warnings for very obvious and
> very simple cases.  More complicated cases involving examining code across
> different functions is difficult and time-consuming for Clang, if not
> impossible if the functions are across different translation units.  If you
> are concerned about this problem in the future, I recommend the dynamic
> analysis tool Undefined Behavior Sanitizer (
> http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html).  Using the
> null checker will allow you to catch the null dereference at runtime.
>
>
>
>
>
> On Tue, Jun 21, 2016 at 10:28 PM, Riyaz Puthiyapurayil via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
> [corrected some typos in the earlier email]
>
>
>
> We have some very old code that resembles the following that causes it to
> crash when compiled with clang (it “works” with gcc).  The problem is the
> dereference of ‘p’ in the constructor argument of fooLI below (line marked
> as [2]).  Due to this dereference, the constructor FooLI(const Foo<T>& foo)
> is invoked which in turn invokes FooBI(…). Since p has already been
> dereferenced, clang assumes that p is non-null (which is OK because
> well-formed C++ code would not dereference a NULL pointer). Clang therefore
> eliminates the null check on foo_ at line [1] assuming that it always
> non-NULL. So when a null pointer is returned by getFooL(), a crash occurs.
>
>
>
> class FooBI {
>
> public:
>
>     :
>
>     FooBI(const FooB* foo_) :
>
>         foo(foo_ && foo_->bar ? foo_ : NULL) {}   // [1]
>
>     :
>
> };
>
> template<class T>class FooLI : public FooBI {
>
> public:
>
>     :
>
>     FooLI(const FooL<T>* foo) :
>
>         FooBI(foo)
>
>     {}
>
>     FooLI(const Foo<T>& foo) :
>
>         FooBI(&foo)
>
>     {}
>
>     :
>
> };
>
> :
>
>     FooL<Boo>* p = getFooL();
>
>     FooLI fooLI = FooLI(*p); // [2] p can be sometimes NULL
>
>
>
> The fix for this problem in our code is to avoid the dereference on line
> [2] which will then invoke FooLI(const FooL<T>* foo) and the null check on
> line [1] will be retained. Is it possible for clang to generate a warning
> about this? -Wundefined-bool-conversion does generate warnings in similar
> situations but not specifically for this case. Perhaps, this should be a
> different warning. There are two choices of constructors, one accepting a
> pointer and another accepting a reference. When the pointer is then
> dereferenced and converted to a reference, do you think it merits a warning?
>
>
>
>
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160624/39a39c31/attachment.html>