remove wasted allocation space for JL_ARRAY_IMPL_NUL (#51275)

Handle this more correctly in Cstring conversion instead. Although
apparently this feature and test has not worked in Julia since String
became a first class object (and not wrapping Array).

Refs: cee4cb7

Author: vtjnash

base/c.jl

@@ -199,7 +199,7 @@ cconvert(::Type{Cstring}, s::AbstractString) =
 
 function cconvert(::Type{Cwstring}, s::AbstractString)
     v = transcode(Cwchar_t, String(s))
-    !isempty(v) && v[end] == 0 || push!(v, 0)
+    push!(v, 0)
     return v
 end
 
@@ -211,7 +211,7 @@ containsnul(p::Ptr, len) =
 containsnul(s::String) = containsnul(unsafe_convert(Ptr{Cchar}, s), sizeof(s))
 containsnul(s::AbstractString) = '\0' in s
 
-function unsafe_convert(::Type{Cstring}, s::Union{String,AbstractVector{UInt8}})
+function unsafe_convert(::Type{Cstring}, s::String)
     p = unsafe_convert(Ptr{Cchar}, s)
     containsnul(p, sizeof(s)) &&
         throw(ArgumentError("embedded NULs are not allowed in C strings: $(repr(s))"))

src/array.c

@@ -16,7 +16,15 @@
 extern "C" {
 #endif
 
-#define JL_ARRAY_IMPL_NUL 1
+// similar to MEMDEBUG's purpose, this ensures there is a garbage byte after a
+// most UInt8 arrays (even String-backed) so that almost any C string
+// processing on it is guaranteed to run too far and return some garbage
+// answers to warn the user of their bug.
+#ifdef NDEBUG
+#define JL_ARRAY_MEMDEBUG_TERMINATOR 0
+#else
+#define JL_ARRAY_MEMDEBUG_TERMINATOR 1
+#endif
 
 #define JL_ARRAY_ALIGN(jl_value, nbytes) LLT_ALIGN(jl_value, nbytes)
 
@@ -110,7 +118,7 @@ static jl_array_t *_new_array_(jl_value_t *atype, uint32_t ndims, size_t *dims,
     else if (validated == 2)
         jl_error("invalid Array size");
     if (isunboxed) {
-        if (elsz == 1 && !isunion) {
+        if (JL_ARRAY_MEMDEBUG_TERMINATOR && elsz == 1 && !isunion) {
             // extra byte for all julia allocated byte arrays
             tot++;
         }
@@ -150,9 +158,11 @@ static jl_array_t *_new_array_(jl_value_t *atype, uint32_t ndims, size_t *dims,
 
     if (zeroinit)
         memset(data, 0, tot);
+    if (JL_ARRAY_MEMDEBUG_TERMINATOR && elsz == 1 && !isunion) {
+        ((char*)data)[tot - 1] = 0xfe;
+        msan_allocated_memory(&((char*)data)[tot - 1], 1);
+    }
     a->data = data;
-    if (JL_ARRAY_IMPL_NUL && elsz == 1)
-        ((char*)data)[tot - 1] = '\0';
     a->length = nel;
     a->flags.ndims = ndims;
     a->flags.ptrarray = !isunboxed;
@@ -345,7 +355,7 @@ JL_DLLEXPORT jl_array_t *jl_ptr_to_array_1d(jl_value_t *atype, void *data,
     if (own_buffer) {
         a->flags.how = 2;
         jl_gc_track_malloced_array(ct->ptls, a);
-        jl_gc_count_allocd(nel*elsz + (elsz == 1 ? 1 : 0));
+        jl_gc_count_allocd(nel*elsz);
     }
     else {
         a->flags.how = 0;
@@ -411,7 +421,7 @@ JL_DLLEXPORT jl_array_t *jl_ptr_to_array(jl_value_t *atype, void *data,
     if (own_buffer) {
         a->flags.how = 2;
         jl_gc_track_malloced_array(ct->ptls, a);
-        jl_gc_count_allocd(nel*elsz + (elsz == 1 ? 1 : 0));
+        jl_gc_count_allocd(nel*elsz);
     }
     else {
         a->flags.how = 0;
@@ -476,6 +486,8 @@ JL_DLLEXPORT jl_value_t *jl_array_to_string(jl_array_t *a)
             a->nrows = 0;
             a->length = 0;
             a->maxsize = 0;
+            if (jl_string_data(o)[len] != '\0')
+                jl_string_data(o)[len] = '\0';
             return o;
         }
     }
@@ -657,7 +669,7 @@ static int NOINLINE array_resize_buffer(jl_array_t *a, size_t newlen)
     size_t oldlen = a->nrows;
     int isbitsunion = jl_array_isbitsunion(a);
     assert(nbytes >= oldnbytes);
-    if (elsz == 1 && !isbitsunion) {
+    if (JL_ARRAY_MEMDEBUG_TERMINATOR && elsz == 1 && !isbitsunion) {
         nbytes++;
         oldnbytes++;
     }
@@ -676,11 +688,11 @@ static int NOINLINE array_resize_buffer(jl_array_t *a, size_t newlen)
         // if data is in a String, keep it that way
         jl_value_t *s;
         if (a->flags.isshared) {
-            s = jl_alloc_string(nbytes - (elsz == 1));
+            s = jl_alloc_string(nbytes - (JL_ARRAY_MEMDEBUG_TERMINATOR && elsz == 1));
             newbuf = 1;
         }
         else {
-            s = jl_gc_realloc_string(jl_array_data_owner(a), nbytes - (elsz == 1));
+            s = jl_gc_realloc_string(jl_array_data_owner(a), nbytes - (JL_ARRAY_MEMDEBUG_TERMINATOR && elsz == 1));
         }
         jl_array_data_owner(a) = s;
         jl_gc_wb(a, s);
@@ -700,8 +712,10 @@ static int NOINLINE array_resize_buffer(jl_array_t *a, size_t newlen)
             jl_gc_wb_buf(a, a->data, nbytes);
         }
     }
-    if (JL_ARRAY_IMPL_NUL && elsz == 1 && !isbitsunion)
-        memset((char*)a->data + oldnbytes - 1, 0, nbytes - oldnbytes + 1);
+    if (JL_ARRAY_MEMDEBUG_TERMINATOR && elsz == 1 && !isbitsunion) {
+        memset((char*)a->data + oldnbytes - 1, 0xfe, nbytes - oldnbytes + 1);
+        msan_allocated_memory((char*)a->data + oldnbytes - 1, nbytes - oldnbytes + 1);
+    }
     (void)oldlen;
     assert(oldlen == a->nrows &&
            "Race condition detected: recursive resizing on the same array.");
@@ -975,7 +989,7 @@ STATIC_INLINE void jl_array_shrink(jl_array_t *a, size_t dec)
         oldnbytes += a->maxsize;
     }
 
-    if (elsz == 1 && !isbitsunion) {
+    if (JL_ARRAY_MEMDEBUG_TERMINATOR && elsz == 1 && !isbitsunion) {
         newbytes++;
         oldnbytes++;
     }
@@ -1055,8 +1069,8 @@ STATIC_INLINE void jl_array_del_at_beg(jl_array_t *a, size_t idx, size_t dec,
     if (__unlikely(newoffs != offset) || idx > 0) {
         char *olddata = (char*)a->data;
         char *newdata = olddata - (a->offset - newoffs) * elsz;
-        char *typetagdata;
-        char *newtypetagdata;
+        char *typetagdata = NULL;
+        char *newtypetagdata = NULL;
         if (isbitsunion) {
             typetagdata = jl_array_typetagdata(a);
             newtypetagdata = typetagdata - (a->offset - newoffs);
@@ -1065,7 +1079,7 @@ STATIC_INLINE void jl_array_del_at_beg(jl_array_t *a, size_t idx, size_t dec,
         size_t nb1 = idx * elsz; // size in bytes of the first block
         size_t nbtotal = a->nrows * elsz; // size in bytes of the new array
         // Implicit '\0' for byte arrays
-        if (elsz == 1 && !isbitsunion)
+        if (JL_ARRAY_MEMDEBUG_TERMINATOR && elsz == 1 && !isbitsunion)
             nbtotal++;
         if (idx > 0) {
             memmove_safe(a->flags.hasptr, newdata, olddata, nb1);
@@ -1102,8 +1116,10 @@ STATIC_INLINE void jl_array_del_at_end(jl_array_t *a, size_t idx, size_t dec,
         }
     }
     n -= dec;
-    if (elsz == 1 && !isbitsunion)
-        data[n] = 0;
+    if (JL_ARRAY_MEMDEBUG_TERMINATOR && elsz == 1 && !isbitsunion) {
+        data[n] = 0xfe;
+        msan_allocated_memory(&data[n], 1);
+    }
     a->nrows = n;
     a->length = n;
 }
@@ -1279,47 +1295,6 @@ JL_DLLEXPORT jl_value_t *(jl_array_data_owner)(jl_array_t *a) JL_NOTSAFEPOINT
     return jl_array_data_owner(a);
 }
 
-STATIC_INLINE int jl_has_implicit_byte_owned(jl_array_t *a)
-{
-    assert(a->flags.how != 3);
-    if (!a->flags.isshared)
-        return 1;
-    return a->flags.how == 1;
-}
-
-STATIC_INLINE int jl_has_implicit_byte(jl_array_t *a)
-{
-    // * unshared:
-    //   * how: 0-2
-    //     We own and allocated the data.
-    //     It should have the extra byte.
-    // * shared:
-    //   * how: 0, 2
-    //     The data might come from external source without implicit NUL byte.
-    //     There could be an entra byte for a `reinterpreted` array
-    //     but that should be unlikely for strings.
-    //   * how: 1
-    //     We allocated the data with the extra byte.
-    //   * how: 3
-    //     We should check the owner.
-    if (a->flags.how == 3) {
-        a = (jl_array_t*)jl_array_data_owner(a);
-        if (jl_is_string(a)) return 1;
-        return a->elsize == 1 && jl_has_implicit_byte_owned(a);
-    }
-    return jl_has_implicit_byte_owned(a);
-}
-
-// Create an array with the same content
-JL_DLLEXPORT jl_array_t *jl_array_cconvert_cstring(jl_array_t *a)
-{
-    assert(jl_typeof(a) == jl_array_uint8_type);
-    if (!jl_has_implicit_byte(a))
-        a = jl_array_copy(a);
-    ((char*)a->data)[a->nrows] = 0;
-    return a;
-}
-
 #ifdef __cplusplus
 }
 #endif

src/jl_exported_funcs.inc

@@ -25,7 +25,6 @@
     XX(jl_arrayref) \
     XX(jl_arrayset) \
     XX(jl_arrayunset) \
-    XX(jl_array_cconvert_cstring) \
     XX(jl_array_copy) \
     XX(jl_array_del_at) \
     XX(jl_array_del_beg) \

src/julia_internal.h

@@ -1493,8 +1493,6 @@ STATIC_INLINE void *jl_get_frame_addr(void)
 #endif
 }
 
-JL_DLLEXPORT jl_array_t *jl_array_cconvert_cstring(jl_array_t *a);
-
 // Log `msg` to the current logger by calling CoreLogging.logmsg_shim() on the
 // julia side. If any of module, group, id, file or line are NULL, these will
 // be passed to the julia side as `nothing`.  If `kwargs` is NULL an empty set

test/core.jl

@@ -4554,48 +4554,6 @@ test_shared_array_resize(Int)
 test_shared_array_resize(Any)
 end
 
-module TestArrayNUL
-using Test
-function check_nul(a::Vector{UInt8})
-    b = ccall(:jl_array_cconvert_cstring,
-              Ref{Vector{UInt8}}, (Vector{UInt8},), a)
-    @test unsafe_load(pointer(b), length(b) + 1) == 0x0
-    return b === a
-end
-
-a = UInt8[]
-b = "aaa"
-c = [0x2, 0x1, 0x3]
-
-@test check_nul(a)
-@test check_nul(unsafe_wrap(Vector{UInt8},b))
-@test check_nul(c)
-d = [0x2, 0x1, 0x3]
-@test check_nul(d)
-push!(d, 0x3)
-@test check_nul(d)
-push!(d, 0x3)
-@test check_nul(d)
-ccall(:jl_array_del_end, Cvoid, (Any, UInt), d, 2)
-@test check_nul(d)
-ccall(:jl_array_grow_end, Cvoid, (Any, UInt), d, 1)
-@test check_nul(d)
-ccall(:jl_array_grow_end, Cvoid, (Any, UInt), d, 1)
-@test check_nul(d)
-ccall(:jl_array_grow_end, Cvoid, (Any, UInt), d, 10)
-@test check_nul(d)
-ccall(:jl_array_del_beg, Cvoid, (Any, UInt), d, 8)
-@test check_nul(d)
-ccall(:jl_array_grow_beg, Cvoid, (Any, UInt), d, 8)
-@test check_nul(d)
-ccall(:jl_array_grow_beg, Cvoid, (Any, UInt), d, 8)
-@test check_nul(d)
-f = unsafe_wrap(Array, pointer(d), length(d))
-@test !check_nul(f)
-f = unsafe_wrap(Array, ccall(:malloc, Ptr{UInt8}, (Csize_t,), 10), 10, own = true)
-@test !check_nul(f)
-end
-
 # Copy of `#undef`
 copyto!(Vector{Any}(undef, 10), Vector{Any}(undef, 10))
 function test_copy_alias(::Type{T}) where T