<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
@font-face
{font-family:"Viner Hand ITC";
panose-1:3 7 5 2 3 5 2 2 2 3;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p
{mso-style-priority:99;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
p.CodeBlock, li.CodeBlock, div.CodeBlock
{mso-style-name:"Code Block";
margin:0in;
margin-bottom:.0001pt;
background:#F2F2F2;
border:none;
padding:0in;
font-size:12.0pt;
font-family:Consolas;}
p.MailQuote, li.MailQuote, div.MailQuote
{mso-style-name:"Mail Quote";
margin:0in;
margin-bottom:.0001pt;
border:none;
padding:0in;
font-size:9.0pt;
font-family:"Calibri",sans-serif;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Sorry, I think you have misunderstood something. gcc does not warn or crash with the original example but clang crashes. With my modified code, both gcc and clang generate
a crash (neither generates a warning).<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">It is important for me to always get a crash when there is a null reference (both with gcc and clang and with an unoptimized build, so that we don’t have to rely on clang doing
the optimization to expose the undefined behavior in our code). My modified code does just that. The modified code is just manually doing the clang optimization of eliding the nullcheck.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Hope it is clear now. I haven’t checked whether my example contains typos but I didn’t provide a complete standalone test anyway. The actual code does compile fine with both
clang and gcc.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoNormal" style="text-align:justify"><i><span style="font-size:11.0pt;font-family:"Viner Hand ITC";color:black">-Riyaz<o:p></o:p></span></i></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> Richard Trieu [mailto:rtrieu@google.com]
<br>
<b>Sent:</b> Friday, June 24, 2016 2:45 PM<br>
<b>To:</b> Riyaz Puthiyapurayil <Riyaz.Puthiyapurayil@synopsys.com><br>
<b>Cc:</b> cfe-dev@lists.llvm.org<br>
<b>Subject:</b> Re: [cfe-dev] Can clang generate a warning for this?<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">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.<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<div>
<p class="MsoNormal">On Fri, Jun 24, 2016 at 2:16 PM, Riyaz Puthiyapurayil <<a href="mailto:Riyaz.Puthiyapurayil@synopsys.com" target="_blank">Riyaz.Puthiyapurayil@synopsys.com</a>> wrote:<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">Thanks. UndefinedBehaviorSanitizer looks interesting. Will take a look.</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">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). </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">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.</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">My recoding to expose this issue with gcc is as follows:</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<div style="border-top:solid #1F497D 1.0pt;border-left:none;border-bottom:solid #1F497D 1.0pt;border-right:none;padding:1.0pt 0in 1.0pt 0in">
<p style="background:#F2F2F2">class FooBI {<o:p></o:p></p>
<p style="background:#F2F2F2">public:<o:p></o:p></p>
<p style="background:#F2F2F2"> :<o:p></o:p></p>
<p style="background:#F2F2F2"> FooBI(const FooB* foo_) :<o:p></o:p></p>
<p style="background:#F2F2F2"> foo(foo_ && foo_->bar ? foo_ : NULL) {} // [1] has a null check<o:p></o:p></p>
<p style="background:#F2F2F2"> FooBI(const FooB& foo_) :<o:p></o:p></p>
<p style="background:#F2F2F2"> foo(foo.bar ? &foo_ : NULL) {} // There is no null-check here
<o:p></o:p></p>
<p style="background:#F2F2F2"> // as this expects the reference<o:p></o:p></p>
<p style="background:#F2F2F2"> // to be non-NULL (gcc and unoptimized
<o:p></o:p></p>
<p style="background:#F2F2F2"> // code will now generate crash for<o:p></o:p></p>
<p style="background:#F2F2F2"> // a null reference)<o:p></o:p></p>
<p style="background:#F2F2F2"> :<o:p></o:p></p>
<p style="background:#F2F2F2">};<o:p></o:p></p>
<p style="background:#F2F2F2">template<class T>class FooLI : public FooBI {<o:p></o:p></p>
<p style="background:#F2F2F2">public:<o:p></o:p></p>
<p style="background:#F2F2F2"> : <o:p></o:p></p>
<p style="background:#F2F2F2"> FooLI(const FooL<T>* foo) :<o:p></o:p></p>
<p style="background:#F2F2F2"> FooBI(foo)<o:p></o:p></p>
<p style="background:#F2F2F2"> {}<o:p></o:p></p>
<p style="background:#F2F2F2"> FooLI(const Foo<T>& foo) :<o:p></o:p></p>
<p style="background:#F2F2F2"> FooBI(foo) // changed from FooBI(&foo)<o:p></o:p></p>
<p style="background:#F2F2F2"> {}<o:p></o:p></p>
<p style="background:#F2F2F2"> :<o:p></o:p></p>
<p style="background:#F2F2F2">};<o:p></o:p></p>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;text-align:justify">
<i><span style="font-size:11.0pt;font-family:"Viner Hand ITC";color:black">-Riyaz</span></i><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> </span><o:p></o:p></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> Richard Trieu [mailto:<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>]
<br>
<b>Sent:</b> Thursday, June 23, 2016 2:47 PM<br>
<b>To:</b> Riyaz Puthiyapurayil <<a href="mailto:Riyaz.Puthiyapurayil@synopsys.com" target="_blank">Riyaz.Puthiyapurayil@synopsys.com</a>><br>
<b>Cc:</b> <a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<b>Subject:</b> Re: [cfe-dev] Can clang generate a warning for this?</span><o:p></o:p></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">There's two suggestions for warnings here:<o:p></o:p></p>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">1) Warn that pointer p is given a pointer from somewhere and then dereferenced without a null check.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">2) Warn that two similar constructions only differing by pointer versus reference, and should suggest the appropriate constructor.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">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 (<a href="http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html" target="_blank">http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html</a>).
Using the null checker will allow you to catch the null dereference at runtime.<o:p></o:p></p>
</div>
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">On Tue, Jun 21, 2016 at 10:28 PM, Riyaz Puthiyapurayil via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<div>
<div>
<p>[corrected some typos in the earlier email]<o:p></o:p></p>
<p> <o:p></o:p></p>
<p>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.<o:p></o:p></p>
<p> <o:p></o:p></p>
<p>class FooBI {<o:p></o:p></p>
<p>public:<o:p></o:p></p>
<p> :<o:p></o:p></p>
<p> FooBI(const FooB* foo_) :<o:p></o:p></p>
<p> foo(foo_ && foo_->bar ? foo_ : NULL) {} // [1]<o:p></o:p></p>
<p> :<o:p></o:p></p>
<p>};<o:p></o:p></p>
<p>template<class T>class FooLI : public FooBI {<o:p></o:p></p>
<p>public:<o:p></o:p></p>
<p> : <o:p></o:p></p>
<p> FooLI(const FooL<T>* foo) :<o:p></o:p></p>
<p> FooBI(foo)<o:p></o:p></p>
<p> {}<o:p></o:p></p>
<p> FooLI(const Foo<T>& foo) :<o:p></o:p></p>
<p> FooBI(&foo)<o:p></o:p></p>
<p> {}<o:p></o:p></p>
<p> :<o:p></o:p></p>
<p>};<o:p></o:p></p>
<p>:<o:p></o:p></p>
<p> FooL<Boo>* p = getFooL(); <o:p></o:p></p>
<p> FooLI fooLI = FooLI(*p); // [2] p can be sometimes NULL <o:p></o:p></p>
<p> <o:p></o:p></p>
<p>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?<o:p></o:p></p>
<p> <o:p></o:p></p>
<p> <o:p></o:p></p>
</div>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;margin-bottom:12.0pt"><br>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><o:p></o:p></p>
</blockquote>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
</div>
</div>
</body>
</html>