-
Notifications
You must be signed in to change notification settings - Fork 51
Fix: strict-aliasing violations in treeshr #2983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: alpha
Are you sure you want to change the base?
Conversation
241a3d8 to
288c303
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors pointer casting operations by replacing unsafe pointer casts with safer alternatives. The changes introduce helper functions for type conversions and improve code maintainability by avoiding undefined behavior from type-punning through pointer casts.
Key changes:
- Replaced macro-based RFA conversion with inline functions that perform byte-by-byte manipulation
- Added helper functions
nid_to_int()andint_to_nid()for safe NID/int conversions - Updated all call sites to use the new helper functions and
memcpyinstead of pointer casts
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| treeshr/treeshrp.h | Introduces inline functions for RFA-to-seek conversions and NID/int type conversions, replacing unsafe pointer cast macros |
| treeshr/TreeRenameNode.c | Updates to use nid_to_int() helper instead of pointer casting |
| treeshr/TreeOpen.c | Updates to use nid_to_int() helper; removes volatile qualifier from brother_nid |
| treeshr/TreeGetRecord.c | Updates to use int_to_nid() helper instead of pointer casting |
| treeshr/TreeGetNci.c | Replaces pointer casts with memcpy and helper functions for safer type conversions |
| treeshr/TreeDeleteNode.c | Updates to use nid_to_int() helper instead of pointer casting |
| treeshr/TreeAddNode.c | Simplifies code by removing unnecessary pointer cast (variable is already an int) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
These changes look OK to me. (It will be good when we have all the other "strict aliasing violations" fixed too.) |
| static void SubtreeNodeConnect(PINO_DATABASE *dblist, NODE *parent, | ||
| NODE *subtreetop) | ||
| { | ||
| NID child_nid, parent_nid = {0, 0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while we're making changes here, should we move each of these on their own line?
|
This PR is presently the result of a "merge" and not a "rebase". The most recent commit has two parents. |
Replace pointer-int casts with memcpy Fixes #2982 ** Note there are strict aliasing violations in other parts of the codebase too.** treeshrp.h - add int_to_nid function - add nid_to_int function - fix RFA macros to be aliasing safe TreeAddNode.c - remove unnessary cast TreeDeleteNode.c - use nid_to_int function TreeGetNci.c - use int_to_nid and nid_to_int functions - fix other strict-aliasing violations TreeGetRecord.c - use int_to_nid function TreeOpen.c - remove volatile qualifier - use nid_to_int function TreeRenameNode.c - use nid_to_int functions
remove bogus RFA macros in treeshrp.h
6e8fb5d to
fc7ce43
Compare
Replace pointer-int casts with memcpy
Fixes #2982
** Note there are strict aliasing violations in other parts of the codebase too.**
treeshrp.h - add int_to_nid function
- add nid_to_int function
- fix RFA macros to be aliasing safe
TreeAddNode.c - remove unnessary cast
TreeDeleteNode.c - use nid_to_int function
TreeGetNci.c - use int_to_nid and nid_to_int functions
- fix other strict-aliasing violations
TreeGetRecord.c - use int_to_nid function
TreeOpen.c - remove volatile qualifier
- use nid_to_int function
TreeRenameNode.c - use nid_to_int functions