linux-safety.lists.elisa.tech archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86/unwind: remove unneeded initialization
@ 2020-10-28 12:21 Lukas Bulwahn
  2020-10-29  2:36 ` Nathan Chancellor
  2020-10-29 11:49 ` Walter Harms
  0 siblings, 2 replies; 6+ messages in thread
From: Lukas Bulwahn @ 2020-10-28 12:21 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Josh Poimboeuf, x86
  Cc: H . Peter Anvin, Peter Zijlstra, Nathan Chancellor,
	Nick Desaulniers, linux-kernel, clang-built-linux,
	kernel-janitors, linux-safety, Lukas Bulwahn

make clang-analyzer on x86_64 defconfig caught my attention with:

  arch/x86/kernel/unwind_orc.c:38:7: warning: Value stored to 'mid' during
  its initialization is never read [clang-analyzer-deadcode.DeadStores]
          int *mid = first, *found = first;
               ^

Commit ee9f8fce9964 ("x86/unwind: Add the ORC unwinder") introduced
__orc_find() with this unneeded dead-store initialization.

Put the variable in local scope and initialize only once the value is
needed to make clang-analyzer happy.

As compilers will detect this unneeded assignment and optimize this
anyway, the resulting object code is effectively identical before and
after this change.

No functional change. Effectively, no change to object code.

Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
applies cleanly on current master and next-20201028

Josh, please ack.
Ingo, Borislav, please pick this minor non-urgent clean-up patch.

 arch/x86/kernel/unwind_orc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index 6a339ce328e0..5c64eed08257 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -35,7 +35,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
 {
 	int *first = ip_table;
 	int *last = ip_table + num_entries - 1;
-	int *mid = first, *found = first;
+	int *found = first;
 
 	if (!num_entries)
 		return NULL;
@@ -47,7 +47,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
 	 * ignored when they conflict with a real entry.
 	 */
 	while (first <= last) {
-		mid = first + ((last - first) / 2);
+		int *mid = first + ((last - first) / 2);
 
 		if (orc_ip(mid) <= ip) {
 			found = mid;
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86/unwind: remove unneeded initialization
  2020-10-28 12:21 [PATCH] x86/unwind: remove unneeded initialization Lukas Bulwahn
@ 2020-10-29  2:36 ` Nathan Chancellor
  2020-10-29 11:49 ` Walter Harms
  1 sibling, 0 replies; 6+ messages in thread
From: Nathan Chancellor @ 2020-10-29  2:36 UTC (permalink / raw)
  To: Lukas Bulwahn
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Josh Poimboeuf,
	x86, H . Peter Anvin, Peter Zijlstra, Nick Desaulniers,
	linux-kernel, clang-built-linux, kernel-janitors, linux-safety

On Wed, Oct 28, 2020 at 01:21:02PM +0100, Lukas Bulwahn wrote:
> make clang-analyzer on x86_64 defconfig caught my attention with:
> 
>   arch/x86/kernel/unwind_orc.c:38:7: warning: Value stored to 'mid' during
>   its initialization is never read [clang-analyzer-deadcode.DeadStores]
>           int *mid = first, *found = first;
>                ^
> 
> Commit ee9f8fce9964 ("x86/unwind: Add the ORC unwinder") introduced
> __orc_find() with this unneeded dead-store initialization.
> 
> Put the variable in local scope and initialize only once the value is
> needed to make clang-analyzer happy.
> 
> As compilers will detect this unneeded assignment and optimize this
> anyway, the resulting object code is effectively identical before and
> after this change.
> 
> No functional change. Effectively, no change to object code.
> 
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>

Seems fine to me.

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

> ---
> applies cleanly on current master and next-20201028
> 
> Josh, please ack.
> Ingo, Borislav, please pick this minor non-urgent clean-up patch.
> 
>  arch/x86/kernel/unwind_orc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index 6a339ce328e0..5c64eed08257 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -35,7 +35,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
>  {
>  	int *first = ip_table;
>  	int *last = ip_table + num_entries - 1;
> -	int *mid = first, *found = first;
> +	int *found = first;
>  
>  	if (!num_entries)
>  		return NULL;
> @@ -47,7 +47,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
>  	 * ignored when they conflict with a real entry.
>  	 */
>  	while (first <= last) {
> -		mid = first + ((last - first) / 2);
> +		int *mid = first + ((last - first) / 2);
>  
>  		if (orc_ip(mid) <= ip) {
>  			found = mid;
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86/unwind: remove unneeded initialization
  2020-10-28 12:21 [PATCH] x86/unwind: remove unneeded initialization Lukas Bulwahn
  2020-10-29  2:36 ` Nathan Chancellor
@ 2020-10-29 11:49 ` Walter Harms
  2020-10-29 12:04   ` Peter Zijlstra
  1 sibling, 1 reply; 6+ messages in thread
From: Walter Harms @ 2020-10-29 11:49 UTC (permalink / raw)
  To: Lukas Bulwahn, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Josh Poimboeuf, x86
  Cc: H . Peter Anvin, Peter Zijlstra, Nathan Chancellor,
	Nick Desaulniers, linux-kernel, clang-built-linux,
	kernel-janitors, linux-safety

this looks like a reimplementation of bsearch()
perhaps the maintainer can add a comment why the 
kernel implementation is not suitable here ?


jm2c
wh

________________________________________
Von: Lukas Bulwahn [lukas.bulwahn@gmail.com]
Gesendet: Mittwoch, 28. Oktober 2020 13:21
An: Thomas Gleixner; Ingo Molnar; Borislav Petkov; Josh Poimboeuf; x86@kernel.org
Cc: H . Peter Anvin; Peter Zijlstra; Nathan Chancellor; Nick Desaulniers; linux-kernel@vger.kernel.org; clang-built-linux@googlegroups.com; kernel-janitors@vger.kernel.org; linux-safety@lists.elisa.tech; Lukas Bulwahn
Betreff: [PATCH] x86/unwind: remove unneeded initialization

make clang-analyzer on x86_64 defconfig caught my attention with:

  arch/x86/kernel/unwind_orc.c:38:7: warning: Value stored to 'mid' during
  its initialization is never read [clang-analyzer-deadcode.DeadStores]
          int *mid = first, *found = first;
               ^

Commit ee9f8fce9964 ("x86/unwind: Add the ORC unwinder") introduced
__orc_find() with this unneeded dead-store initialization.

Put the variable in local scope and initialize only once the value is
needed to make clang-analyzer happy.

As compilers will detect this unneeded assignment and optimize this
anyway, the resulting object code is effectively identical before and
after this change.

No functional change. Effectively, no change to object code.

Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
applies cleanly on current master and next-20201028

Josh, please ack.
Ingo, Borislav, please pick this minor non-urgent clean-up patch.

 arch/x86/kernel/unwind_orc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index 6a339ce328e0..5c64eed08257 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -35,7 +35,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
 {
        int *first = ip_table;
        int *last = ip_table + num_entries - 1;
-       int *mid = first, *found = first;
+       int *found = first;

        if (!num_entries)
                return NULL;
@@ -47,7 +47,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
         * ignored when they conflict with a real entry.
         */
        while (first <= last) {
-               mid = first + ((last - first) / 2);
+               int *mid = first + ((last - first) / 2);

                if (orc_ip(mid) <= ip) {
                        found = mid;
--
2.17.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86/unwind: remove unneeded initialization
  2020-10-29 11:49 ` Walter Harms
@ 2020-10-29 12:04   ` Peter Zijlstra
  0 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2020-10-29 12:04 UTC (permalink / raw)
  To: Walter Harms
  Cc: Lukas Bulwahn, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Josh Poimboeuf, x86, H . Peter Anvin, Nathan Chancellor,
	Nick Desaulniers, linux-kernel, clang-built-linux,
	kernel-janitors, linux-safety

On Thu, Oct 29, 2020 at 11:49:50AM +0000, Walter Harms wrote:
> this looks like a reimplementation of bsearch()
> perhaps the maintainer can add a comment why the 
> kernel implementation is not suitable here ?

If you look carefully it doesn't do an exact match, which is what
bsearch() does.

bsearch() also isn't stable in the precense of duplicates.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86/unwind: remove unneeded initialization
  2020-10-20  7:19 Lukas Bulwahn
@ 2020-10-20  7:22 ` Lukas Bulwahn
  0 siblings, 0 replies; 6+ messages in thread
From: Lukas Bulwahn @ 2020-10-20  7:22 UTC (permalink / raw)
  To: linux-safety

On Tue, Oct 20, 2020 at 9:20 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
> make clang-analyzer on x86_64 defconfig caught my attention with:
>
>   arch/x86/kernel/unwind_orc.c:38:7: warning: Value stored to 'mid' during
>   its initialization is never read [clang-analyzer-deadcode.DeadStores]
>           int *mid = first, *found = first;
>                ^
>
> Commit ee9f8fce9964 ("x86/unwind: Add the ORC unwinder") introduced
> __orc_find() with this unneeded dead-store initialization.
>
> Put the variable in local scope and initialize only once the value is
> needed to make clang-analyzer happy.
>
> As compilers will detect these unneeded assignments and optimize this
> anyway, the resulting object code is effectively identical before and
> after this change.
>
> No functional change. Effectively, no change to object code.
>
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---


Please help me with your review.

Also help by compiling and comparing the disassembly before and after
with your compiler.


>  arch/x86/kernel/unwind_orc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index 6a339ce328e0..5c64eed08257 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -35,7 +35,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
>  {
>         int *first = ip_table;
>         int *last = ip_table + num_entries - 1;
> -       int *mid = first, *found = first;
> +       int *found = first;
>
>         if (!num_entries)
>                 return NULL;
> @@ -47,7 +47,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
>          * ignored when they conflict with a real entry.
>          */
>         while (first <= last) {
> -               mid = first + ((last - first) / 2);
> +               int *mid = first + ((last - first) / 2);
>
>                 if (orc_ip(mid) <= ip) {
>                         found = mid;
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] x86/unwind: remove unneeded initialization
@ 2020-10-20  7:19 Lukas Bulwahn
  2020-10-20  7:22 ` Lukas Bulwahn
  0 siblings, 1 reply; 6+ messages in thread
From: Lukas Bulwahn @ 2020-10-20  7:19 UTC (permalink / raw)
  To: linux-safety; +Cc: Lukas Bulwahn

make clang-analyzer on x86_64 defconfig caught my attention with:

  arch/x86/kernel/unwind_orc.c:38:7: warning: Value stored to 'mid' during
  its initialization is never read [clang-analyzer-deadcode.DeadStores]
          int *mid = first, *found = first;
               ^

Commit ee9f8fce9964 ("x86/unwind: Add the ORC unwinder") introduced
__orc_find() with this unneeded dead-store initialization.

Put the variable in local scope and initialize only once the value is
needed to make clang-analyzer happy.

As compilers will detect these unneeded assignments and optimize this
anyway, the resulting object code is effectively identical before and
after this change.

No functional change. Effectively, no change to object code.

Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
 arch/x86/kernel/unwind_orc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index 6a339ce328e0..5c64eed08257 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -35,7 +35,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
 {
 	int *first = ip_table;
 	int *last = ip_table + num_entries - 1;
-	int *mid = first, *found = first;
+	int *found = first;
 
 	if (!num_entries)
 		return NULL;
@@ -47,7 +47,7 @@ static struct orc_entry *__orc_find(int *ip_table, struct orc_entry *u_table,
 	 * ignored when they conflict with a real entry.
 	 */
 	while (first <= last) {
-		mid = first + ((last - first) / 2);
+		int *mid = first + ((last - first) / 2);
 
 		if (orc_ip(mid) <= ip) {
 			found = mid;
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-29 12:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-28 12:21 [PATCH] x86/unwind: remove unneeded initialization Lukas Bulwahn
2020-10-29  2:36 ` Nathan Chancellor
2020-10-29 11:49 ` Walter Harms
2020-10-29 12:04   ` Peter Zijlstra
  -- strict thread matches above, loose matches on Subject: below --
2020-10-20  7:19 Lukas Bulwahn
2020-10-20  7:22 ` Lukas Bulwahn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).