Previously, at two places (in g_object_real_dispose() and shortly before
finalize()), we would call
g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
This clears @quark_weak_notifies at once and then invokes all
notifications.
This means, if you were inside a notification callback and called
g_object_weak_unref() on the object for *another* weak-reference, then
an exception failed:
GLib-GObject-FATAL-CRITICAL: g_object_weak_unref_cb: couldn't find weak ref 0x401320(0x16b9fe0)
Granted, maybe inside a GWeakNotify you shouldn't call much of anything
on where_the_object_was. However, unregistering things (like calling
g_object_weak_unref()) should still reasonably work.
Instead, now remove each weak notification one by one and invoke it.
As we now invoke the callbacks in a loop, if a callee registers a new
callback, then that one gets unregistered right away too. Previously,
we would during g_object_real_dispose() only notify the notifications
that were present when the loop starts. This is similar to what happens
in closure_array_destroy_all(). This is a change in behavior, but it
will be fixed in a separate follow-up commit.
https://gitlab.gnome.org/GNOME/glib/-/issues/1002
g_object_weak_unref() would have done a fast-removal of the entry, which
messes up the order of the weak notifications.
During destruction of the object we emit the weak notifications. They
are emitted in the order in which they were registered (FIFO). Except,
when a g_object_weak_unref() messes up the order. Avoid that and
preserve the order.
Now, do a memmove(), which is O(n). But note that we already track weak
references in a flat array that requires a O(n) linear search. Thus,
g_object_weak_unref() was already O(n) and that didn't change. More
importantly, users are well advised to limit themselves to a reasonably
small number of weak notifications. And for small n, the linear search
and the memmove() is an efficient solution.
At the time when this code was added ([1]), the code and the comment was
correct. g_object_run_dispose() did not clear GWeakRef.
That was later adjusted to clear them ([2]), but at various times it was
not ensured that the GWeakRef was cleared *before* the weak notification
is emitted.
This is now fixed, and the checks for "where_the_object_was" are no
longer necessary. Drop them.
I considered to keep the checks just to be extra safe. But we need to
rely on how g_object_run_dispose() works in detail. By now there is a
test that checks GWeakRef are cleared before emitting the notifications,
so we should not accidentally mess this up and the code is no longer
needed.
[1] commit e82eb490fe ('Handle the case of g_object_run_dispose() in GBinding')
[2] commit a7262d6357 ('gobject: Cleanup weak locations data as part of dispose')
This changes behavior from commit [1] most similar to what was before.
The point of g_object_run_dispose() is to break reference cycles to
bring down an object. We don't expect the object to take new references
to keep it alive for longer. We probably also don't expect it to
register new weak references. We also don't expect the dispose() callees
to check g_weak_ref_get() for the object. In that case, this change
makes not difference.
Note that during g_object_run_dispose() the ref count does not yet go to
zero, still we clear GWeakRef. As such, GWeakRef rather tracks when
objects get disposed, instead of when the ref count really goes to zero.
That is intentional (e.g. issue [2]).
But compare to g_object_unref(), where we also clear GWeakRef *before*
calling dispose. That makes more sense, because inside dispose() (and
for example during weak notifications), we probably want to see that
g_weak_ref_get() indicates the object is already disposed. For that
reason, it seems more correct to clear out the GWeakRef before calling
dispose().
Also, the dispose() callees (e.g. the weak notifications) might refuse to
let the object die by intentionally keeping strong references around.
Not sure why they would do that, it is similar to resurrecting an object
during dispose(). But if they do, they might also want to register new
GWeakRef. In that case, we wouldn't want to unset those newly set
GWeakRef unconditionally right after.
In most cases, it shouldn't make a difference. In the case where it
does, this is the more sensible order of doing things.
[1] commit 2952cfd7a7 ('gobject: drop clearing quark_weak_locations from g_object_real_dispose()')
[2] https://gitlab.gnome.org/GNOME/glib/-/issues/2266
During object initialization, we may want to freeze the notifications,
but only do so once (and once unfreeze at the end).
Rework how that was done. We can avoid an additional GData lookup.
By now, GObjectNotifyQueue gets reallocated. So quite possibly if we
keep the queue, it is a dangling pointer.
That is error prone, but it's also unnecessary. All we need to know is
whether we bumped the freeze count and need to unfreeze. The queue
itself was not useful, because we anyway must take a lock (via
g_datalist_id_update_atomic()) to do anything with it.
Instead, use a nqueue_is_frozen boolean variable.
GSList is almost in all use cases a bad choice. It's bad for locality
and requires a heap allocation per entry.
Instead, use an array, and grow the buffer exponentially via realloc().
Now, that we use g_datalist_id_update_atomic(), it is also easy to
update the pointer. Hence, the GObjectNotifyQueue struct does not point
to an array of pspecs. Instead the entire GObjectNotifyQueue itself gets
reallocated, thus saving one heap allocation for the separate head
structure.
We can tighten up the types which are being used, to prevent the
warnings. Not everything in the world has to be a `guint`.
These warnings only showed up on the macOS CI runner.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
These only show up on macOS. Apparently it’s more sensitive to assigning
`gboolean` (which is secretly `int`) to a `guint` bitfield. 🤷
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
These don’t show up for me on Linux, but are now causing CI failures on
macOS (https://gitlab.gnome.org/GNOME/glib/-/jobs/5006543):
```
../gobject/gclosure.c:923:40: error: implicit conversion changes signedness: 'gboolean' (aka 'int') to 'guint' (aka 'unsigned int') [-Werror,-Wsign-conversion]
ATOMIC_SET (closure, in_marshal, in_marshal);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
```
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
This makes the code a little clearer. In most cases, it’s not a
functional change.
In a few cases, the values are different. I believe the original values
were incorrect (accidentally transposed, perhaps). This never caused an
issue because they were all immediately overwritten during construction
of a `GParamSpec`: these values were defaults in the `instance_init`
vfunc of the `GTypeInstance` for a `GParamSpec`, but the actual min/max
for the `GParamSpec` instance were immediately written over them in the
constructor (such as `g_param_spec_int()`).
Spotted in !4593.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
The "without first having or creating a strong reference" part is wrong.
While we invoke the dispose() method, we always still hold the last
reference. The calling thread called g_object_unref() with a strong
reference that we are about to give up, but at the point where we call
dispose(), we didn't yet decrement the ref count to zero. Doing so would
be a fatal bug.
As such, during dispose() the object is still healthy and still has a
strong pointer. You can call `g_weak_ref_set()` on that pointer without
taking an additional strong reference. Of course, if you don't actually
take a strong reference (and thus don't resurrect the object), then
right afterwards, the last reference is dropped to zero, and the
GWeakRef gets reset again.
But there is no need to claim that you need to take another strong
reference to set a GWeakRef during dispose(). This was always the case.
Also, reword the previous paragraph. I think this is clearer.
These are all fairly straightforward, but I didn’t get them locally;
they only showed up on CI.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
Fixing #3405 is going to take a lot of work, so let’s split it up into
pieces and work on them separately. The `gobject/` and `gobject/tests/`
directories now compile cleanly with `-Wsign-conversion` (see the
previous commits), so let’s enable the warning for those directories to
prevent regressions while we continue to work on the other directories.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
There’s a painful inconsistency in the types of the
`g_{test_rand,random,rand}_int{,_range}()` functions, which vary
arbitrarily between `gint32` and `guint32`.
Unfortunately since those functions mention `int` explicitly in the name
(and then some of them return an `unsigned` integer), I don’t see a way
to make the APIs consistent without significant deprecations or
additions.
So, for the moment, to fix various `-Wsign-conversion` warnings, plaster
the tests with casts.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
This fixes a load of -Wsign-conversion warnings. The dest type setter
function is being used (presumably by design?) so there’s sometimes a
type mismatch (signed/unsigned, or size) with the constant value being
used by the test. This just makes the existing implicit casts explicit.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
Fix all the instances where `-Wsign-conversion` was pointing out that
`g_assert_cmpint()` had been used on unsigned inputs, or vice-versa.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
Not sure why these constants were chosen the way they were, but that’s
not a problem I’m going to investigate right now. This just makes the
implicit cast explicit to shut the compiler warning up.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
This fixes `-Wsign-conversion` warnings, though I’m not sure why the
compiler is emitting them. The signed/unsigned status of flag enum
members is not particularly well defined in the C standard (and even
less well understood by me), so just do what seems necessary to shut the
compiler up.
The benefits of enabling `-Wsign-conversion` across the codebase
hopefully outweighs this noise.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
While we’re at it, rename the variables to make the intent a bit
clearer: these functions return a boolean indicating whether any of the
values were modified to make them valid. `n_changed` is a counter of the
number of modified values.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
Unfortunately the signatures of our atomic functions alternate between
using signed and unsigned integers across different functions, so we
can’t just use one type as input. Add some explicit casts to fix
harmless `-Wsign-conversion` warnings.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
Rather than reinventing it ourselves. The old version in `gboxed.c`
could lose the second half of very long strings, as it truncated the
`size_t` string length to the `ssize_t` accepted by
`g_string_new_len()`.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
Now all accesses to quark_notify_queue are guarded by the GData lock.
Several non-trivial operations are implemented via
g_datalist_id_update_atomic().
The OPTIONAL_BIT_LOCK_NOTIFY lock is thus unnecessary and can be dropped.
Note that with the move to g_datalist_id_update_atomic(), we now
potentially do more work while holding the GData lock (e.g. some code
paths allocation additional memory). But note that
g_datalist_id_set_data() already has code paths where it must allocate
memory to track the GDataElt. Also, most objects are not used in
parallel, so holding the per-object (per-GData) lock longer does not
affect them. Also, many operations also require a object_bit_lock(), so
it seems very unlikely that you really could achieve higher parallelism
by taking more locks (and minimizing the time to hold the GData lock).
On the contrary, taking one lock less and doing all the work there is
beneficial.
A common pattern is to look whether a GData entry exists, and if it
doesn't, add it.
For that, we currently always must take a OPTIONAL_BIT_LOCK_NOTIFY lock.
This can be avoided, because GData already uses an internal mutex. By
using g_datalist_id_update_atomic(), we can perform all relevant
operations while holding that mutex.
Move functionality from g_object_notify_queue_freeze() inside
g_datalist_id_update_atomic().
The goal will be to drop the OPTIONAL_BIT_LOCK_NOTIFY lock in a later
commit.
With the previous changes, all accesses to the WeakRefStack go through
g_datalist_id_update_atomic() and are guarded by the GData's bit lock.
At this point, the OPTIONAL_BIT_LOCK_WEAK_REFS lock is unnecessary and
can be dropped.
A minor benefit is that g_object_weak_{ref,unref}() needs now one lock
less.
Also note that this rework fixes a potential race for weak refs. Note
that we have two calls
g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
that don't take OPTIONAL_BIT_LOCK_WEAK_REFS lock. One of these calls is
right before finalize(). At that point, no other thread can hold a
reference to the object to race and we are good. However, the other call
is from g_object_real_dispose(). At that point, theoretically the object
could have been resurrected and a pointer could have been passed to
another thread. Calling then g_object_weak_ref()/g_object_weak_unref()
will race. We would have required a OPTIONAL_BIT_LOCK_WEAK_REFS lock
around those g_datalist_id_set_data(,,NULL) calls.
Instead, this is now also fixed, because every update to the
WeakRefStack happens while holding the GData lock. So if you call
g_datalist_id_set_data(,,NULL), the WeakRefStack is removed from the
GData (and later freed by weak_refs_notify() and can no longer
concurrently updated by g_object_weak_{ref,unref}().
This is a step step to drop OPTIONAL_BIT_LOCK_WEAK_REFS lock. See the
next commits why that is done.
Also, free the WeakRefStack, if there are no more references.
Previously, it was never freed.
It was always passed the same value by all users of the macro.
This introduces no functional changes.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Rather than atomically querying the flags, and then atomically reffing
the `GClosure`, do the atomic ref and return the new flags value as a
side-effect. This eliminates the need for an additional atomic read, and
eliminates a race window between the two accesses.
It does mean we need a new internal version of `g_closure_ref()` which
returns the new `GClosureFlags` though.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Remove the last few non-atomic reads of `GClosure.ref_count`. See
previous commits for an explanation of why this is important.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Currently, `GClosure` does atomic writes to the flags in its initial
bitfield, but it never does atomic reads from them. This is not safe —
even if the memory read for an int is atomic, the compiler might
duplicate or rearrange a non-atomic read and give an unexpected result.
Using an atomic fetch inserts the necessary compiler and hardware
barriers to ensure a single result is fetched.
(See !4575 of an example of where this kind of bug has caused
misbehaviour.)
So, introduce an atomic read helper for the `GClosure` bitfield. Rather
than reading a single member of the bitfield, it returns the entire
bitfield, as several `GClosure` methods need access to multiple members
of the bitfield, and atomically fetching them all separately would not
be atomic overall.
Fix various `GClosure` methods to use the new atomic read function.
There are more parts of the code still to fix — these were just the
straightforward ones.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Depending on luck, g_closure_ref may access closure->vint and observe
different values between reads. This manifests as a test failure in
signals-refcount{2,4}, properties-refcount1, and closure-refcount depending
on timing and re-runs.
Jakub Jelinek analysed this on GCC bug PR119607 after I'd reported it
over there as a possible GCC regression.
The critical part being g_closure_ref -> ATOMIC_INC_ASSIGN -> ATOMIC_CHANGE_FIELD
where closure->vint gets re-read repeatedly, both outside and inside the retry
loop. To fix that:
1. Atomically fetch it the first time;
2. Use the cached read, not a fresh read, of vint in the loop;
3. Use g_atomic_int_compare_and_exchange_full in the loop so we get a freshly
cached vint if it changed in another thread.
Bug: https://gcc.gnu.org/PR119607
Fixes: 834ddd19 ('turned all modifications to the first 32 integer bits in a closure into')
Co-authored-by: Jakub Jelinek <jakub@redhat.com>
Previously, we would call
g_datalist_id_set_data (&object->qdata, quark_closure_array, NULL);
which called destroy_closure_array() on the CArray.
At that point, it would iterate over the CArray, and invalidate all
closures. But note that this invokes external callbacks, which in turn
can destroy other closures, which can call object_remove_closure().
But now that closure can no longer be found and an assertion fails.
Instead of removing the entire CArray at once, remove each closure one
by one in a loop.
This problem is similar to issue 1002, except here it's about closure
watches instead of GWeakNotify.
Note that now we destroy closures one-by-one in a loop, and we iterate
the loop as long as we have closures. That makes a difference when a new
closure gets registered while we destroy them all. Previously, newly
registered closures would survive. It would be possible to implement the
previous behavior, but I think the new behavior is better. It is anyway
a very remote use case.
This is a GDestoryNotify that asserts not to be called. Obviously, this
is only for testing and asserting (defensive programming).
It is private API inside "libgobject-2.0.so.0". If unused, we rely that
the linker can strip it out.
There are two calls to
g_datalist_id_set_data (&object->qdata, quark_closure_array, NULL);
that don't take a OPTIONAL_BIT_LOCK_CLOSURE_ARRAY lock. These are inside
g_object_real_dispose() and right before finalize(). The one before
finalize() is fine, becase we are already in a situation where nobody
else holds a reference on object.
However not so with g_object_real_dispose(). That is called after we
checked that there is only one strong reference left and we are inside
the call to dispose(). However, at that point (before chaining up
g_object_real_dispose()) the callee is able can pass the reference
to another thread. That other thread could create a Closure and destroy it
again. This calles object_remove_closure() (accessing CArray) which now
races against g_object_real_dispose() (destroying CArray).
Granted, this is very unlikely to happen. But let's try to avoid such
races in principle.
We can avoid this problem with less overhead by doing everything while
holding the GData lock, using g_datalist_id_update_atomic(). This is
probably even faster, as we don't need the additional
OPTIONAL_BIT_LOCK_CLOSURE_ARRAY lock.
Also free the empty closure data during object_remove_closure(). This
frees some unused memory.
