| Message ID | 20221026120029.12555-1-lukas.bulwahn@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Dead stores in maple-tree | expand |
* 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
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
* 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