| Message ID | 20201118115646.2461726-2-kuhn.chenqun@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | two qtest bugfix | expand |
On 11/18/20 12:56 PM, Chen Qun wrote: > According to the glib function requirements, we need initialise > the variable. Otherwise there will be compilation warnings: > > glib-autocleanups.h:28:3: warning: ‘full_name’ may be > used uninitialized in this function [-Wmaybe-uninitialized] > 28 | g_free (*pp); > | ^~~~~~~~~~~~ > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> > --- > tests/qtest/npcm7xx_timer-test.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/tests/qtest/npcm7xx_timer-test.c b/tests/qtest/npcm7xx_timer-test.c > index f08b0cd62a..83774a5b90 100644 > --- a/tests/qtest/npcm7xx_timer-test.c > +++ b/tests/qtest/npcm7xx_timer-test.c > @@ -512,11 +512,9 @@ static void test_disable_on_expiration(gconstpointer test_data) > */ > static void tim_add_test(const char *name, const TestData *td, GTestDataFunc fn) > { Or: > - g_autofree char *full_name; + g_autofree char *full_name = NULL; > - > - full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s", > - tim_index(td->tim), timer_index(td->timer), > - name); > + g_autofree char *full_name = g_strdup_printf( > + "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim), > + timer_index(td->timer), name); > qtest_add_data_func(full_name, td, fn); > } > >
> -----Original Message----- > From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com] > Sent: Wednesday, November 18, 2020 8:14 PM > To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-devel@nongnu.org; > qemu-trivial@nongnu.org > Cc: lvivier@redhat.com; peter.maydell@linaro.org; thuth@redhat.com; > Zhanghailiang <zhang.zhanghailiang@huawei.com>; > hskinnemoen@google.com; wuhaotsh@google.com; Euler Robot > <euler.robot@huawei.com> > Subject: Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree > need to be initialized > > On 11/18/20 12:56 PM, Chen Qun wrote: > > According to the glib function requirements, we need initialise the > > variable. Otherwise there will be compilation warnings: > > > > glib-autocleanups.h:28:3: warning: ‘full_name’ may be used > > uninitialized in this function [-Wmaybe-uninitialized] > > 28 | g_free (*pp); > > | ^~~~~~~~~~~~ > > > > Reported-by: Euler Robot <euler.robot@huawei.com> > > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> > > --- > > tests/qtest/npcm7xx_timer-test.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/tests/qtest/npcm7xx_timer-test.c > > b/tests/qtest/npcm7xx_timer-test.c > > index f08b0cd62a..83774a5b90 100644 > > --- a/tests/qtest/npcm7xx_timer-test.c > > +++ b/tests/qtest/npcm7xx_timer-test.c > > @@ -512,11 +512,9 @@ static void > test_disable_on_expiration(gconstpointer test_data) > > */ > > static void tim_add_test(const char *name, const TestData *td, > > GTestDataFunc fn) { > > Or: > > > - g_autofree char *full_name; > + g_autofree char *full_name = NULL; > Yes, this also meets the glib requirements. But, the assignment statement following is not complex, so we could do both of variable definition and assignment in a statement. Thanks, Chen Qun > > - > > - full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s", > > - tim_index(td->tim), > timer_index(td->timer), > > - name); > > + g_autofree char *full_name = g_strdup_printf( > > + "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim), > > + timer_index(td->timer), name); > > qtest_add_data_func(full_name, td, fn); } > > > >
On Wed, Nov 18, 2020 at 3:57 AM Chen Qun <kuhn.chenqun@huawei.com> wrote: > > According to the glib function requirements, we need initialise > the variable. Otherwise there will be compilation warnings: > > glib-autocleanups.h:28:3: warning: ‘full_name’ may be > used uninitialized in this function [-Wmaybe-uninitialized] > 28 | g_free (*pp); > | ^~~~~~~~~~~~ > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com> I'd be totally fine with Philippe's suggestion too. Thanks! > --- > tests/qtest/npcm7xx_timer-test.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/tests/qtest/npcm7xx_timer-test.c b/tests/qtest/npcm7xx_timer-test.c > index f08b0cd62a..83774a5b90 100644 > --- a/tests/qtest/npcm7xx_timer-test.c > +++ b/tests/qtest/npcm7xx_timer-test.c > @@ -512,11 +512,9 @@ static void test_disable_on_expiration(gconstpointer test_data) > */ > static void tim_add_test(const char *name, const TestData *td, GTestDataFunc fn) > { > - g_autofree char *full_name; > - > - full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s", > - tim_index(td->tim), timer_index(td->timer), > - name); > + g_autofree char *full_name = g_strdup_printf( > + "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim), > + timer_index(td->timer), name); > qtest_add_data_func(full_name, td, fn); > } > > -- > 2.23.0 >
On 18/11/2020 13.13, Philippe Mathieu-Daudé wrote: > On 11/18/20 12:56 PM, Chen Qun wrote: >> According to the glib function requirements, we need initialise >> the variable. Otherwise there will be compilation warnings: >> >> glib-autocleanups.h:28:3: warning: ‘full_name’ may be >> used uninitialized in this function [-Wmaybe-uninitialized] >> 28 | g_free (*pp); >> | ^~~~~~~~~~~~ >> >> Reported-by: Euler Robot <euler.robot@huawei.com> >> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> >> --- >> tests/qtest/npcm7xx_timer-test.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/tests/qtest/npcm7xx_timer-test.c b/tests/qtest/npcm7xx_timer-test.c >> index f08b0cd62a..83774a5b90 100644 >> --- a/tests/qtest/npcm7xx_timer-test.c >> +++ b/tests/qtest/npcm7xx_timer-test.c >> @@ -512,11 +512,9 @@ static void test_disable_on_expiration(gconstpointer test_data) >> */ >> static void tim_add_test(const char *name, const TestData *td, GTestDataFunc fn) >> { > > Or: > >> - g_autofree char *full_name; > + g_autofree char *full_name = NULL; I think we better go with the current version of the patch - otherwise a supersmart new compiler version might warn again that the NULL that we assign here is never used... Reviewed-by: Thomas Huth <thuth@redhat.com>
On Wed, 18 Nov 2020 at 11:57, Chen Qun <kuhn.chenqun@huawei.com> wrote: > > According to the glib function requirements, we need initialise > the variable. Otherwise there will be compilation warnings: > > glib-autocleanups.h:28:3: warning: ‘full_name’ may be > used uninitialized in this function [-Wmaybe-uninitialized] > 28 | g_free (*pp); > | ^~~~~~~~~~~~ > > static void tim_add_test(const char *name, const TestData *td, GTestDataFunc fn) > { > - g_autofree char *full_name; > - > - full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s", > - tim_index(td->tim), timer_index(td->timer), > - name); > + g_autofree char *full_name = g_strdup_printf( > + "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim), > + timer_index(td->timer), name); Which compiler is so unintelligent that it cannot see that a declaration immediately followed by an assignment must always initialize the variable ??? thanks -- PMM
> -----Original Message----- > From: Peter Maydell [mailto:peter.maydell@linaro.org] > Sent: Thursday, November 19, 2020 4:26 PM > To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> > Cc: QEMU Developers <qemu-devel@nongnu.org>; QEMU Trivial > <qemu-trivial@nongnu.org>; Hao Wu <wuhaotsh@google.com>; Havard > Skinnemoen <hskinnemoen@google.com>; Zhanghailiang > <zhang.zhanghailiang@huawei.com>; Thomas Huth <thuth@redhat.com>; > Laurent Vivier <lvivier@redhat.com>; Euler Robot <euler.robot@huawei.com> > Subject: Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree > need to be initialized > > On Wed, 18 Nov 2020 at 11:57, Chen Qun <kuhn.chenqun@huawei.com> > wrote: > > > > According to the glib function requirements, we need initialise the > > variable. Otherwise there will be compilation warnings: > > > > glib-autocleanups.h:28:3: warning: ‘full_name’ may be used > > uninitialized in this function [-Wmaybe-uninitialized] > > 28 | g_free (*pp); > > | ^~~~~~~~~~~~ > > > > static void tim_add_test(const char *name, const TestData *td, > > GTestDataFunc fn) { > > - g_autofree char *full_name; > > - > > - full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s", > > - tim_index(td->tim), > timer_index(td->timer), > > - name); > > + g_autofree char *full_name = g_strdup_printf( > > + "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim), > > + timer_index(td->timer), name); > > Which compiler is so unintelligent that it cannot see that a declaration > immediately followed by an assignment must always initialize the variable ??? > Hi Peter, Glib requires that all g_auto* macros must be initialized. https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree Thanks, Chen Qun
On Thu, 19 Nov 2020 at 08:35, Chenqun (kuhn) <kuhn.chenqun@huawei.com> wrote: > > > -----Original Message----- > > > static void tim_add_test(const char *name, const TestData *td, > > > GTestDataFunc fn) { > > > - g_autofree char *full_name; > > > - > > > - full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s", > > > - tim_index(td->tim), > > timer_index(td->timer), > > > - name); > > > + g_autofree char *full_name = g_strdup_printf( > > > + "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim), > > > + timer_index(td->timer), name); > > > > Which compiler is so unintelligent that it cannot see that a declaration > > immediately followed by an assignment must always initialize the variable ??? > > > Hi Peter, > Glib requires that all g_auto* macros must be initialized. > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree Yes, and we initialize it with the "full_name = ..." line. The g_autofree documentation says "this macro has similar constraints as g_autoptr()", and the g_autoptr() documentation says "You must initialise the variable in some way — either by use of an initialiser or by ensuring that it is assigned to unconditionally before it goes out of scope." In this case the test code is doing the second of those two things. thanks -- PMM
> -----Original Message----- > From: Peter Maydell [mailto:peter.maydell@linaro.org] > Sent: Thursday, November 19, 2020 4:39 PM > To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> > Cc: QEMU Developers <qemu-devel@nongnu.org>; QEMU Trivial > <qemu-trivial@nongnu.org>; Hao Wu <wuhaotsh@google.com>; Havard > Skinnemoen <hskinnemoen@google.com>; Zhanghailiang > <zhang.zhanghailiang@huawei.com>; Thomas Huth <thuth@redhat.com>; > Laurent Vivier <lvivier@redhat.com>; Euler Robot <euler.robot@huawei.com> > Subject: Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree > need to be initialized > > On Thu, 19 Nov 2020 at 08:35, Chenqun (kuhn) <kuhn.chenqun@huawei.com> > wrote: > > > > > -----Original Message----- > > > > static void tim_add_test(const char *name, const TestData *td, > > > > GTestDataFunc fn) { > > > > - g_autofree char *full_name; > > > > - > > > > - full_name = > g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s", > > > > - tim_index(td->tim), > > > timer_index(td->timer), > > > > - name); > > > > + g_autofree char *full_name = g_strdup_printf( > > > > + "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim), > > > > + timer_index(td->timer), name); > > > > > > Which compiler is so unintelligent that it cannot see that a > > > declaration immediately followed by an assignment must always initialize > the variable ??? > > > > > Hi Peter, > > Glib requires that all g_auto* macros must be initialized. > > > > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html > > #g-autofree > > Yes, and we initialize it with the "full_name = ..." line. > The g_autofree documentation says "this macro has similar constraints as > g_autoptr()", and the g_autoptr() documentation says "You must initialise the > variable in some way — either by use of an initialiser or by ensuring that it is > assigned to unconditionally before it goes out of scope." > > In this case the test code is doing the second of those two things. Emm, maybe I didn't get it right. I've tried something as following: There are three pieces of code complied in GCC9.3 and GCC7.3. Code1： g_autofree char *full_name; full_name = g_strdup_printf("npcm7xx_timer"); Code2: g_autofree char *full_name = g_strdup_printf("npcm7xx_timer"); Code3： g_autofree char *full_name; full_name = NULL; The result is as follows: Code1: An warnig is generated for GCC7.3 or GCC9.3. Code2 and Code3: no any warnig whether compiler is GCC7.3 or GCC9.3. I cannot explain why the Code1 warning is generated. But it always generates warning on the GCC compiler. Thanks, Chen Qun
> > On Thu, 19 Nov 2020 at 08:35, Chenqun (kuhn) > <kuhn.chenqun@huawei.com> > > wrote: > > > > > > > -----Original Message----- > > > > > static void tim_add_test(const char *name, const TestData *td, > > > > > GTestDataFunc fn) { > > > > > - g_autofree char *full_name; > > > > > - > > > > > - full_name = > > g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s", > > > > > - tim_index(td->tim), > > > > timer_index(td->timer), > > > > > - name); > > > > > + g_autofree char *full_name = g_strdup_printf( > > > > > + "npcm7xx_timer/tim[%d]/timer[%d]/%s", > tim_index(td->tim), > > > > > + timer_index(td->timer), name); > > > > > > > > Which compiler is so unintelligent that it cannot see that a > > > > declaration immediately followed by an assignment must always > > > > initialize > > the variable ??? > > > > > > > Hi Peter, > > > Glib requires that all g_auto* macros must be initialized. > > > > > > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.ht > > > ml > > > #g-autofree > > > > Yes, and we initialize it with the "full_name = ..." line. > > The g_autofree documentation says "this macro has similar constraints > > as g_autoptr()", and the g_autoptr() documentation says "You must > > initialise the variable in some way — either by use of an initialiser > > or by ensuring that it is assigned to unconditionally before it goes out of > scope." > > > > In this case the test code is doing the second of those two things. > > Emm, maybe I didn't get it right. I've tried something as following: > There are three pieces of code complied in GCC9.3 and GCC7.3. > Code1： > g_autofree char *full_name; > full_name = g_strdup_printf("npcm7xx_timer"); I guess the GCC thinks that g_strdup_printf() may fail to be executed. After the function fails to be executed, and not give a NULL return value. If this occurs, g_autofree will still executes g_free(*pp). So, an warning is generated in the Code1 case. In the example code provided by g-autoptr() documentation, the value assignment statement of the 'dirname' variable uses g_variant_lookup_value(). If the g_variant_lookup_value() function fails to be executed, a NULL value is returned. So, this example code is safe. https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autoptr > > Code2: > g_autofree char *full_name = g_strdup_printf("npcm7xx_timer"); In this case, if g_strdup_printf() fails, the variable definition also fails. So, the Code2 is safe. > > Code3： > g_autofree char *full_name; > full_name = NULL; > > The result is as follows: > Code1: An warnig is generated for GCC7.3 or GCC9.3. > > Code2 and Code3: no any warnig whether compiler is GCC7.3 or GCC9.3. > > I cannot explain why the Code1 warning is generated. But it always generates > warning on the GCC compiler. The above analysis is just my own guess. That may not be the truth. Thanks, Chen Qun
diff --git a/tests/qtest/npcm7xx_timer-test.c b/tests/qtest/npcm7xx_timer-test.c index f08b0cd62a..83774a5b90 100644 --- a/tests/qtest/npcm7xx_timer-test.c +++ b/tests/qtest/npcm7xx_timer-test.c @@ -512,11 +512,9 @@ static void test_disable_on_expiration(gconstpointer test_data) */ static void tim_add_test(const char *name, const TestData *td, GTestDataFunc fn) { - g_autofree char *full_name; - - full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s", - tim_index(td->tim), timer_index(td->timer), - name); + g_autofree char *full_name = g_strdup_printf( + "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim), + timer_index(td->timer), name); qtest_add_data_func(full_name, td, fn); }
According to the glib function requirements, we need initialise the variable. Otherwise there will be compilation warnings: glib-autocleanups.h:28:3: warning: ‘full_name’ may be used uninitialized in this function [-Wmaybe-uninitialized] 28 | g_free (*pp); | ^~~~~~~~~~~~ Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> --- tests/qtest/npcm7xx_timer-test.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)