[PATCH 0/1] Dead stores in maple-tree

[PATCH 0/1] Dead stores in maple-tree

[Lukas Bulwahn]

Dear maple-tree authors, dear Liam, dear Matthew,

there are some Dead Stores that clang-analyzer reports:

lib/maple_tree.c:2906:2: warning: Value stored to 'last' is never read [clang-analyzer-deadcode.DeadStores]
lib/maple_tree.c:2907:2: warning: Value stored to 'prev_min' is never read [clang-analyzer-deadcode.DeadStores]

I addressed these two cases, which were most obvious and clear to fix;
see patch of this one-element series.

Further, clang-analyzer reports more, which I did not address:

lib/maple_tree.c:332:2: warning: Value stored to 'node' is never read [clang-analyzer-deadcode.DeadStores]
lib/maple_tree.c:337:2: warning: Value stored to 'node' is never read [clang-analyzer-deadcode.DeadStores]

Unclear to me if the tool is wrong or right in its analysis here for the two functions above.

lib/maple_tree.c:1212:23: warning: Value stored to 'nodep' during its initialization is never read [clang-analyzer-deadcode.DeadStores]

A lot of pointer magic. Unclear to me if the tool is wrong or right in its analysis here.

lib/maple_tree.c:5014:5: warning: Value stored to 'count' is never read [clang-analyzer-deadcode.DeadStores]

Unclear if the code is intended as it is now.

In mas_anode_descend(), the variable count is really just assigned and used once
effectively. The second assignment is never read. So, the variable count could
just be removed in mas_anode_descend().


Maybe these further warnings are helpful to clean up the code or find an issue
that was overlooked so far.


Best regards,

Lukas


Lukas Bulwahn (1):
  lib: maple_tree: remove unneeded initialization in mtree_range_walk()

 lib/maple_tree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/1] lib: maple_tree: remove unneeded initialization in mtree_range_walk()

[Lukas Bulwahn]

Before the do-while loop in mtree_range_walk(), the variables next, min,
max need to be initialized. The variables last, prev_min and prev_max are
set within the loop body before they are eventually used after exiting the
loop body.

As it is a do-while loop, the loop body is executed at least once, so the
variables last, prev_min and prev_max do not need to be initialized before
the loop body.

Remove unneeded initialization of last and prev_min.

The needless initialization was reported by clang-analyzer as Dead Stores.

As the compiler already identifies these assignments as unneeded, it
optimizes the assignments away. Hence:

No functional change. No change in object code.

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

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index e1743803c851..fbde494444b8 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -2903,8 +2903,8 @@ static inline void *mtree_range_walk(struct ma_state *mas)
 	unsigned long max, min;
 	unsigned long prev_max, prev_min;
 
-	last = next = mas->node;
-	prev_min = min = mas->min;
+	next = mas->node;
+	min = mas->min;
 	max = mas->max;
 	do {
 		offset = 0;
-- 
2.17.1

Re: [PATCH 1/1] lib: maple_tree: remove unneeded initialization in mtree_range_walk()

[Liam Howlett]

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

* Lukas Bulwahn <lukas.bulwahn@gmail.com> [221026 08:01]:
> Before the do-while loop in mtree_range_walk(), the variables next, min,
> max need to be initialized. The variables last, prev_min and prev_max are
> set within the loop body before they are eventually used after exiting the
> loop body.
> 
> As it is a do-while loop, the loop body is executed at least once, so the
> variables last, prev_min and prev_max do not need to be initialized before
> the loop body.
> 
> Remove unneeded initialization of last and prev_min.
> 
> The needless initialization was reported by clang-analyzer as Dead Stores.
> 
> As the compiler already identifies these assignments as unneeded, it
> optimizes the assignments away. Hence:
> 
> No functional change. No change in object code.
> 
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---
>  lib/maple_tree.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> index e1743803c851..fbde494444b8 100644
> --- a/lib/maple_tree.c
> +++ b/lib/maple_tree.c
> @@ -2903,8 +2903,8 @@ static inline void *mtree_range_walk(struct ma_state *mas)
>  	unsigned long max, min;
>  	unsigned long prev_max, prev_min;
>  
> -	last = next = mas->node;
> -	prev_min = min = mas->min;
> +	next = mas->node;
> +	min = mas->min;
>  	max = mas->max;
>  	do {
>  		offset = 0;
> -- 
> 2.17.1
> 

Re: [PATCH 0/1] Dead stores in maple-tree

[Liam Howlett]

* Lukas Bulwahn <lukas.bulwahn@gmail.com> [221026 08:01]:
> Dear maple-tree authors, dear Liam, dear Matthew,
> 
> there are some Dead Stores that clang-analyzer reports:
> 
> lib/maple_tree.c:2906:2: warning: Value stored to 'last' is never read [clang-analyzer-deadcode.DeadStores]
> lib/maple_tree.c:2907:2: warning: Value stored to 'prev_min' is never read [clang-analyzer-deadcode.DeadStores]
> 
> I addressed these two cases, which were most obvious and clear to fix;
> see patch of this one-element series.
> 
> Further, clang-analyzer reports more, which I did not address:
> 
> lib/maple_tree.c:332:2: warning: Value stored to 'node' is never read [clang-analyzer-deadcode.DeadStores]
> lib/maple_tree.c:337:2: warning: Value stored to 'node' is never read [clang-analyzer-deadcode.DeadStores]
> 
> Unclear to me if the tool is wrong or right in its analysis here for the two functions above.

The tool is correct but these aren't going anywhere.  They are compiled
out and are needed for the future.

> 
> lib/maple_tree.c:1212:23: warning: Value stored to 'nodep' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
> 
> A lot of pointer magic. Unclear to me if the tool is wrong or right in its analysis here.

Agreed, this is unclear.. I don't like it and it needs to be removed.
I'll send something out shortly. This was refactoring by the looks of it.

> 
> lib/maple_tree.c:5014:5: warning: Value stored to 'count' is never read [clang-analyzer-deadcode.DeadStores]
> 
> Unclear if the code is intended as it is now.
> 
> In mas_anode_descend(), the variable count is really just assigned and used once
> effectively. The second assignment is never read. So, the variable count could
> just be removed in mas_anode_descend().

This was probably left over from refactoring as well.  I will fix this
as well, thanks.

> 
> 
> Maybe these further warnings are helpful to clean up the code or find an issue
> that was overlooked so far.

Much appreciated,
Liam

Re: [PATCH 0/1] Dead stores in maple-tree

[Dan Carpenter]

On Wed, Oct 26, 2022 at 02:23:19PM +0000, Liam Howlett wrote:
> * Lukas Bulwahn <lukas.bulwahn@gmail.com> [221026 08:01]:
> > Dear maple-tree authors, dear Liam, dear Matthew,
> > 
> > there are some Dead Stores that clang-analyzer reports:
> > 
> > lib/maple_tree.c:2906:2: warning: Value stored to 'last' is never read [clang-analyzer-deadcode.DeadStores]
> > lib/maple_tree.c:2907:2: warning: Value stored to 'prev_min' is never read [clang-analyzer-deadcode.DeadStores]
> > 
> > I addressed these two cases, which were most obvious and clear to fix;
> > see patch of this one-element series.
> > 
> > Further, clang-analyzer reports more, which I did not address:
> > 
> > lib/maple_tree.c:332:2: warning: Value stored to 'node' is never read [clang-analyzer-deadcode.DeadStores]
> > lib/maple_tree.c:337:2: warning: Value stored to 'node' is never read [clang-analyzer-deadcode.DeadStores]
> > 
> > Unclear to me if the tool is wrong or right in its analysis here for the two functions above.
> 
> The tool is correct but these aren't going anywhere.  They are compiled
> out and are needed for the future.
> 

lib/maple_tree.c
   330  static inline void mte_set_full(const struct maple_enode *node)
   331  {
   332          node = (void *)((unsigned long)node & ~MAPLE_ENODE_NULL);
   333  }
   334  
   335  static inline void mte_clear_full(const struct maple_enode *node)
   336  {
   337          node = (void *)((unsigned long)node | MAPLE_ENODE_NULL);
   338  }

That code is really puzzling...  How far into the future before it starts
making sense?

regards,
dan carpenter

Re: [PATCH 0/1] Dead stores in maple-tree

[Liam Howlett]

* Dan Carpenter <dan.carpenter@oracle.com> [221027 03:43]:
> On Wed, Oct 26, 2022 at 02:23:19PM +0000, Liam Howlett wrote:
> > * Lukas Bulwahn <lukas.bulwahn@gmail.com> [221026 08:01]:
> > > Dear maple-tree authors, dear Liam, dear Matthew,
> > > 
> > > there are some Dead Stores that clang-analyzer reports:
> > > 
> > > lib/maple_tree.c:2906:2: warning: Value stored to 'last' is never read [clang-analyzer-deadcode.DeadStores]
> > > lib/maple_tree.c:2907:2: warning: Value stored to 'prev_min' is never read [clang-analyzer-deadcode.DeadStores]
> > > 
> > > I addressed these two cases, which were most obvious and clear to fix;
> > > see patch of this one-element series.
> > > 
> > > Further, clang-analyzer reports more, which I did not address:
> > > 
> > > lib/maple_tree.c:332:2: warning: Value stored to 'node' is never read [clang-analyzer-deadcode.DeadStores]
> > > lib/maple_tree.c:337:2: warning: Value stored to 'node' is never read [clang-analyzer-deadcode.DeadStores]
> > > 
> > > Unclear to me if the tool is wrong or right in its analysis here for the two functions above.
> > 
> > The tool is correct but these aren't going anywhere.  They are compiled
> > out and are needed for the future.
> > 
> 
> lib/maple_tree.c

~line 302:
/* Bit 1 indicates the root is a node */
#define MAPLE_ROOT_NODE                 0x02
/* maple_type stored bit 3-6 */
#define MAPLE_ENODE_TYPE_SHIFT          0x03
/* Bit 2 means a NULL somewhere below */
#define MAPLE_ENODE_NULL                0x04


>    330  static inline void mte_set_full(const struct maple_enode *node)
>    331  {
>    332          node = (void *)((unsigned long)node & ~MAPLE_ENODE_NULL);
>    333  }
>    334  
>    335  static inline void mte_clear_full(const struct maple_enode *node)
>    336  {
>    337          node = (void *)((unsigned long)node | MAPLE_ENODE_NULL);
>    338  }

Looking at the code.... the analysis is correct and these need to be
fixed.  Thanks Dan & Lukas.

> 
> That code is really puzzling...  How far into the future before it starts
> making sense?

If you want to know details like this, you can look at the comments in
the header and c file - that's where the development information
resides.  Information about a node is encoded in the last bits of that
nodes pointer - since they are aligned we can use a mask to restore the
pointer.  Internally I refer to nodes with encoded information as
maple_enodes.  This part is to do with finding out if there is a free
index within the range the node holds.  Think about searching for the
next available index for a unique identifier.

Thanks,
Liam