From bed46edb99eece3473f478341919c6284f98cf43 Mon Sep 17 00:00:00 2001 From: Anna Rift Date: Wed, 30 Oct 2024 10:19:55 -0700 Subject: [PATCH 1/4] Replace some c_ptrTo array workarounds with c_addrOf Signed-off-by: Anna Rift --- modules/internal/ChapelArray.chpl | 4 ++-- modules/internal/ChapelHashtable.chpl | 16 +++------------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/modules/internal/ChapelArray.chpl b/modules/internal/ChapelArray.chpl index 531605f93538..03251c66b35e 100644 --- a/modules/internal/ChapelArray.chpl +++ b/modules/internal/ChapelArray.chpl @@ -3693,7 +3693,7 @@ module ChapelArray { chpl_arrayToPtrErrorHelper(arr); use CTypes; - const ptr = c_pointer_return(arr[arr.domain.low]); + const ptr = c_addrOf(arr[arr.domain.low]); if castToVoidStar then return ptr: c_ptr(void); else @@ -3703,7 +3703,7 @@ module ChapelArray { chpl_arrayToPtrErrorHelper(arr); use CTypes; - const ptr = c_pointer_return_const(arr[arr.domain.low]); + const ptr = c_addrOfConst(arr[arr.domain.low]); if castToVoidStar then return ptr: c_ptrConst(void); else diff --git a/modules/internal/ChapelHashtable.chpl b/modules/internal/ChapelHashtable.chpl index 9f029f1c1cda..62b8822a85d7 100644 --- a/modules/internal/ChapelHashtable.chpl +++ b/modules/internal/ChapelHashtable.chpl @@ -62,29 +62,19 @@ module ChapelHashtable { const sizeofElement = _ddata_sizeof_element(ret); - // The memset call below needs to be able to set _array records. - // But c_ptrTo on an _array will return a pointer to - // the first element, which messes up the shallowCopy/shallowSwap code - // - // As a workaround, this function just returns a pointer to the argument, - // whether or not it is an array. - inline proc ptrTo(ref x) { - return c_pointer_return(x); - } - select initMethod { when ArrayInit.noInit { // do nothing } when ArrayInit.serialInit { for slot in _allSlots(size) { - memset(ptrTo(ret[slot]), 0:uint(8), sizeofElement.safeCast(c_size_t)); + memset(c_addrOf(ret[slot]), 0:uint(8), sizeofElement.safeCast(c_size_t)); } } when ArrayInit.parallelInit { // This should match the 'these' iterator in terms of idx->task forall slot in _allSlots(size) { - memset(ptrTo(ret[slot]), 0:uint(8), sizeofElement.safeCast(c_size_t)); + memset(c_addrOf(ret[slot]), 0:uint(8), sizeofElement.safeCast(c_size_t)); } } when ArrayInit.gpuInit { @@ -92,7 +82,7 @@ module ChapelHashtable { if CHPL_LOCALE_MODEL=="gpu" { extern proc chpl_gpu_memset(addr, byte, numBytes); foreach slot in _allSlots(size) { - chpl_gpu_memset(ptrTo(ret[slot]), 0:uint(8), sizeofElement); + chpl_gpu_memset(c_addrOf(ret[slot]), 0:uint(8), sizeofElement); } } else { From f390c61cef3c9c200cd7103870d2fe7b55b65bc0 Mon Sep 17 00:00:00 2001 From: Anna Rift Date: Thu, 31 Oct 2024 08:47:39 -0700 Subject: [PATCH 2/4] Remove c_ptrTo workarounds in Sort Signed-off-by: Anna Rift --- modules/standard/Sort.chpl | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl index cdb4fd1da3b3..bc66630022c4 100644 --- a/modules/standard/Sort.chpl +++ b/modules/standard/Sort.chpl @@ -1859,21 +1859,6 @@ module ShallowCopy { private use CTypes; private use OS.POSIX; - // The shallowCopy / shallowSwap code needs to be able to copy/swap - // _array records. But c_ptrTo on an _array will return a pointer to - // the first element, which messes up the shallowCopy/shallowSwap code - // - // As a workaround, this function just returns a pointer to the argument, - // whether or not it is an array. - // - // TODO: these should be replaced with the appropriate c_addrOf[Const] calls - private inline proc ptrTo(ref x) { - return c_pointer_return(x); - } - private inline proc ptrToConst(const ref x) { - return c_pointer_return_const(x); - } - // These shallow copy functions "move" a record around // (i.e. they neither swap nor call a copy initializer). // @@ -1886,11 +1871,11 @@ module ShallowCopy { dst = src; } else { var size = c_sizeof(st); - memcpy(ptrTo(dst), ptrTo(src), size); + memcpy(c_addrOf(dst), c_addrOf(src), size); if boundsChecking { // The version moved from should never be used again, // but we clear it out just in case. - memset(ptrTo(src), 0, size); + memset(c_addrOf(src), 0, size); } } } @@ -1920,11 +1905,11 @@ module ShallowCopy { } else { var size = c_sizeof(st); // tmp = lhs - memcpy(ptrTo(tmp), ptrTo(lhs), size); + memcpy(c_addrOf(tmp), c_addrOf(lhs), size); // lhs = rhs - memcpy(ptrTo(lhs), ptrTo(rhs), size); + memcpy(c_addrOf(lhs), c_addrOf(rhs), size); // rhs = tmp - memcpy(ptrTo(rhs), ptrTo(tmp), size); + memcpy(c_addrOf(rhs), c_addrOf(tmp), size); } } @@ -1952,7 +1937,7 @@ module ShallowCopy { if A._instance.isDefaultRectangular() { type st = __primitive("static field type", A._value, "eltType"); var size = (nElts:c_size_t)*c_sizeof(st); - memcpy(ptrTo(A[dst_idx]), ptrTo(A[src_idx]), size); + memcpy(c_addrOf(A[dst_idx]), c_addrOf(A[src_idx]), size); } else { var ok = chpl__bulkTransferArray(/*dst*/ A, {dst_idx..#nElts_idx}, @@ -1994,7 +1979,7 @@ module ShallowCopy { SrcA._instance.isDefaultRectangular() { type st = __primitive("static field type", DstA._value, "eltType"); var size = (nElts:c_size_t)*c_sizeof(st); - memcpy(ptrTo(DstA[dst_idx]), ptrToConst(SrcA[src_idx]), size); + memcpy(c_addrOf(DstA[dst_idx]), c_addrOfConst(SrcA[src_idx]), size); } else { var ok = chpl__bulkTransferArray(/*dst*/ DstA, {dst_idx..#nElts_dst_idx}, From 536fb2feb52a3552556a38933a6bb4652222b58f Mon Sep 17 00:00:00 2001 From: Anna Rift Date: Wed, 29 Jan 2025 11:43:12 -0800 Subject: [PATCH 3/4] Remove c_addrOf[Const] separate array overloads Signed-off-by: Anna Rift --- modules/standard/CTypes.chpl | 47 ++++++++---------------------------- 1 file changed, 10 insertions(+), 37 deletions(-) diff --git a/modules/standard/CTypes.chpl b/modules/standard/CTypes.chpl index 11fd4af9f5a5..e04148684ff5 100644 --- a/modules/standard/CTypes.chpl +++ b/modules/standard/CTypes.chpl @@ -953,43 +953,6 @@ module CTypes { // Begin c_addrOf[Const] definitions - /* - Returns a :type:`c_ptr` to the address of an array. - - This is distinct from :func:`c_ptrTo` in that it returns a pointer to - the array object itself, rather than to the first element of the array's - buffer. - - Note that the existence of this :type:`c_ptr` has no impact on the lifetime - of the array. The returned pointer will be invalid if the array is freed. - */ - @chpldoc.nodoc - inline proc c_addrOf(ref arr: []) { - if (boundsChecking && arr._value.locale != here) then - // Changed from error to unstable warning in 2.4. Warning can be removed - // once we're confident it's not causing problems. - if chpl_warnUnstable then - compilerWarning( - "calling c_addrOf on an array from another locale is unstable"); - - return c_pointer_return(arr); - } - - /* - Like :proc:`c_addrOf` for arrays, but returns a :type:`c_ptrConst` which - disallows direct modification of the pointee. - */ - @chpldoc.nodoc - inline proc c_addrOfConst(const arr: []) { - if (boundsChecking && arr._value.locale != here) then - // See note on corresponding c_addrOf overload - if chpl_warnUnstable then - compilerWarning( - "calling c_addrOfConst on an array from another locale is unstable"); - - return c_pointer_return_const(arr); - } - /* Returns a :type:`c_ptr` to the address of any Chapel object. @@ -1001,6 +964,11 @@ module CTypes { inline proc c_addrOf(ref x: ?t): c_ptr(t) { if isDomainType(t) then compilerError("c_addrOf domain type not supported"); + if (isArrayType(t) && boundsChecking && x._value.locale != here) then + // See note on corresponding c_addrOf overload + if chpl_warnUnstable then + compilerWarning( + "calling c_addrOf on an array from another locale is unstable"); return c_pointer_return(x); } @@ -1019,6 +987,11 @@ module CTypes { inline proc c_addrOfConst(const ref x: ?t): c_ptrConst(t) { if isDomainType(t) then compilerError("c_addrOfConst domain type not supported"); + if (isArrayType(t) && boundsChecking && x._value.locale != here) then + // See note on corresponding c_addrOf overload + if chpl_warnUnstable then + compilerWarning( + "calling c_addrOfConst on an array from another locale is unstable"); return c_pointer_return_const(x); } From 1000b2ed21f11d1e83452e3703ab2861d8f37379 Mon Sep 17 00:00:00 2001 From: Anna Rift Date: Fri, 26 Sep 2025 10:23:19 -0700 Subject: [PATCH 4/4] Revert "Remove c_ptrTo workarounds in Sort" Reverted as it seems these workarounds have been removed from Sort on main in the meantime. This reverts commit f390c61cef3c9c200cd7103870d2fe7b55b65bc0. Signed-off-by: Anna Rift --- modules/standard/Sort.chpl | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl index bc66630022c4..cdb4fd1da3b3 100644 --- a/modules/standard/Sort.chpl +++ b/modules/standard/Sort.chpl @@ -1859,6 +1859,21 @@ module ShallowCopy { private use CTypes; private use OS.POSIX; + // The shallowCopy / shallowSwap code needs to be able to copy/swap + // _array records. But c_ptrTo on an _array will return a pointer to + // the first element, which messes up the shallowCopy/shallowSwap code + // + // As a workaround, this function just returns a pointer to the argument, + // whether or not it is an array. + // + // TODO: these should be replaced with the appropriate c_addrOf[Const] calls + private inline proc ptrTo(ref x) { + return c_pointer_return(x); + } + private inline proc ptrToConst(const ref x) { + return c_pointer_return_const(x); + } + // These shallow copy functions "move" a record around // (i.e. they neither swap nor call a copy initializer). // @@ -1871,11 +1886,11 @@ module ShallowCopy { dst = src; } else { var size = c_sizeof(st); - memcpy(c_addrOf(dst), c_addrOf(src), size); + memcpy(ptrTo(dst), ptrTo(src), size); if boundsChecking { // The version moved from should never be used again, // but we clear it out just in case. - memset(c_addrOf(src), 0, size); + memset(ptrTo(src), 0, size); } } } @@ -1905,11 +1920,11 @@ module ShallowCopy { } else { var size = c_sizeof(st); // tmp = lhs - memcpy(c_addrOf(tmp), c_addrOf(lhs), size); + memcpy(ptrTo(tmp), ptrTo(lhs), size); // lhs = rhs - memcpy(c_addrOf(lhs), c_addrOf(rhs), size); + memcpy(ptrTo(lhs), ptrTo(rhs), size); // rhs = tmp - memcpy(c_addrOf(rhs), c_addrOf(tmp), size); + memcpy(ptrTo(rhs), ptrTo(tmp), size); } } @@ -1937,7 +1952,7 @@ module ShallowCopy { if A._instance.isDefaultRectangular() { type st = __primitive("static field type", A._value, "eltType"); var size = (nElts:c_size_t)*c_sizeof(st); - memcpy(c_addrOf(A[dst_idx]), c_addrOf(A[src_idx]), size); + memcpy(ptrTo(A[dst_idx]), ptrTo(A[src_idx]), size); } else { var ok = chpl__bulkTransferArray(/*dst*/ A, {dst_idx..#nElts_idx}, @@ -1979,7 +1994,7 @@ module ShallowCopy { SrcA._instance.isDefaultRectangular() { type st = __primitive("static field type", DstA._value, "eltType"); var size = (nElts:c_size_t)*c_sizeof(st); - memcpy(c_addrOf(DstA[dst_idx]), c_addrOfConst(SrcA[src_idx]), size); + memcpy(ptrTo(DstA[dst_idx]), ptrToConst(SrcA[src_idx]), size); } else { var ok = chpl__bulkTransferArray(/*dst*/ DstA, {dst_idx..#nElts_dst_idx},