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 <[email protected]>
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;
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 <[email protected]>
>
> 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
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 <[email protected]>
> >
> > 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.
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 <[email protected]>
>>>
>>> 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
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 <[email protected]>
> > >
> > > 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
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 <[email protected]>
> > > >
> > > > 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.
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