Proposal: -Wshadow-field flag

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

Proposal: -Wshadow-field flag

Ehsan Akhgari
Hi everyone,

I would like to propose adding a -Wshadow-field to clang.  This flag would emit a warning if a derived class declares a member with the same name as one in one of the base classes, regardless of visibility.

Here is the rationale: In the Mozilla code base, there are fields that are often declared with the same name in thousands of classes, either by convention or through macros.  One concrete example is mRefCnt, which is the variable holding the object's reference count.  Every once in a while, we find bugs that are caused by the derived class inadvertently shadowing the base member, and therefore code in the derived class and its descendants in the hierarchy will access the wrong variable.  In the case of mRefCnt, for example, these bugs are security sensitive since this typically causes the reference count of the object to get out of balance, causing use-after-free issues.  One can conceive of other similar problems with other fields as well.

I was going to implement this warning in the clang plugin that we use for static analysis of our code base, but realized that this may probably be useful for other projects as well.  Is there any interest in this feature?  If yes, I would be happy to submit a patch.

Cheers,
--
Ehsan

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

Re: Proposal: -Wshadow-field flag

Reid Kleckner-3
I think this would be a great addition! In general, -Wshadow fires in a lot of situations, and I think having more granularity here is helpful for users.

On Sat, Apr 18, 2015 at 11:05 AM, Ehsan Akhgari <[hidden email]> wrote:
Hi everyone,

I would like to propose adding a -Wshadow-field to clang.  This flag would emit a warning if a derived class declares a member with the same name as one in one of the base classes, regardless of visibility.

Here is the rationale: In the Mozilla code base, there are fields that are often declared with the same name in thousands of classes, either by convention or through macros.  One concrete example is mRefCnt, which is the variable holding the object's reference count.  Every once in a while, we find bugs that are caused by the derived class inadvertently shadowing the base member, and therefore code in the derived class and its descendants in the hierarchy will access the wrong variable.  In the case of mRefCnt, for example, these bugs are security sensitive since this typically causes the reference count of the object to get out of balance, causing use-after-free issues.  One can conceive of other similar problems with other fields as well.

I was going to implement this warning in the clang plugin that we use for static analysis of our code base, but realized that this may probably be useful for other projects as well.  Is there any interest in this feature?  If yes, I would be happy to submit a patch.

Cheers,
--
Ehsan

_______________________________________________
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: Proposal: -Wshadow-field flag

Ehsan Akhgari
On Mon, Apr 20, 2015 at 5:33 PM, Reid Kleckner <[hidden email]> wrote:
I think this would be a great addition! In general, -Wshadow fires in a lot of situations, and I think having more granularity here is helpful for users.

Note that -Wshadow doesn't diagnose this specific case, so my proposal isn't really a subset of -Wshadow.
 
On Sat, Apr 18, 2015 at 11:05 AM, Ehsan Akhgari <[hidden email]> wrote:
Hi everyone,

I would like to propose adding a -Wshadow-field to clang.  This flag would emit a warning if a derived class declares a member with the same name as one in one of the base classes, regardless of visibility.

Here is the rationale: In the Mozilla code base, there are fields that are often declared with the same name in thousands of classes, either by convention or through macros.  One concrete example is mRefCnt, which is the variable holding the object's reference count.  Every once in a while, we find bugs that are caused by the derived class inadvertently shadowing the base member, and therefore code in the derived class and its descendants in the hierarchy will access the wrong variable.  In the case of mRefCnt, for example, these bugs are security sensitive since this typically causes the reference count of the object to get out of balance, causing use-after-free issues.  One can conceive of other similar problems with other fields as well.

I was going to implement this warning in the clang plugin that we use for static analysis of our code base, but realized that this may probably be useful for other projects as well.  Is there any interest in this feature?  If yes, I would be happy to submit a patch.

Cheers,
--
Ehsan

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev





--
Ehsan

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

Re: Proposal: -Wshadow-field flag

Reid Kleckner-3
On Tue, Apr 21, 2015 at 2:01 PM, Ehsan Akhgari <[hidden email]> wrote:
On Mon, Apr 20, 2015 at 5:33 PM, Reid Kleckner <[hidden email]> wrote:
I think this would be a great addition! In general, -Wshadow fires in a lot of situations, and I think having more granularity here is helpful for users.

Note that -Wshadow doesn't diagnose this specific case, so my proposal isn't really a subset of -Wshadow.

Right, this sounds like a new and useful warning by itself.

I was mostly saying this seems like the right direction. The existing checks under -Wshadow are both too little and too much. Having more checks and more granular flags will help us explore the space.

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

Re: Proposal: -Wshadow-field flag

David Blaikie
FWIW I'd be a bit concerned that this warning, while it might have a good false positive rate on a codebase with the particular idioms you've described, would have a false positive rate a bit too high for a normal diagnostic bar.

Only one way to find out, though.

On Tue, Apr 21, 2015 at 2:30 PM, Reid Kleckner <[hidden email]> wrote:
On Tue, Apr 21, 2015 at 2:01 PM, Ehsan Akhgari <[hidden email]> wrote:
On Mon, Apr 20, 2015 at 5:33 PM, Reid Kleckner <[hidden email]> wrote:
I think this would be a great addition! In general, -Wshadow fires in a lot of situations, and I think having more granularity here is helpful for users.

Note that -Wshadow doesn't diagnose this specific case, so my proposal isn't really a subset of -Wshadow.

Right, this sounds like a new and useful warning by itself.

I was mostly saying this seems like the right direction. The existing checks under -Wshadow are both too little and too much. Having more checks and more granular flags will help us explore the space.

_______________________________________________
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: Proposal: -Wshadow-field flag

Ehsan Akhgari
In reply to this post by Reid Kleckner-3
On Tue, Apr 21, 2015 at 5:30 PM, Reid Kleckner <[hidden email]> wrote:
On Tue, Apr 21, 2015 at 2:01 PM, Ehsan Akhgari <[hidden email]> wrote:
On Mon, Apr 20, 2015 at 5:33 PM, Reid Kleckner <[hidden email]> wrote:
I think this would be a great addition! In general, -Wshadow fires in a lot of situations, and I think having more granularity here is helpful for users.

Note that -Wshadow doesn't diagnose this specific case, so my proposal isn't really a subset of -Wshadow.

Right, this sounds like a new and useful warning by itself.

Great to hear that.  I'll give it a shot then.  :-)
 
I was mostly saying this seems like the right direction. The existing checks under -Wshadow are both too little and too much. Having more checks and more granular flags will help us explore the space.

Yeah, agreed.  For those interested, this is a good read on why we decided to not turn on -Wshadow whole-sale for Mozilla: <https://bugzilla.mozilla.org/show_bug.cgi?id=563195#c15>.

Cheers,
--
Ehsan

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

Re: Proposal: -Wshadow-field flag

Ehsan Akhgari
In reply to this post by David Blaikie
On Tue, Apr 21, 2015 at 5:41 PM, David Blaikie <[hidden email]> wrote:
FWIW I'd be a bit concerned that this warning, while it might have a good false positive rate on a codebase with the particular idioms you've described, would have a false positive rate a bit too high for a normal diagnostic bar.

Only one way to find out, though.

That is a good point.

FWIW, I don't think we should turn this warning on as part of -Wall, maybe as part of -Wextra?  I'm not at all familiar with what the usual process for turning on warnings by default looks like, so I would appreciate if someone can point me in the right direction there.

Cheers,
Ehsan
 
On Tue, Apr 21, 2015 at 2:30 PM, Reid Kleckner <[hidden email]> wrote:
On Tue, Apr 21, 2015 at 2:01 PM, Ehsan Akhgari <[hidden email]> wrote:
On Mon, Apr 20, 2015 at 5:33 PM, Reid Kleckner <[hidden email]> wrote:
I think this would be a great addition! In general, -Wshadow fires in a lot of situations, and I think having more granularity here is helpful for users.

Note that -Wshadow doesn't diagnose this specific case, so my proposal isn't really a subset of -Wshadow.

Right, this sounds like a new and useful warning by itself.

I was mostly saying this seems like the right direction. The existing checks under -Wshadow are both too little and too much. Having more checks and more granular flags will help us explore the space.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev





--
Ehsan

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

Re: Proposal: -Wshadow-field flag

Reid Kleckner-3
On Tue, Apr 21, 2015 at 3:08 PM, Ehsan Akhgari <[hidden email]> wrote:
On Tue, Apr 21, 2015 at 5:41 PM, David Blaikie <[hidden email]> wrote:
FWIW I'd be a bit concerned that this warning, while it might have a good false positive rate on a codebase with the particular idioms you've described, would have a false positive rate a bit too high for a normal diagnostic bar.

Only one way to find out, though.

That is a good point.

FWIW, I don't think we should turn this warning on as part of -Wall, maybe as part of -Wextra?  I'm not at all familiar with what the usual process for turning on warnings by default looks like, so I would appreciate if someone can point me in the right direction there.

Confusingly, "on by default" and "enabled by -Wall" are distinct in Clang, but I don't find the distinction is not very useful.

I would suggest adding the warning under -Wshadow-field, and make it a subgroup of -Wshadow. This way, -Wshadow users will get new warnings that they probably wanted anyway. -Wshadow is not part of -Wall, so -Wall won't enable it. Mark each new diagnostic as DefaultIgnore in DiagnosticSemaKinds.td, so that it will not be on by default.

Once it's in, we can try it out, and if it has a really low false-positive rate, then we can discuss enabling it by default and/or putting it in -Wall.

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

Re: Proposal: -Wshadow-field flag

David Blaikie
On Tue, Apr 21, 2015 at 3:29 PM, Reid Kleckner <[hidden email]> wrote:

> On Tue, Apr 21, 2015 at 3:08 PM, Ehsan Akhgari <[hidden email]>
> wrote:
>>
>> On Tue, Apr 21, 2015 at 5:41 PM, David Blaikie <[hidden email]> wrote:
>>>
>>> FWIW I'd be a bit concerned that this warning, while it might have a good
>>> false positive rate on a codebase with the particular idioms you've
>>> described, would have a false positive rate a bit too high for a normal
>>> diagnostic bar.
>>>
>>> Only one way to find out, though.
>>
>>
>> That is a good point.
>>
>> FWIW, I don't think we should turn this warning on as part of -Wall, maybe
>> as part of -Wextra?  I'm not at all familiar with what the usual process for
>> turning on warnings by default looks like, so I would appreciate if someone
>> can point me in the right direction there.
>
>
> Confusingly, "on by default" and "enabled by -Wall" are distinct in Clang,
> but I don't find the distinction is not very useful.
>
> I would suggest adding the warning under -Wshadow-field, and make it a
> subgroup of -Wshadow. This way, -Wshadow users will get new warnings that
> they probably wanted anyway.

The part of this warning that I'm not sure the 'normal' user of
-Wshadow need is where it warns regardless of access control - for
most users I wouldn't imagine it's important that my base class has a
private field called 'foo' when my derived class has a field called
'foo'.

> -Wshadow is not part of -Wall, so -Wall won't
> enable it. Mark each new diagnostic as DefaultIgnore in
> DiagnosticSemaKinds.td, so that it will not be on by default.
>
> Once it's in, we can try it out, and if it has a really low false-positive
> rate, then we can discuss enabling it by default and/or putting it in -Wall.

Historically there's been a fair bit of push back on off-by-default
warnings as they tend to add weight to the compiler without much in
the way of usage or quality checks (because of a lack of usage), but
perhaps this attitude has changed.

- David
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev