[PATCH] mm/ksm.c - Fix compile warnings (PS ignore my formatting in previous email)

[PATCH] mm/ksm.c - Fix compile warnings (PS ignore my formatting in previous email)

[askb]

Found the compiler warning on linux-next: 	

mm/ksm.c: In function ‘ksm_scan_thread’:
mm/ksm.c:1083: warning: ‘page2[0u]’ may be used uninitialized in this
function
mm/ksm.c:1083: note: ‘page2[0u]’ was declared here

fix for the above warning:

Signed-off-by: Anil SB <askb23@gmail.com>

diff --git a/mm/ksm.c b/mm/ksm.c
index bef1af4..2ea0fd3 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1080,7 +1080,7 @@ static void stable_tree_append(struct rmap_item *rmap_item,
  */
 static void cmp_and_merge_page(struct page *page, struct rmap_item *rmap_item)
 {
-	struct page *page2[1];
+	struct page *page2[1] = {NULL};
 	struct rmap_item *tree_rmap_item;
 	unsigned int checksum;
 	int err;

Re: [PATCH] mm/ksm.c - Fix compile warnings (PS ignore my formatting in previous email)

[walter harms]

askb schrieb:
> Found the compiler warning on linux-next: 	
> 
> mm/ksm.c: In function ‘ksm_scan_thread’:
> mm/ksm.c:1083: warning: ‘page2[0u]’ may be used uninitialized in this
> function
> mm/ksm.c:1083: note: ‘page2[0u]’ was declared here
> 
> fix for the above warning:
> 
> Signed-off-by: Anil SB <askb23@gmail.com>
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index bef1af4..2ea0fd3 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -1080,7 +1080,7 @@ static void stable_tree_append(struct rmap_item *rmap_item,
>   */
>  static void cmp_and_merge_page(struct page *page, struct rmap_item *rmap_item)
>  {
> -	struct page *page2[1];
> +	struct page *page2[1] = {NULL};
>  	struct rmap_item *tree_rmap_item;
>  	unsigned int checksum;
>  	int err;
> 
> 

a 1 element array looks strange, did you look what kind of magic is here used ?

re,
 wh

Re: [PATCH] mm/ksm.c - Fix compile warnings (PS ignore my formatting in previous email)

[askb]

On Wed, 2009-11-04 at 10:57 +0100, walter harms wrote:
> 
> askb schrieb:
> > Found the compiler warning on linux-next: 	
> > 
> > mm/ksm.c: In function ‘ksm_scan_thread’:
> > mm/ksm.c:1083: warning: ‘page2[0u]’ may be used uninitialized in this
> > function
> > mm/ksm.c:1083: note: ‘page2[0u]’ was declared here
> > 
> > fix for the above warning:
> > 
> > Signed-off-by: Anil SB <askb23@gmail.com>
> > 
> > diff --git a/mm/ksm.c b/mm/ksm.c
> > index bef1af4..2ea0fd3 100644
> > --- a/mm/ksm.c
> > +++ b/mm/ksm.c
> > @@ -1080,7 +1080,7 @@ static void stable_tree_append(struct rmap_item *rmap_item,
> >   */
> >  static void cmp_and_merge_page(struct page *page, struct rmap_item *rmap_item)
> >  {
> > -	struct page *page2[1];
> > +	struct page *page2[1] = {NULL};
> >  	struct rmap_item *tree_rmap_item;
> >  	unsigned int checksum;
> >  	int err;
> > 
> > 
> 
> a 1 element array looks strange, did you look what kind of magic is here used ?
> 
>From my understanding, it is used for place holder to check and return
an identical page from stable_tree_search(). Can we do with a double
indirection to struct page instead of the above? 
PS point me in the right direction.

Regards.

Re: [PATCH] mm/ksm.c - Fix compile warnings (PS ignore my formatting in previous email)

[walter harms]

askb schrieb:
> On Wed, 2009-11-04 at 10:57 +0100, walter harms wrote:
>> askb schrieb:
>>> Found the compiler warning on linux-next: 	
>>>
>>> mm/ksm.c: In function ‘ksm_scan_thread’:
>>> mm/ksm.c:1083: warning: ‘page2[0u]’ may be used uninitialized in this
>>> function
>>> mm/ksm.c:1083: note: ‘page2[0u]’ was declared here
>>>
>>> fix for the above warning:
>>>
>>> Signed-off-by: Anil SB <askb23@gmail.com>
>>>
>>> diff --git a/mm/ksm.c b/mm/ksm.c
>>> index bef1af4..2ea0fd3 100644
>>> --- a/mm/ksm.c
>>> +++ b/mm/ksm.c
>>> @@ -1080,7 +1080,7 @@ static void stable_tree_append(struct rmap_item *rmap_item,
>>>   */
>>>  static void cmp_and_merge_page(struct page *page, struct rmap_item *rmap_item)
>>>  {
>>> -	struct page *page2[1];
>>> +	struct page *page2[1] = {NULL};
>>>  	struct rmap_item *tree_rmap_item;
>>>  	unsigned int checksum;
>>>  	int err;
>>>
>>>
>> a 1 element array looks strange, did you look what kind of magic is here used ?
>>
>>From my understanding, it is used for place holder to check and return
> an identical page from stable_tree_search(). Can we do with a double
> indirection to struct page instead of the above? 
> PS point me in the right direction.
> 

1. sorry for delay.

2. there is no "right" direction. the result should be clear for the code, and this
may change between releases, special circumstances may occur and so on.

You may try something like that:

struct page *dst ;

	function ( &dst );

IMHO that would clarify the code and fix the naming (page,page2 is not very descriptiv).


re,
 wh

Re: [PATCH] mm/ksm.c - Fix compile warnings (PS ignore my formatting in previous email)

[Hugh Dickins]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2225 bytes --]

On Wed, 4 Nov 2009, askb wrote:
> On Wed, 2009-11-04 at 10:57 +0100, walter harms wrote:
> > 
> > askb schrieb:
> > > Found the compiler warning on linux-next: 	
> > > 
> > > mm/ksm.c: In function ‘ksm_scan_thread’:
> > > mm/ksm.c:1083: warning: ‘page2[0u]’ may be used uninitialized in this
> > > function
> > > mm/ksm.c:1083: note: ‘page2[0u]’ was declared here
> > > 
> > > fix for the above warning:
> > > 
> > > Signed-off-by: Anil SB <askb23@gmail.com>
> > > 
> > > diff --git a/mm/ksm.c b/mm/ksm.c
> > > index bef1af4..2ea0fd3 100644
> > > --- a/mm/ksm.c
> > > +++ b/mm/ksm.c
> > > @@ -1080,7 +1080,7 @@ static void stable_tree_append(struct rmap_item *rmap_item,
> > >   */
> > >  static void cmp_and_merge_page(struct page *page, struct rmap_item *rmap_item)
> > >  {
> > > -	struct page *page2[1];
> > > +	struct page *page2[1] = {NULL};
> > >  	struct rmap_item *tree_rmap_item;
> > >  	unsigned int checksum;
> > >  	int err;
> > > 
> > > 
> > 
> > a 1 element array looks strange, did you look what kind of magic is here used ?
> > 
> >From my understanding, it is used for place holder to check and return
> an identical page from stable_tree_search(). Can we do with a double
> indirection to struct page instead of the above? 
> PS point me in the right direction.

I don't particularly like things like page2[1] myself, but there's
nothing actually wrong with it, so no urgency to change it.

mm/ksm.c is under active development (better Cc the people involved
with a sourcefile when considering changes to it), and I have a
collection of patches under testing, which do change some names
around here (along with more serious changes).  So, thanks for looking
at this, but I'd prefer to avoid the interference of a trivial patch
at the moment - sorry.

Which version of the compiler gave you that uninitialized warning?
A version which lots of people are using, or just some snapshot?
I've never seen a warning there myself, but they do keep "refining"
that logic.

Hugh's hypothesis: for every variable x initialized by a subfunction,
there exists at least one version V of gcc, such that V reports that
x may be used uninitialized.

Hugh

Re: [PATCH] mm/ksm.c - Fix compile warnings (PS ignore my formatting in previous email)

[askb]

On Mon, 2009-11-09 at 10:51 +0000, Hugh Dickins wrote:
> On Wed, 4 Nov 2009, askb wrote:
> > On Wed, 2009-11-04 at 10:57 +0100, walter harms wrote:
> > > 
> > > askb schrieb:
> > > > Found the compiler warning on linux-next: 	
> > > > 
> > > > mm/ksm.c: In function ‘ksm_scan_thread’:
> > > > mm/ksm.c:1083: warning: ‘page2[0u]’ may be used uninitialized in this
> > > > function
> > > > mm/ksm.c:1083: note: ‘page2[0u]’ was declared here
> > > > 
> > > > fix for the above warning:
> > > > 
> > > > Signed-off-by: Anil SB <askb23@gmail.com>
> > > > 
> > > > diff --git a/mm/ksm.c b/mm/ksm.c
> > > > index bef1af4..2ea0fd3 100644
> > > > --- a/mm/ksm.c
> > > > +++ b/mm/ksm.c
> > > > @@ -1080,7 +1080,7 @@ static void stable_tree_append(struct rmap_item *rmap_item,
> > > >   */
> > > >  static void cmp_and_merge_page(struct page *page, struct rmap_item *rmap_item)
> > > >  {
> > > > -	struct page *page2[1];
> > > > +	struct page *page2[1] = {NULL};
> > > >  	struct rmap_item *tree_rmap_item;
> > > >  	unsigned int checksum;
> > > >  	int err;
> > > > 
> > > > 
> > > 
> > > a 1 element array looks strange, did you look what kind of magic is here used ?
> > > 
> > >From my understanding, it is used for place holder to check and return
> > an identical page from stable_tree_search(). Can we do with a double
> > indirection to struct page instead of the above? 
> > PS point me in the right direction.
> 
> I don't particularly like things like page2[1] myself, but there's
> nothing actually wrong with it, so no urgency to change it.
> 
> mm/ksm.c is under active development (better Cc the people involved
> with a sourcefile when considering changes to it), and I have a
> collection of patches under testing, which do change some names
> around here (along with more serious changes).  So, thanks for looking
> at this, but I'd prefer to avoid the interference of a trivial patch
> at the moment - sorry.
> 
> Which version of the compiler gave you that uninitialized warning?

gcc version 4.3.2 [gcc-4_3-branch revision 141291] (SUSE Linux)

> A version which lots of people are using, or just some snapshot?
> I've never seen a warning there myself, but they do keep "refining"
> that logic.
> 
I got the warning while doing "make randconfig". However, now after a
cleanup and redoing the same, the warning is not seen.

> Hugh's hypothesis: for every variable x initialized by a subfunction,
> there exists at least one version V of gcc, such that V reports that
> x may be used uninitialized.
> 
> Hugh

Thanks.

Re: [PATCH] mm/ksm.c - Fix compile warnings (PS ignore my formatting in previous email)

[Hugh Dickins]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1227 bytes --]

On Mon, 9 Nov 2009, askb wrote:
> On Mon, 2009-11-09 at 10:51 +0000, Hugh Dickins wrote:
> > On Wed, 4 Nov 2009, askb wrote:
> > > On Wed, 2009-11-04 at 10:57 +0100, walter harms wrote:
> > > > askb schrieb:
> > > > > Found the compiler warning on linux-next: 	
> > > > > 
> > > > > mm/ksm.c: In function ‘ksm_scan_thread’:
> > > > > mm/ksm.c:1083: warning: ‘page2[0u]’ may be used uninitialized in this
> > > > > function
> > > > > mm/ksm.c:1083: note: ‘page2[0u]’ was declared here
...
> > Which version of the compiler gave you that uninitialized warning?
> 
> gcc version 4.3.2 [gcc-4_3-branch revision 141291] (SUSE Linux)

Thanks: funny, that's the one I've been using most too.

> 
> > A version which lots of people are using, or just some snapshot?
> > I've never seen a warning there myself, but they do keep "refining"
> > that logic.
> > 
> I got the warning while doing "make randconfig". However, now after a
> cleanup and redoing the same, the warning is not seen.

I just tried the most obvious alternatives (CC_OPTIMIZE_FOR_SIZE,
OPTIMIZE_INLINING, off and on), but still didn't see the warning.

Never mind, I've shoved a NULL in my tree, but no rush for this.

Hugh