06:59 <arigo_> cfbolz: let me just say, aaaaaaaa what?

06:59 <arigo_> this bug seems easy to reproduce and was there since the introduction of incminimark, but wasn't discovered until now?

07:00 <arigo_> (easy once you know it of course)

07:09 <cfbolz> arigo_: yes :-(

07:09 <cfbolz> arigo_: in practice you need to be extremely unlucky though, things need to happen in *just* the wrong order

07:10 <arigo_> and writing references into prebuilt objects is kinda rare

07:10 <cfbolz> yes

07:10 <cfbolz> it's a hypothesis of mine, but maybe it started happening more because I added some more caching mechanisms into pycode (of which there are quite a few prebuilt ones)

07:11 <cfbolz> arigo_: it's definitely an example where a model checker would be cool :-)

07:12 <cfbolz> or maybe i should write a hypothesis stateful test

07:12 <arigo_> I thought we already had some random tests

07:13 <cfbolz> for some things like the minimarkpages, yes

07:23 <cfbolz> arigo_: I wonder whether the four lines starting here: https://github.com/pypy/pypy/blob/main/rpython/memory/gc/incminimark.py#L2410 were trying to address the same problem (the commit has no tests though)

07:30 <arigo_> maybe the GC is already correct except for the assert

07:31 <arigo_> but it would require proof

07:31 <cfbolz> arigo_: yes, that's entirely plausible, except for the fact that I find crashes that are exactly freeing objects that are still reachable (in the same programs that fail with the assert earlier)

07:32 <cfbolz> but I didn't manage to reproduce the crash in a small test

07:32 <cfbolz> so I'm not entirely satisfied with the current situation yet

07:34 <cfbolz> and so far it looks like the crashes go away with my changes too. but I would still like to find a test :-(

07:49 <arigo_> the line that says "black -> white-but-prebuilt-so-dont-care", did you add that just now?

07:53 <arigo_> ah I see, the flag NO_HEAP_PTRS is removed, so you get a "black -> white-but-prebuit-but-no-longer-NO_HEAP_PTRS"

07:56 <arigo_> what did you change?

08:30 <cfbolz> arigo_: what I changed is that if NO_HEAP_PTRS is removed, I also add the object to the to-be-marked list (objects_to_trace) if we're in the MARKING phase

08:30 <cfbolz> in the write barrier

08:31 <cfbolz> this turns it from white to gray

08:49 <cfbolz> commit: https://github.com/pypy/pypy/commit/497818528f7b6992afdf028d03e39a4646619305

08:52 <arigo_> OK, sounds reasonable to me

08:52 <cfbolz> arigo_: yes, I'm pretty sure it's not "wrong" conceptually

08:52 <arigo_> maybe a different fix would be to record the length of self.prebuilt_root_objects at the start of a collection, and at the end, if it grew, trace the extra objects

08:52 <cfbolz> what I'm doing now is to write a random test that would have found the same assertion error

08:53 <cfbolz> arigo_: because right now it's tracing them all?

08:54 <arigo_> ah, good question. maybe or maybe not, but what I said would still fix it, no?

08:54 <cfbolz> yeah

08:55 <arigo_> at worst it's looking a second time unnecessarily into some prebuilt objects, but at most once per prebuilt object for the lifetime of the process

08:56 <cfbolz> yeah. I think this is essentially what my fix does too, at the cost of adding it to objects_to_trace (instead of saving the index)

08:56 <arigo_> but you need to fix some _debug_objects_to_trace_dict1/2 too in DEBUG mode, no?

08:57 <cfbolz> no, why?

08:57 <cfbolz> _debug_objects_to_trace_dict1 is the dict version of objects_to_trace

08:57 <arigo_> I'm just confused because you're adding stuff to objects_to_trace but not _debug_objects_to_trace_dict1

08:57 <cfbolz> the debug mode is quite inefficient, it just recreates these dicts at every collection

08:58 <cfbolz> it does this: self._debug_objects_to_trace_dict1 = self.objects_to_trace.stack2dict()

08:59 <arigo_> ah, and the write barrier is outside collections, of course

08:59 <cfbolz> yes

08:59 <cfbolz> I also had to swap back in a lot 😅

09:01 <arigo_> "XXX do we need to add the dest_addr to objects_to_trace too?" I would say yes

09:02 <cfbolz> yeah, I think so too

09:03 <arigo_> it's a really rare case, and it's safe to do so anyway

09:03 <cfbolz> I wonder who copies things into prebuilt arrays ;-)

09:03 <cfbolz> ok, a random test finds the assert immediately, good

09:03 <cfbolz> if I undo the change

09:09 <cfbolz> but so far I cannot find a crash if I turn debugging off

15:48 <arigo_> cfbolz: actually... I think that prebuilt_root_objects is going to be completely walked anyway at the end of marking?

15:49 <arigo_> a bit confused

15:51 <arigo_> doesn't this mean that there was never any problem in the GC? apart from the debug-mode asserts triggering when they shouldn't?

15:57 <arigo_> you said that https://github.com/pypy/pypy/commit/497818528f7b6992afdf028d03e39a4646619305 appears to fix something, but I don't get why right now, because whatever you add to 'objects_to_trace' is also added to 'prebuilt_root_objects' and that's going to be seen in https://github.com/pypy/pypy/blob/789f964fff59c722b0872abcdc56d2b1373a9f3b/rpython/memory/gc/incminimark.py#L2636, which essentially copies 'prebuilt_root_objects' into 'objects_to_trace'

15:59 <arigo_> I think it should have no effect, apart from adding the object to _debug_objects_to_trace_dict1 earlier, which silences the assert

16:22 <cfbolz> arigo_: there is definitely a problem in the GC, because the nanobind tests crash with asserts turned off

16:24 <cfbolz> so even if it "just" fixes the asserts, that sounds useful to me, because then I can try to re-run nanobind and get an *actual* assertion error

16:25 <cfbolz> I agree with your argument, fwiw

16:33 <cfbolz> (the crash is unfortunately somewhat rare, so it will take a while to repeatedly run it and see whether it reoccurs)

16:35 <cfbolz> arigo_: or would you instead simply change the assert to also count the objects in prebuilt_root_objects as grey?