| Message ID | 20210502174836.838816-2-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc-bios/s390-ccw: Allow building with Clang, too | expand |
On 5/2/21 7:48 PM, Thomas Huth wrote: > When compiling the s390-ccw bios with Clang, the compiler emits a warning: > > pc-bios/s390-ccw/main.c:210:5: warning: variable 'found' is used uninitialized > whenever switch default is taken [-Wsometimes-uninitialized] > default: > ^~~~~~~ > pc-bios/s390-ccw/main.c:214:16: note: uninitialized use occurs here > IPL_assert(found, "Boot device not found\n"); > ^~~~~ > > It's a false positive, it only happens because Clang is not smart enough > to see that the panic() function in the "default:" case can never return. > > Anyway, let's explicitely mark panic() with "noreturn" to shut up the > warning. Why not simply initialize the variable instead? > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > pc-bios/s390-ccw/s390-ccw.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h > index 6cd92669e9..79db69ff54 100644 > --- a/pc-bios/s390-ccw/s390-ccw.h > +++ b/pc-bios/s390-ccw/s390-ccw.h > @@ -89,6 +89,7 @@ bool menu_is_enabled_enum(void); > > #define MAX_BOOT_ENTRIES 31 > > +__attribute__ ((__noreturn__)) > static inline void panic(const char *string) > { > sclp_print(string); >
On 02/05/2021 20.57, Philippe Mathieu-Daudé wrote: > On 5/2/21 7:48 PM, Thomas Huth wrote: >> When compiling the s390-ccw bios with Clang, the compiler emits a warning: >> >> pc-bios/s390-ccw/main.c:210:5: warning: variable 'found' is used uninitialized >> whenever switch default is taken [-Wsometimes-uninitialized] >> default: >> ^~~~~~~ >> pc-bios/s390-ccw/main.c:214:16: note: uninitialized use occurs here >> IPL_assert(found, "Boot device not found\n"); >> ^~~~~ >> >> It's a false positive, it only happens because Clang is not smart enough >> to see that the panic() function in the "default:" case can never return. >> >> Anyway, let's explicitely mark panic() with "noreturn" to shut up the >> warning. > > Why not simply initialize the variable instead? First, it's a false positive. If you only look at the code, someone might later wonder whether it's really necessary or not and try to remove it again (and since there is no warning with gcc, this patch would also have a good chance to get merged, bringing us back to where we are right now). Second, declaring panic() as noreturn is certainly more sustainable, since it might prevent similar situations in other parts of the code in the future. Thomas >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> pc-bios/s390-ccw/s390-ccw.h | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h >> index 6cd92669e9..79db69ff54 100644 >> --- a/pc-bios/s390-ccw/s390-ccw.h >> +++ b/pc-bios/s390-ccw/s390-ccw.h >> @@ -89,6 +89,7 @@ bool menu_is_enabled_enum(void); >> >> #define MAX_BOOT_ENTRIES 31 >> >> +__attribute__ ((__noreturn__)) >> static inline void panic(const char *string) >> { >> sclp_print(string); >> >
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 5/2/21 7:48 PM, Thomas Huth wrote: >> When compiling the s390-ccw bios with Clang, the compiler emits a warning: >> >> pc-bios/s390-ccw/main.c:210:5: warning: variable 'found' is used uninitialized >> whenever switch default is taken [-Wsometimes-uninitialized] >> default: >> ^~~~~~~ >> pc-bios/s390-ccw/main.c:214:16: note: uninitialized use occurs here >> IPL_assert(found, "Boot device not found\n"); >> ^~~~~ >> >> It's a false positive, it only happens because Clang is not smart enough >> to see that the panic() function in the "default:" case can never return. >> >> Anyway, let's explicitely mark panic() with "noreturn" to shut up the >> warning. > > Why not simply initialize the variable instead? Because telling an optimizing compiler the truth is a good idea?
On 5/3/21 6:53 AM, Thomas Huth wrote: > On 02/05/2021 20.57, Philippe Mathieu-Daudé wrote: >> On 5/2/21 7:48 PM, Thomas Huth wrote: >>> When compiling the s390-ccw bios with Clang, the compiler emits a >>> warning: >>> >>> pc-bios/s390-ccw/main.c:210:5: warning: variable 'found' is used >>> uninitialized >>> whenever switch default is taken [-Wsometimes-uninitialized] >>> default: >>> ^~~~~~~ >>> pc-bios/s390-ccw/main.c:214:16: note: uninitialized use occurs here >>> IPL_assert(found, "Boot device not found\n"); >>> ^~~~~ >>> >>> It's a false positive, it only happens because Clang is not smart enough >>> to see that the panic() function in the "default:" case can never >>> return. >>> >>> Anyway, let's explicitely mark panic() with "noreturn" to shut up the >>> warning. >> >> Why not simply initialize the variable instead? > > First, it's a false positive. If you only look at the code, someone > might later wonder whether it's really necessary or not and try to > remove it again (and since there is no warning with gcc, this patch > would also have a good chance to get merged, bringing us back to where > we are right now). > > Second, declaring panic() as noreturn is certainly more sustainable, > since it might prevent similar situations in other parts of the code in > the future. Fine then: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Sun, 2 May 2021 19:48:33 +0200 Thomas Huth <thuth@redhat.com> wrote: > When compiling the s390-ccw bios with Clang, the compiler emits a warning: > > pc-bios/s390-ccw/main.c:210:5: warning: variable 'found' is used uninitialized > whenever switch default is taken [-Wsometimes-uninitialized] > default: > ^~~~~~~ > pc-bios/s390-ccw/main.c:214:16: note: uninitialized use occurs here > IPL_assert(found, "Boot device not found\n"); > ^~~~~ > > It's a false positive, it only happens because Clang is not smart enough > to see that the panic() function in the "default:" case can never return. > > Anyway, let's explicitely mark panic() with "noreturn" to shut up the > warning. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > pc-bios/s390-ccw/s390-ccw.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h > index 6cd92669e9..79db69ff54 100644 > --- a/pc-bios/s390-ccw/s390-ccw.h > +++ b/pc-bios/s390-ccw/s390-ccw.h > @@ -89,6 +89,7 @@ bool menu_is_enabled_enum(void); > > #define MAX_BOOT_ENTRIES 31 > > +__attribute__ ((__noreturn__)) > static inline void panic(const char *string) > { > sclp_print(string); I'm surprised that the noreturn annotation of disabled_wait (called right after the sclp_print()) is not enough. Anyway, Reviewed-by: Cornelia Huck <cohuck@redhat.com>
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h index 6cd92669e9..79db69ff54 100644 --- a/pc-bios/s390-ccw/s390-ccw.h +++ b/pc-bios/s390-ccw/s390-ccw.h @@ -89,6 +89,7 @@ bool menu_is_enabled_enum(void); #define MAX_BOOT_ENTRIES 31 +__attribute__ ((__noreturn__)) static inline void panic(const char *string) { sclp_print(string);
When compiling the s390-ccw bios with Clang, the compiler emits a warning: pc-bios/s390-ccw/main.c:210:5: warning: variable 'found' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized] default: ^~~~~~~ pc-bios/s390-ccw/main.c:214:16: note: uninitialized use occurs here IPL_assert(found, "Boot device not found\n"); ^~~~~ It's a false positive, it only happens because Clang is not smart enough to see that the panic() function in the "default:" case can never return. Anyway, let's explicitely mark panic() with "noreturn" to shut up the warning. Signed-off-by: Thomas Huth <thuth@redhat.com> --- pc-bios/s390-ccw/s390-ccw.h | 1 + 1 file changed, 1 insertion(+)