Analyzer: Extract region-walking behavior from CallAndMessageChecker

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Analyzer: Extract region-walking behavior from CallAndMessageChecker

Jordy Rose
CallAndMessageChecker currently has a simple check to see if any of the
members of a pass-by-value struct are uninitialized. It handles nested
structs, but not structs containing arrays.

I propose to extract this region-walking behavior out into a new class,
RegionWalker. This would then be the basis for the current pass-by-value
check and a possible new one (see below). My implementation of RegionWalker
is loosely modeled after RecursiveASTVisitor (see attached patch), and
supports both structs and arrays, and both nested.


The additional check I propose is for passing pointers to undefined values
in an argument slot marked as "const". Clearly, the argument value is not
useful, and for a const argument it can't be /set/ by the method either.
(The exception would be "const void *", which is both the type of
Objective-C "id" and sometimes an indicator that the pointer won't be
dereferenced by the callee.)

For the case of an array or struct (such as the source buffer for
strcpy()), we have to be a little more cautious -- the pointer might be
useful if /any/ of the fields are initialized. This is where RegionWalker
would come in.

But that's for a later patch.

Suggestions, comments? Is this overkill? (Especially if the new check
doesn't seem like a good idea.)
Jordy
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

RegionWalker.patch (18K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Analyzer: Extract region-walking behavior from CallAndMessageChecker

Ted Kremenek

On Jun 23, 2010, at 12:05 AM, Jordy Rose wrote:

> CallAndMessageChecker currently has a simple check to see if any of the
> members of a pass-by-value struct are uninitialized. It handles nested
> structs, but not structs containing arrays.
>
> I propose to extract this region-walking behavior out into a new class,
> RegionWalker. This would then be the basis for the current pass-by-value
> check and a possible new one (see below). My implementation of RegionWalker
> is loosely modeled after RecursiveASTVisitor (see attached patch), and
> supports both structs and arrays, and both nested.

Hi Jordy,

I think in principle the addition of RegionWalker is a nice refactoring, but I'm really concerned about the following:

+    const ElementRegion *ER = MemMgr.getElementRegion(EleTy, IndexVal, R, Ctx);

This call means that we will create a new ElementRegion for accessing every single element of an array.  This is really expensive.  Not only will this be really slow for large constant-sized arrays, it will cause a bunch of ElementRegion objects to get generated that will stay around for the entire lifetime of the GRExprEngine object.

As is, I don't think this can go in.

My high-level question is what is the purpose of this API?  Is to iterate over "direct" bindings, or all the values in memory?  The StoreManager does support an API to iterate over bindings, but the behavior will be highly dependent on the implementation of the StoreManager.  We really need to design these APIs with algorithmic efficiency in mind. If we need to add more high-level APIs to StoreManager to make the kind of queries we want to do more efficient than I prefer we do that.  RegionWalker right now defeats all the laziness of RegionStore by instantiation regions on-the-fly.  This is going to be a huge performance killer.

Also, I'm not certain if extending the CallAndMessageChecker to include arrays will be all that useful.  Consider an array field that holds an array of bytes.  If that buffer holds a null-terminated string, with some of the remaining characters left uninitialized, that isn't a bug unless those values are actually accessed.  Indeed I have already seen some false positives from this check when it comes to passing in structs to functions that contain fields that are not initialized but are not accessed.  There is only really a bug here if the fields are accessed, so the CallAndMessageChecker may already be too aggressive.  Also looking at arrays makes it more aggressive and expensive, and I'm not certain if the returns are worth it.

> The additional check I propose is for passing pointers to undefined values
> in an argument slot marked as "const". Clearly, the argument value is not
> useful, and for a const argument it can't be /set/ by the method either.
> (The exception would be "const void *", which is both the type of
> Objective-C "id" and sometimes an indicator that the pointer won't be
> dereferenced by the callee.)

Objective-C pointers are special-cased in the type system, so I'd restrict this to just C structs for now.  It seems like a useful check that we can implement and then see how much useful results we get back (and how many false positives).  We have to be careful with C++ classes and inheritance, e.g.:

class A { ... };
class B : public A { int uninit_field; ... };
...
void foo(const A *a);
...
B b;
foo(&b);

In this case, although 'b' contains an uninitialized field, it doesn't matter because 'b' is passed as an 'A*' to foo().  It is possible that foo() does a downcast to B*, but without knowledge about foo() we don't know.

> For the case of an array or struct (such as the source buffer for
> strcpy()), we have to be a little more cautious -- the pointer might be
> useful if /any/ of the fields are initialized. This is where RegionWalker
> would come in.
>
> But that's for a later patch.
>
> Suggestions, comments? Is this overkill? (Especially if the new check
> doesn't seem like a good idea.)

My main concern about CallAndMessageChecker's introspection of uninitialized struct fields (which I think I wrote) is that it is very approximate and I'm not convinced it has much added value.  While passing uninitialized scalars into a function is unambiguously a bug, passing in a struct or array with uninitialized fields is not.  In such cases, the best way to detect a bug is if we knew the called function accessed the given field or array element that was known to be uninitialized in the caller.  Thus to really do this check right we probably need to build function summaries to detect when it is not okay to pass in an uninitialized field value.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyzer: Extract region-walking behavior from CallAndMessageChecker

Ted Kremenek
On Jun 23, 2010, at 4:30 PM, Ted Kremenek wrote:

>
> On Jun 23, 2010, at 12:05 AM, Jordy Rose wrote:
>
>> CallAndMessageChecker currently has a simple check to see if any of the
>> members of a pass-by-value struct are uninitialized. It handles nested
>> structs, but not structs containing arrays.
>>
>> I propose to extract this region-walking behavior out into a new class,
>> RegionWalker. This would then be the basis for the current pass-by-value
>> check and a possible new one (see below). My implementation of RegionWalker
>> is loosely modeled after RecursiveASTVisitor (see attached patch), and
>> supports both structs and arrays, and both nested.
>
> Hi Jordy,
>
> I think in principle the addition of RegionWalker is a nice refactoring, but I'm really concerned about the following:
>
> +    const ElementRegion *ER = MemMgr.getElementRegion(EleTy, IndexVal, R, Ctx);
>
> This call means that we will create a new ElementRegion for accessing every single element of an array.  This is really expensive.  Not only will this be really slow for large constant-sized arrays, it will cause a bunch of ElementRegion objects to get generated that will stay around for the entire lifetime of the GRExprEngine object.

One other comment: explicit iteration over the array also won't work from an algorithmic standpoint once we support arrays with symbolic sizes.  In this case, we'll need to query the StoreManager if we want to know "is there an element that is uninitialized", etc.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyzer: Extract region-walking behavior from CallAndMessageChecker

Jordy Rose
In reply to this post by Ted Kremenek

On Wed, 23 Jun 2010 16:30:29 -0700, Ted Kremenek <[hidden email]>
wrote:
> On Jun 23, 2010, at 12:05 AM, Jordy Rose wrote:
>
>> CallAndMessageChecker currently has a simple check to see if any of the
>> members of a pass-by-value struct are uninitialized. It handles nested
>> structs, but not structs containing arrays.
>>
>> I propose to extract this region-walking behavior out into a new class,
>> RegionWalker. This would then be the basis for the current
pass-by-value

>> check and a possible new one (see below). My implementation of
>> RegionWalker
>> is loosely modeled after RecursiveASTVisitor (see attached patch), and
>> supports both structs and arrays, and both nested.
>
> Hi Jordy,
>
> I think in principle the addition of RegionWalker is a nice refactoring,
> but I'm really concerned about the following:
>
> +    const ElementRegion *ER = MemMgr.getElementRegion(EleTy, IndexVal,
R,
> Ctx);
>
> This call means that we will create a new ElementRegion for accessing
> every single element of an array.  This is really expensive.  Not only
will
> this be really slow for large constant-sized arrays, it will cause a
bunch
> of ElementRegion objects to get generated that will stay around for the
> entire lifetime of the GRExprEngine object.
>
> As is, I don't think this can go in.

I was concerned about this too, but couldn't think of another way to
actually visit every scalar element of a complicated nesting of structs and
arrays. On the face of it, this could be solved with stack-based
ElementRegion and FieldRegion objects...apart from being the only such case
of stack-based regions, and a slightly distasteful idea. *grin*

But due to your other objections, perhaps it is not a useful finding.
Perhaps if a pass-by-value struct had /no/ initialized members, it'd be
worth warning...but again, there, the effort to actually walk all the
subregions might not be worth it.


> My high-level question is what is the purpose of this API?  Is to
iterate
> over "direct" bindings, or all the values in memory?  The StoreManager
does
> support an API to iterate over bindings, but the behavior will be highly
> dependent on the implementation of the StoreManager.  We really need to
> design these APIs with algorithmic efficiency in mind. If we need to add
> more high-level APIs to StoreManager to make the kind of queries we want
to
> do more efficient than I prefer we do that.  RegionWalker right now
defeats
> all the laziness of RegionStore by instantiation regions on-the-fly.
This
> is going to be a huge performance killer.

Huh. I had originally thought that RegionStore's subregion map was flat,
that /any/ subregion showed up under the main one. Which wouldn't allow for
the nice "field chain" in CallAndMessageChecker. But this isn't the case.

So perhaps I could rewrite RegionWalker to make use of this, though your
points make it seem much less useful than I had thought.

The original motivation was to avoid duplicating code between
CallAndMessageChecker, and this new const buffer checker idea. None of the
other checkers yet have the same behavior. I'm feeling cautious about the
new check as well -- there /are/ cases where a const pointer is never
dereferenced, and so it doesn't matter if its pointee is completely
uninitialized.

Perhaps this should be put on hold in favor of work with a clearer payoff.
Thanks for pointing out the problems with this.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyzer: Extract region-walking behavior from CallAndMessageChecker

Ted Kremenek
On Jun 23, 2010, at 6:52 PM, Jordy Rose wrote:

>
> On Wed, 23 Jun 2010 16:30:29 -0700, Ted Kremenek <[hidden email]>
> wrote:
>> On Jun 23, 2010, at 12:05 AM, Jordy Rose wrote:
>>
>>> CallAndMessageChecker currently has a simple check to see if any of the
>>> members of a pass-by-value struct are uninitialized. It handles nested
>>> structs, but not structs containing arrays.
>>>
>>> I propose to extract this region-walking behavior out into a new class,
>>> RegionWalker. This would then be the basis for the current
> pass-by-value
>>> check and a possible new one (see below). My implementation of
>>> RegionWalker
>>> is loosely modeled after RecursiveASTVisitor (see attached patch), and
>>> supports both structs and arrays, and both nested.
>>
>> Hi Jordy,
>>
>> I think in principle the addition of RegionWalker is a nice refactoring,
>> but I'm really concerned about the following:
>>
>> +    const ElementRegion *ER = MemMgr.getElementRegion(EleTy, IndexVal,
> R,
>> Ctx);
>>
>> This call means that we will create a new ElementRegion for accessing
>> every single element of an array.  This is really expensive.  Not only
> will
>> this be really slow for large constant-sized arrays, it will cause a
> bunch
>> of ElementRegion objects to get generated that will stay around for the
>> entire lifetime of the GRExprEngine object.
>>
>> As is, I don't think this can go in.
>
> I was concerned about this too, but couldn't think of another way to
> actually visit every scalar element of a complicated nesting of structs and
> arrays. On the face of it, this could be solved with stack-based
> ElementRegion and FieldRegion objects...apart from being the only such case
> of stack-based regions, and a slightly distasteful idea. *grin*
>
> But due to your other objections, perhaps it is not a useful finding.
> Perhaps if a pass-by-value struct had /no/ initialized members, it'd be
> worth warning...but again, there, the effort to actually walk all the
> subregions might not be worth it.
>
>
>> My high-level question is what is the purpose of this API?  Is to
> iterate
>> over "direct" bindings, or all the values in memory?  The StoreManager
> does
>> support an API to iterate over bindings, but the behavior will be highly
>> dependent on the implementation of the StoreManager.  We really need to
>> design these APIs with algorithmic efficiency in mind. If we need to add
>> more high-level APIs to StoreManager to make the kind of queries we want
> to
>> do more efficient than I prefer we do that.  RegionWalker right now
> defeats
>> all the laziness of RegionStore by instantiation regions on-the-fly.
> This
>> is going to be a huge performance killer.
>
> Huh. I had originally thought that RegionStore's subregion map was flat,
> that /any/ subregion showed up under the main one. Which wouldn't allow for
> the nice "field chain" in CallAndMessageChecker. But this isn't the case.
>
> So perhaps I could rewrite RegionWalker to make use of this, though your
> points make it seem much less useful than I had thought.
>
> The original motivation was to avoid duplicating code between
> CallAndMessageChecker, and this new const buffer checker idea. None of the
> other checkers yet have the same behavior. I'm feeling cautious about the
> new check as well -- there /are/ cases where a const pointer is never
> dereferenced, and so it doesn't matter if its pointee is completely
> uninitialized.
>
> Perhaps this should be put on hold in favor of work with a clearer payoff.
> Thanks for pointing out the problems with this.

Yeah, there are a variety of issues here.  To achieve scalability with our symbolic stores, we will need to make more things implicit or lazy.  I've realized through my own experimentation that explicit walking of regions and their bindings, while conceptually simple, doesn't always mesh well with this goal.  This probably means that we need to gradually evolve the APIs for querying the store to be more declarative, and then let the store figure out the most efficient way to execute that query based on its own internal implementation.

I'm mixed enough about the pass-by-value field analysis in CallAndMessageChecker that I'm steadily getting more inclined to remove it.  I don't think that particular feature of the check has found any bugs that I'm aware of, but I have seen plenty of false positives.

Thanks for thinking about this though, and please keep your code on the back burner.  We may decide to still use, albeit in possibly a different form.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyzer: Extract region-walking behavior from CallAndMessageChecker

Zhongxing Xu
Hi,

I agree with Ted that such explicit walking each element could be
expensive. But if such check shows some value, we can conditionally
turn it on with a command line option like
'-analyzer-check-uninit-fields' and let the user decide.

On Sat, Jun 26, 2010 at 2:27 PM, Ted Kremenek <[hidden email]> wrote:

> On Jun 23, 2010, at 6:52 PM, Jordy Rose wrote:
>
>>
>> On Wed, 23 Jun 2010 16:30:29 -0700, Ted Kremenek <[hidden email]>
>> wrote:
>>> On Jun 23, 2010, at 12:05 AM, Jordy Rose wrote:
>>>
>>>> CallAndMessageChecker currently has a simple check to see if any of the
>>>> members of a pass-by-value struct are uninitialized. It handles nested
>>>> structs, but not structs containing arrays.
>>>>
>>>> I propose to extract this region-walking behavior out into a new class,
>>>> RegionWalker. This would then be the basis for the current
>> pass-by-value
>>>> check and a possible new one (see below). My implementation of
>>>> RegionWalker
>>>> is loosely modeled after RecursiveASTVisitor (see attached patch), and
>>>> supports both structs and arrays, and both nested.
>>>
>>> Hi Jordy,
>>>
>>> I think in principle the addition of RegionWalker is a nice refactoring,
>>> but I'm really concerned about the following:
>>>
>>> +    const ElementRegion *ER = MemMgr.getElementRegion(EleTy, IndexVal,
>> R,
>>> Ctx);
>>>
>>> This call means that we will create a new ElementRegion for accessing
>>> every single element of an array.  This is really expensive.  Not only
>> will
>>> this be really slow for large constant-sized arrays, it will cause a
>> bunch
>>> of ElementRegion objects to get generated that will stay around for the
>>> entire lifetime of the GRExprEngine object.
>>>
>>> As is, I don't think this can go in.
>>
>> I was concerned about this too, but couldn't think of another way to
>> actually visit every scalar element of a complicated nesting of structs and
>> arrays. On the face of it, this could be solved with stack-based
>> ElementRegion and FieldRegion objects...apart from being the only such case
>> of stack-based regions, and a slightly distasteful idea. *grin*
>>
>> But due to your other objections, perhaps it is not a useful finding.
>> Perhaps if a pass-by-value struct had /no/ initialized members, it'd be
>> worth warning...but again, there, the effort to actually walk all the
>> subregions might not be worth it.
>>
>>
>>> My high-level question is what is the purpose of this API?  Is to
>> iterate
>>> over "direct" bindings, or all the values in memory?  The StoreManager
>> does
>>> support an API to iterate over bindings, but the behavior will be highly
>>> dependent on the implementation of the StoreManager.  We really need to
>>> design these APIs with algorithmic efficiency in mind. If we need to add
>>> more high-level APIs to StoreManager to make the kind of queries we want
>> to
>>> do more efficient than I prefer we do that.  RegionWalker right now
>> defeats
>>> all the laziness of RegionStore by instantiation regions on-the-fly.
>> This
>>> is going to be a huge performance killer.
>>
>> Huh. I had originally thought that RegionStore's subregion map was flat,
>> that /any/ subregion showed up under the main one. Which wouldn't allow for
>> the nice "field chain" in CallAndMessageChecker. But this isn't the case.
>>
>> So perhaps I could rewrite RegionWalker to make use of this, though your
>> points make it seem much less useful than I had thought.
>>
>> The original motivation was to avoid duplicating code between
>> CallAndMessageChecker, and this new const buffer checker idea. None of the
>> other checkers yet have the same behavior. I'm feeling cautious about the
>> new check as well -- there /are/ cases where a const pointer is never
>> dereferenced, and so it doesn't matter if its pointee is completely
>> uninitialized.
>>
>> Perhaps this should be put on hold in favor of work with a clearer payoff.
>> Thanks for pointing out the problems with this.
>
> Yeah, there are a variety of issues here.  To achieve scalability with our symbolic stores, we will need to make more things implicit or lazy.  I've realized through my own experimentation that explicit walking of regions and their bindings, while conceptually simple, doesn't always mesh well with this goal.  This probably means that we need to gradually evolve the APIs for querying the store to be more declarative, and then let the store figure out the most efficient way to execute that query based on its own internal implementation.
>
> I'm mixed enough about the pass-by-value field analysis in CallAndMessageChecker that I'm steadily getting more inclined to remove it.  I don't think that particular feature of the check has found any bugs that I'm aware of, but I have seen plenty of false positives.
>
> Thanks for thinking about this though, and please keep your code on the back burner.  We may decide to still use, albeit in possibly a different form.
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyzer: Extract region-walking behavior from CallAndMessageChecker

Ted Kremenek
To clarify, I think I was trying to make two points.  The first was that the direct walking was likely to be prohibitively expensive.  The second was that the check was likely to yield many false positives.  For the first point, we can possibly deal with the efficiency problem by rephrasing the task as some kind of query on the StoreManager that we specifically have the StoreManager implement in some efficient way.

On Jun 26, 2010, at 9:19 PM, Zhongxing Xu wrote:

> Hi,
>
> I agree with Ted that such explicit walking each element could be
> expensive. But if such check shows some value, we can conditionally
> turn it on with a command line option like
> '-analyzer-check-uninit-fields' and let the user decide.


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyzer: Extract region-walking behavior from CallAndMessageChecker

Zhongxing Xu
With a command line option, the user could decide to turn it on if he
can tolerate the cost and knows his code should not have uninitialized
fields.

On Sun, Jun 27, 2010 at 1:23 PM, Ted Kremenek <[hidden email]> wrote:

> To clarify, I think I was trying to make two points.  The first was that the direct walking was likely to be prohibitively expensive.  The second was that the check was likely to yield many false positives.  For the first point, we can possibly deal with the efficiency problem by rephrasing the task as some kind of query on the StoreManager that we specifically have the StoreManager implement in some efficient way.
>
> On Jun 26, 2010, at 9:19 PM, Zhongxing Xu wrote:
>
>> Hi,
>>
>> I agree with Ted that such explicit walking each element could be
>> expensive. But if such check shows some value, we can conditionally
>> turn it on with a command line option like
>> '-analyzer-check-uninit-fields' and let the user decide.
>
>

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyzer: Extract region-walking behavior from CallAndMessageChecker

Ted Kremenek
I understand your point, but I think I still strongly disagree here.

I'm fine with the checker behavior being controlled by an option, but just having the code go in as is (with these obvious algorithmic performance issues) sets a bad precedent for how we think things should "get done" in the analyzer.  The current check should probably be changed as well because of this reason.

Implementing this the right way involves extending StoreManager with the right APIs so that the query can get executed efficiently.  We shouldn't just hack things into the analyzer because it is immediately convenient, especially when doing it right isn't necessarily that hard.  Having good design and thinking these issues out at the appropriate times is well worth it in the long run, as hacks have a way of piling up.

On Jun 26, 2010, at 10:33 PM, Zhongxing Xu wrote:

> With a command line option, the user could decide to turn it on if he
> can tolerate the cost and knows his code should not have uninitialized
> fields.
>
> On Sun, Jun 27, 2010 at 1:23 PM, Ted Kremenek <[hidden email]> wrote:
>> To clarify, I think I was trying to make two points.  The first was that the direct walking was likely to be prohibitively expensive.  The second was that the check was likely to yield many false positives.  For the first point, we can possibly deal with the efficiency problem by rephrasing the task as some kind of query on the StoreManager that we specifically have the StoreManager implement in some efficient way.
>>
>> On Jun 26, 2010, at 9:19 PM, Zhongxing Xu wrote:
>>
>>> Hi,
>>>
>>> I agree with Ted that such explicit walking each element could be
>>> expensive. But if such check shows some value, we can conditionally
>>> turn it on with a command line option like
>>> '-analyzer-check-uninit-fields' and let the user decide.
>>
>>


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyzer: Extract region-walking behavior from CallAndMessageChecker

Zhongxing Xu
On Tue, Jun 29, 2010 at 6:54 AM, Ted Kremenek <[hidden email]> wrote:
> I understand your point, but I think I still strongly disagree here.
>
> I'm fine with the checker behavior being controlled by an option, but just having the code go in as is (with these obvious algorithmic performance issues) sets a bad precedent for how we think things should "get done" in the analyzer.  The current check should probably be changed as well because of this reason.
>
> Implementing this the right way involves extending StoreManager with the right APIs so that the query can get executed efficiently.  We shouldn't just hack things into the analyzer because it is immediately convenient, especially when doing it right isn't necessarily that hard.  Having good design and thinking these issues out at the appropriate times is well worth it in the long run, as hacks have a way of piling up.

Make sense.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyzer: Extract region-walking behavior from CallAndMessageChecker

Jordy Rose
In reply to this post by Ted Kremenek

I'll rewrite it during some random bit of downtime to only walk the known
subregions, so it doesn't get completely lost.


On Mon, 28 Jun 2010 15:54:37 -0700, Ted Kremenek <[hidden email]>
wrote:
> I understand your point, but I think I still strongly disagree here.
>
> I'm fine with the checker behavior being controlled by an option, but
just
> having the code go in as is (with these obvious algorithmic performance
> issues) sets a bad precedent for how we think things should "get done"
in
> the analyzer.  The current check should probably be changed as well
because
> of this reason.
>
> Implementing this the right way involves extending StoreManager with the
> right APIs so that the query can get executed efficiently.  We shouldn't
> just hack things into the analyzer because it is immediately convenient,
> especially when doing it right isn't necessarily that hard.  Having good
> design and thinking these issues out at the appropriate times is well
worth
> it in the long run, as hacks have a way of piling up.
>
> On Jun 26, 2010, at 10:33 PM, Zhongxing Xu wrote:
>
>> With a command line option, the user could decide to turn it on if he
>> can tolerate the cost and knows his code should not have uninitialized
>> fields.
>>
>> On Sun, Jun 27, 2010 at 1:23 PM, Ted Kremenek <[hidden email]>
wrote:
>>> To clarify, I think I was trying to make two points.  The first was
>>> that the direct walking was likely to be prohibitively expensive.  The
>>> second was that the check was likely to yield many false positives.
For

>>> the first point, we can possibly deal with the efficiency problem by
>>> rephrasing the task as some kind of query on the StoreManager that we
>>> specifically have the StoreManager implement in some efficient way.
>>>
>>> On Jun 26, 2010, at 9:19 PM, Zhongxing Xu wrote:
>>>
>>>> Hi,
>>>>
>>>> I agree with Ted that such explicit walking each element could be
>>>> expensive. But if such check shows some value, we can conditionally
>>>> turn it on with a command line option like
>>>> '-analyzer-check-uninit-fields' and let the user decide.
>>>
>>>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev