(Re)move some GC pushes to be defensive about JuliaLang#35708 (JuliaL…

…ang#38355)

Somebody sent me a (Julia 1.4) trace that turned out to have the
same root cause as JuliaLang#35708. I took a look around just to be sure
that there was no other instance of this pattern and while I
didn't see any, I did see a useless push/pull pair as well
as a GC_PUSH of an unitialized struct. While neither are a
problem by themselves, they will prevent the GC analyzer from
giving an error if any of the function in between ever become
safepoints (since the GC analyzer doesn't track initilized-ness).
I think it as a rule of thumb, we should never push uninitialized
values into a GC frame. Doing so assumes that there are no safepoints
before the value is fully initialized, but if that is the case,
the GC_PUSH may also be delayed until after initialization and if
the assumption ever changes, at least the GC analyzer will catch it.

src/datatype.c

@@ -927,7 +927,7 @@ JL_DLLEXPORT jl_value_t *jl_new_struct(jl_datatype_t *type, ...)
  return jv;
 }
 
-static void init_struct_tail(jl_datatype_t *type, jl_value_t *jv, size_t na)
+static void init_struct_tail(jl_datatype_t *type, jl_value_t *jv, size_t na) JL_NOTSAFEPOINT
 {
  if (na < jl_datatype_nfields(type)) {
  char *data = (char*)jl_data_ptr(jv);
@@ -951,12 +951,10 @@ JL_DLLEXPORT jl_value_t *jl_new_structv(jl_datatype_t *type, jl_value_t **args,
  if (type->instance != NULL)
  return type->instance;
  jl_value_t *jv = jl_gc_alloc(ptls, jl_datatype_size(type), type);
- JL_GC_PUSH1(&jv);
  for (size_t i = 0; i < na; i++) {
  set_nth_field(type, (void*)jv, i, args[i]);
  }
  init_struct_tail(type, jv, na);
- JL_GC_POP();
  return jv;
 }
 
@@ -983,13 +981,13 @@ JL_DLLEXPORT jl_value_t *jl_new_structt(jl_datatype_t *type, jl_value_t *tup)
  }
  jl_value_t *jv = jl_gc_alloc(ptls, jl_datatype_size(type), type);
  jl_value_t *fi = NULL;
- JL_GC_PUSH2(&jv, &fi);
  if (type->layout->npointers > 0) {
  // if there are references, zero the space first to prevent the GC
  // from seeing uninitialized references during jl_get_nth_field and jl_isa,
  // which can allocate.
  memset(jl_data_ptr(jv), 0, jl_datatype_size(type));
  }
+ JL_GC_PUSH2(&jv, &fi);
  for (size_t i = 0; i < nargs; i++) {
  jl_value_t *ft = jl_field_type_concrete(type, i);
  fi = jl_get_nth_field(tup, i);

src/julia_internal.h

@@ -86,7 +86,7 @@ extern "C" {
 //--------------------------------------------------
 // timers
 // Returns time in nanosec
-JL_DLLEXPORT uint64_t jl_hrtime(void);
+JL_DLLEXPORT uint64_t jl_hrtime(void) JL_NOTSAFEPOINT;
 
 // number of cycles since power-on
 static inline uint64_t cycleclock(void)
@@ -283,6 +283,13 @@ STATIC_INLINE jl_value_t *jl_gc_alloc_(jl_ptls_t ptls, size_t sz, void *ty)
  jl_set_typeof(v, ty);
  return v;
 }
+
+/* Programming style note: When using jl_gc_alloc, do not JL_GC_PUSH it into a
+ * gc frame, until it has been fully initialized. An uninitialized value in a
+ * gc frame can crash upon encountering the first safepoint. By delaying use of
+ * the JL_GC_PUSH macro until the value has been initialized, any accidental
+ * safepoints will be caught by the GC analyzer.
+ */
 JL_DLLEXPORT jl_value_t *jl_gc_alloc(jl_ptls_t ptls, size_t sz, void *ty);
 // On GCC, only inline when sz is constant
 #ifdef __GNUC__

src/module.c

@@ -22,7 +22,6 @@ JL_DLLEXPORT jl_module_t *jl_new_module(jl_sym_t *name)
  const jl_uuid_t uuid_zero = {0, 0};
  jl_module_t *m = (jl_module_t*)jl_gc_alloc(ptls, sizeof(jl_module_t),
  jl_module_type);
- JL_GC_PUSH1(&m);
  assert(jl_is_symbol(name));
  m->name = name;
  m->parent = NULL;
@@ -41,6 +40,7 @@ JL_DLLEXPORT jl_module_t *jl_new_module(jl_sym_t *name)
  JL_MUTEX_INIT(&m->lock);
  htable_new(&m->bindings, 0);
  arraylist_new(&m->usings, 0);
+ JL_GC_PUSH1(&m);
  if (jl_core_module) {
  jl_module_using(m, jl_core_module);
  }

src/support/htable.h

@@ -32,7 +32,7 @@ typedef struct {
 
 // initialize hash table, reserving space for `size` expected number of
 // elements. (Expect `h->size > size` for efficient occupancy factor.)
-htable_t *htable_new(htable_t *h, size_t size);
+htable_t *htable_new(htable_t *h, size_t size) JL_NOTSAFEPOINT;
 void htable_free(htable_t *h);
 
 // clear and (possibly) change size

src/sys.c

@@ -398,7 +398,7 @@ JL_DLLEXPORT int jl_cpu_threads(void) JL_NOTSAFEPOINT
 
 // -- high resolution timers --
 // Returns time in nanosec
-JL_DLLEXPORT uint64_t jl_hrtime(void)
+JL_DLLEXPORT uint64_t jl_hrtime(void) JL_NOTSAFEPOINT
 {
  return uv_hrtime();
 }