-
Notifications
You must be signed in to change notification settings - Fork 440
Fix #28008: Handle unmanaged classes in c_addrOf with wide pointers #28043
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: main
Are you sure you want to change the base?
Fix #28008: Handle unmanaged classes in c_addrOf with wide pointers #28043
Conversation
… pointers When c_addrOf is called on an unmanaged class in a wide pointer context (e.g., with CHPL_COMM=gasnet or --no-local), the current implementation incorrectly tries to take the address of the variable itself using c_pointer_return, which treats it as a wide pointer. However, unmanaged class variables are stored as narrow pointers even in wide pointer contexts. This causes: - Segfaults with CHPL_COMM=gasnet - Assertion failures with --no-local and assertions enabled Solution: Add specialized overloads of c_addrOf and c_addrOfConst for unmanaged classes that use c_ptrTo/c_ptrToConst instead of c_pointer_return. This correctly handles the narrow pointer value rather than taking the address of the variable. The fix adds: - c_addrOf overload for isUnmanagedClass(t) returning c_ptr(void) - c_addrOfConst overload for isUnmanagedClass(t) returning c_ptrConst(void) Signed-off-by: Samaresh Kumar Singh <[email protected]>
- Add c_addrOf_unmanaged_class.chpl test that verifies the fix works - Update c_ptr_class_type.good to reflect that c_addrOf(unmanaged) now equals c_ptrTo(unmanaged), changing line from false to true Signed-off-by: Samaresh Kumar Singh <[email protected]>
be3ec76 to
328e511
Compare
|
Note that
|
|
Can someone take a look into this PR please? |
Thank you for the clarification! You're absolutely right that c_addrOf and c_ptrTo should have different semantics. After reviewing the documentation more carefully, I now understand:
I'll research the Chapel compiler internals to understand the proper way to take the address of a narrow pointer variable in a wide pointer context. |
|
The real issue is that c_pointer_return (electron-browser/workbench/workbench.html) expects a wide pointer structure, but unmanaged class variables are stored as narrow pointers. For c_addrOf to return the variable's address (not the heap instance), should I use __primitive("_wide_get_addr", x) or is there a better primitive for handling narrow pointers in wide pointer contexts? |
|
@jabraham17 @mstrout @sbillig Please take a look into this PR. I am happy to help and contribute to this repo! |
|
@SamareshSingh I am happy to look at this PR and work with you to get it developed and merged, but I am not willing to provide prompts to an AI for you. |
Problem
Issue #28008 reports that
c_addrOfon an unmanaged class does not work properly with wide pointers (when usingCHPL_COMM=gasnetor--no-local). This causes:CHPL_COMM=gasnetAssertion failed: (node==0), function chpl_comm_get, file comm-none.c, line 280with--no-localand assertions enabledRoot Cause
In a wide pointer context, unmanaged class variables are stored as narrow pointers, but
c_addrOfusesc_pointer_returnwhich tries to take the address of the variable itself and incorrectly treats it as a wide pointer.From the generated C code in the issue:
The variable is a regular C pointer, but
c_pointer_returntreats it as if it points to a wide pointer structure, causing memory corruption.Solution
Add specialized overloads of
c_addrOfandc_addrOfConstfor unmanaged classes that usec_ptrTo/c_ptrToConstinstead ofc_pointer_return. This correctly extracts the pointer value using__primitive("cast", c_ptr(void), c.borrow())rather than taking the address of the variable.Changes to
modules/standard/CTypes.chpl:inline proc c_addrOf(const x: ?t): c_ptr(void) where isUnmanagedClass(t)inline proc c_addrOfConst(const x: ?t): c_ptrConst(void) where isUnmanagedClass(t)Both delegate to
c_ptrTo/c_ptrToConstwhich handle class pointers correctly.Testing
New Test
Added
test/library/standard/CTypes/c_addrOf_unmanaged_class.chplthat tests the exact scenario from issue #28008:Expected output:
Updated Existing Test
test/types/cptr/c_ptr_class_type.chplline 47 already tested:This previously output
false(indicating the bug), but now correctly outputstrue. Updated.goodfile accordingly.Validation
The fix:
c_addrOfbehavior consistent withc_ptrTofor unmanaged classesc_addrOfon an unmanaged class does not work properly with wide pointers #28008Fixes #28008