<div dir="ltr"><div><div><div><div>The problem I see is for example the following case:<br><span style="font-family:monospace,monospace"><br></span></div><span style="font-family:monospace,monospace">struct PhysicalProperty {<br></span></div><span style="font-family:monospace,monospace"> enum Kind { Area, Volume } kind;<br><br></span></div><span style="font-family:monospace,monospace"> int area, volume;<br></span></div><div><span style="font-family:monospace,monospace"><br> </span><span style="font-family:monospace,monospace"><span style="font-family:monospace,monospace">PhysicalProperty</span>(Kind k) : kind(k) {<br></span></div><div><span style="font-family:monospace,monospace"> switch(k) {</span><span style="font-family:monospace,monospace"><br></span><div><span style="font-family:monospace,monospace"> case Area:<br> area = 0;<br></span></div><div><span style="font-family:monospace,monospace"> break;</span><span style="font-family:monospace,monospace"><br></span><div><span style="font-family:monospace,monospace"> case Volume:<br></span></div><div><span style="font-family:monospace,monospace"> volume = 0;<br></span></div><div><span style="font-family:monospace,monospace"> break;</span></div></div></div><div><span style="font-family:monospace,monospace"> }<br></span></div><div><span style="font-family:monospace,monospace"> }</span><span style="font-family:monospace,monospace"><br><br></span><div><span style="font-family:monospace,monospace"> int& getVolume() {<br></span></div><div><span style="font-family:monospace,monospace"> assert(kind == Volume);<br></span></div><div><span style="font-family:monospace,monospace"> return Vvlume;<br></span></div><div><span style="font-family:monospace,monospace"> }</span><span style="font-family:monospace,monospace"><br><br></span><div><span style="font-family:monospace,monospace"> int& getArea() {<br></span></div><div><span style="font-family:monospace,monospace"> assert(kind == Area);<br></span></div><div><span style="font-family:monospace,monospace"> return area;<br></span></div><div><span style="font-family:monospace,monospace"> }</span></div></div></div><div><div><div><span style="font-family:monospace,monospace">};<br><br></span></div><div><span style="font-family:monospace,monospace"><font face="arial,helvetica,sans-serif">This example represents a good majority of reports from LLVM that is similar to what</font><font face="arial,helvetica,sans-serif"> George described. The kind field is always initialized, and is basically a guard against acquisition of uninitialized data members. Going with the proposed heuristic that fields / field types with "kind" or "tag" in their name should be ignored will achieve little IMO. We could outright ignore objects that contain a field like this, but that would maybe be overkill, as kind fields may also refer to the dynamic type of the object (like `PathDiagnosticPiece`, `SVal`, `MemRegion`, and so on).<br><br>></font></span> * We could find the discriminators using name conventions just as Kristóf mentioned<br>> * Suppress reports from classes with most of their
fields being uninitialized. This is a threshold kind of thing that is
unpopular to some since some could could bounce back and forth around
the threshold.<br><br><span style="font-family:monospace,monospace"></span></div><div><span style="font-family:monospace,monospace"> </span></div></div></div></div><br><div class="gmail_quote"><div dir="ltr">George Karpenkov <<a href="mailto:ekarpenkov@apple.com">ekarpenkov@apple.com</a>> ezt írta (időpont: 2018. aug. 14., K, 23:03):<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space">Annotations IMO definitely make no sense here, as it seems the design pattern is already bad:<div>either a subclass or a proper union should be used instead.</div><div><br></div><div>Perhaps it’s possible to try to heuristically determine that based on naming conventions of fields indeed.<br><div><br><blockquote type="cite"><div>On Aug 13, 2018, at 4:24 PM, Gábor Horváth <<a href="mailto:xazax.hun@gmail.com" target="_blank">xazax.hun@gmail.com</a>> wrote:</div><br class="m_-6391876686436905201Apple-interchange-newline"><div><div dir="ltr"><div>Hi!</div><div><br></div><div>Just brainstorming a bit, some of these might make no sense:</div><div>* We could find the discriminators using name conventions just as Kristóf mentioned</div><div>* We could have annotations, but people usually do not like this approach<br></div><div>* Maybe those classes have separate constructors for each of the type in the union? We could have some suppression heuristics around that:</div><div> - If we have multiple constructors only report fields that is uninitialized in all of them? This might also give us a lot of false negatives and hard to implement<br></div><div> - If the reads of a field is usually behind a guard do not report it as uninitialized</div><div> - Only check fields that can be unconditionally accessed via the public interface, might be fooled by castAs like methods</div><div>* Suppress reports from classes with most of their fields being uninitialized. This is a threshold kind of thing that is unpopular to some since some could could bounce back and forth around the threshold.</div><div>* Option to only care about fields that are initialized on some paths and not initialized on others? <br></div><div><br></div><div>Whichever direction we go, in case the heuristics might also result in some false positives, it might be good idea to make it optional (but on by default).</div><div><br></div><div>Regards,</div><div>Gabor<br></div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, 13 Aug 2018 at 11:31, Kristóf Umann via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div>Hi!<div dir="auto"><br></div><div dir="auto">Thank you so much for this! I'm very grateful for all the reviews and feedback I've been given, makes my work so much more enjoyable.</div><div dir="auto"><br></div><div dir="auto">I'm aware of this issue, LLVM/Clang is littered with these constructs. I haven't tried to fix it just yet. Maybe add a heuristic that fields/types with "Kind" or "Tag" substring in them should be ignored?</div><div dir="auto"><br></div><div dir="auto">Best regards,</div><div dir="auto">Kristóf Umann</div><div dir="auto"><br></div><div dir="auto">(Btw my first name is Kristóf, it's quite confusing in hungarian)</div><br><div class="gmail_extra"><br><div class="gmail_quote">On 13 Aug 2018 20:15, "George Karpenkov" <<a href="mailto:ekarpenkov@apple.com" target="_blank">ekarpenkov@apple.com</a>> wrote:<br type="attribution"><blockquote class="m_-6391876686436905201m_5617950693537870137quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
I have recently evaluated the (relatively recently developed, available with -Xclang -analyzer-checker=alpha.cplusplus.UninitializedObject)<br>
UninitializedObjectChecker, which warns after the constructor call if any of the fields were left uninitialized.<br>
<br>
Good news: I have found a few actual bugs<br>
Bad news: Those bugs were hidden under hundreds of other reports<br>
<br>
Most of the false alarms come from a case where a class is actually used as a union,<br>
and a field is used to differentiate between those types.<br>
Then unused types are simply left uninitialized.<br>
Conceptually, this is not a correct design, but nevertheless, it is rather ubiquitous.<br>
<br>
I have no idea whether we can work around this case, @Umann, any ideas there?<br>
<font color="#888888"><br>
George<br>
</font></blockquote></div><br></div></div></div>
_______________________________________________<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" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div>
</div></blockquote></div><br></div></div></blockquote></div>