[cfe-dev] [analyzer] UninitializedObjectChecker evaluation

[Kristóf Umann via cfe-dev]

The problem I see is for example the following case:

struct PhysicalProperty {
  enum Kind { Area, Volume } kind;

  int area, volume;

  PhysicalProperty(Kind k) : kind(k) {
    switch(k) {
    case Area:
      area = 0;
      break;
    case Volume:
      volume = 0;
      break;
    }
  }

  int& getVolume() {
    assert(kind == Volume);
    return Vvlume;
  }

  int& getArea() {
    assert(kind == Area);
    return area;
  }
};

This example represents a good majority of reports from LLVM that is
similar to what 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).

> * We could find the discriminators using name conventions just as Kristóf
mentioned
> * 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.



George Karpenkov <ekarpenkov at apple.com> ezt írta (időpont: 2018. aug. 14.,
K, 23:03):

> Annotations IMO definitely make no sense here, as it seems the design
> pattern is already bad:
> either a subclass or a proper union should be used instead.
>
> Perhaps it’s possible to try to heuristically determine that based on
> naming conventions of fields indeed.
>
> On Aug 13, 2018, at 4:24 PM, Gábor Horváth <xazax.hun at gmail.com> wrote:
>
> Hi!
>
> Just brainstorming a bit, some of these might make no sense:
> * We could find the discriminators using name conventions just as Kristóf
> mentioned
> * We could have annotations, but people usually do not like this approach
> * Maybe those classes have separate constructors for each of the type in
> the union? We could have some suppression heuristics around that:
>   - 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
>   - If the reads of a field is usually behind a guard do not report it as
> uninitialized
>   - Only check fields that can be unconditionally accessed via the public
> interface, might be fooled by castAs like methods
> * 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.
> * Option to only care about fields that are initialized on some paths and
> not initialized on others?
>
> 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).
>
> Regards,
> Gabor
>
> On Mon, 13 Aug 2018 at 11:31, Kristóf Umann via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> Hi!
>>
>> 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.
>>
>> 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?
>>
>> Best regards,
>> Kristóf Umann
>>
>> (Btw my first name is Kristóf, it's quite confusing in hungarian)
>>
>>
>> On 13 Aug 2018 20:15, "George Karpenkov" <ekarpenkov at apple.com> wrote:
>>
>> Hi,
>>
>> I have recently evaluated the  (relatively recently developed, available
>> with -Xclang -analyzer-checker=alpha.cplusplus.UninitializedObject)
>> UninitializedObjectChecker, which warns after the constructor call if any
>> of the fields were left uninitialized.
>>
>> Good news: I have found a few actual bugs
>> Bad news: Those bugs were hidden under hundreds of other reports
>>
>> Most of the false alarms come from a case where a class is actually used
>> as a union,
>> and a field is used to differentiate between those types.
>> Then unused types are simply left uninitialized.
>> Conceptually, this is not a correct design, but nevertheless, it is
>> rather ubiquitous.
>>
>> I have no idea whether we can work around this case, @Umann, any ideas
>> there?
>>
>> George
>>
>>
>> _______________________________________________
>> 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/20180815/bf54a4e9/attachment.html>