I can reproduce.
@prattmic Possibly a swiss map iteration bug on 32-bit.
I will look more later today.

I suspect that https://github.com/modern-go/reflect2, which is imported by https://github.com/json-iterator/go, can not handle the switch to swissmaps. It reaches into internal details of the runtime with unsafe/linkname/etc. In fact I'm surprised it mostly works.
My immediate thought is that the hiter struct they use is now wrong. Maybe the new hiter is bigger?

I kept the key and value fields in the same place to keep casts OK (reflect2 only accesses those fields AFAICT), but the new maps.Iter type is indeed 8-bytes bigger (64 vs 56 bytes) on 386, so we'll corrupt the stack. maps.Iter is smaller on amd64 (96 vs 104 bytes). The different pointer layout is likely a problem for the GC, even on amd64.

I opened a new issue against reflect2 here: modern-go/reflect2#32

Not sure what if anything the Go project should do about this. I'll leave this issue open in case anyone has some suggestions, but I'm inclined to say "tough luck". But reflect2 is quite popular, so if there was something simple we could do I would consider it.

Thanks for the investigation and quick response.

FWIW I was aware that jsoniter is unmaintained, but I didn’t realise it was this problematic.

A little research shows that Kubernetes decided years ago to move off (although work remains).

Thinking about potential for issues with GC, here are the overlaps on amd64:

type hiter struct {                       | type Iter struct {
        key         unsafe.Pointer        |     key         unsafe.Pointer
        value       unsafe.Pointer        |     elem        unsafe.Pointer
        t           unsafe.Pointer        |     typ         *abi.SwissMapType
        h           unsafe.Pointer        |     m           *Map
        buckets     unsafe.Pointer        |     entryOffset uint64
        bptr        unsafe.Pointer        |     dirOffset   uint64
        overflow    *[]unsafe.Pointer     |     clearSeq    uint64
        oldoverflow *[]unsafe.Pointer     |     globalDepth uint8
        startBucket uintptr               |     dirIdx      int
        offset      uint8                 |     tab         *table
        wrapped     bool                  |
        B           uint8                 |
        i           uint8                 |
        bucket      uintptr               |     group.data  unsafe.Pointer
        checkBucket uintptr               |     entryIdx    uint64
}                                         | }

(These structures are actually the same size. https://go.dev/cl/625275 added one more field to hiter, but that isn't reflected in reflect2, nor was it ever released.)

The pointer/non-pointer mismatches are:

Recorded as pointer, but not actually a pointer:

buckets     <-> entryOffset
bptr        <-> dirOffset
overflow    <-> clearSeq
oldoverflow <-> globalDepth

For these, the GC may throw if these look like pointers that aren't allocated. Also if they are non-zero but look too small to be a valid pointer [1]. The latter is more likely, all of these values will generally be quite small.

Recorded as non-pointer, but actually a pointer:

offset+wrapper+B+i <-> tab
bucket             <-> group.data

For these, if they aren't otherwise reachable then they could be freed early.

Both of these are normally reachable from the map itself, though I don't think reflect2 guarantees the map will still be reachable. (Edit: doh, the map is reachable from Iter). They will become reachable only from Iter if the table grows during iteration [2].

[1] I think? But now I can't find the code.
[2] I haven't double-checked, but I doubt the map can grow in json-iterator, as it wouldn't make sense for that package to mutate the map. But it may apply to other users of reflect2.

It looks like we could potentially work around this problem by rearranging the fields. Move tab and group up to align with a pointer.

Move entryOffset, dirOffset, clearSeq, globalDepth off of pointers. The only problem is that we have 5 non-pointer words, but hiter only has 4.

But globalDepth is small, so if we align it the top byte of a pointer, then that pointer will always look like a kernel address, which is probably OK.

Release blocker until we make a decision.

we have 5 non-pointer words, but hiter only has 4.

I count 6 in Iter - entryOffset, dirOffset, clearSeq, globalDepth, dirIdx, entryIdx.

Here's what I've got so far:

type hiter struct {                       | type Iter struct {
        key         unsafe.Pointer        |     key         unsafe.Pointer
        value       unsafe.Pointer        |     elem        unsafe.Pointer
        t           unsafe.Pointer        |     typ         *abi.SwissMapType
        h           unsafe.Pointer        |     m           *Map
        buckets     unsafe.Pointer        |     tab         *table
        bptr        unsafe.Pointer        |     group.data  unsafe.Pointer
        overflow    *[]unsafe.Pointer     |     clearSeq    uint64
        oldoverflow *[]unsafe.Pointer     |     _unused     uint64 (64-bit) / struct{} (32-bit)
        startBucket uintptr               |     entryOffset uintptr
        offset      uint8                 |     dirOffset   uintptr
        wrapped     bool                  |
        B           uint8                 |
        i           uint8                 |
        bucket      uintptr               |     entryIdx    uint16
                                          |     globalDepth uint8
        checkBucket uintptr               |     dirIdx      int
}   

• I shrank entryIdx to uint16 as it is always <1024. This lets it colocate with globalDepth.
• entryOffset and dirOffset are now uintptr, as we don't need 64-bits of randomness on 32-bit platforms.
• All of the pointers in Iter are now aligned with pointers in hiter.
• All of the non-pointers in Iter are aligned with non-pointers in hiter except clearSeq.

clearSeq still overlaps with the pointer overflow (and oldoverflow on 32-bit). This is somewhat problematic. clearSeq is a count of calls to clear(). It will almost always be quite small, but can technically get arbitrarily large.

We could consider this acceptable. With all of the pointers in Iter covered, we won't get memory corruption. If clearSeq gets too high, applications might get spurious "found pointer to free object" throws, but they should be rare.

Radically different approach:

type hiter struct {                       | type Iter struct {
        key         unsafe.Pointer        |     key         unsafe.Pointer
        value       unsafe.Pointer        |     elem        unsafe.Pointer
        t           unsafe.Pointer        |     iter        *realIter
        h           unsafe.Pointer        |
        buckets     unsafe.Pointer        |
        bptr        unsafe.Pointer        |
        overflow    *[]unsafe.Pointer     |
        oldoverflow *[]unsafe.Pointer     |
        startBucket uintptr               |
        offset      uint8                 |
        wrapped     bool                  |
        B           uint8                 |
        i           uint8                 |
        bucket      uintptr               |
        checkBucket uintptr               |
}                                         | }

We could add an extra layer of indirection. This is unfortunate because it requires an extra allocation, and extra indirection in mapiternext, which will hurt performance. On the other hand, it is much simpler and arguably more future-proof [1], as we can freely modify realIter.

[1] On the other hand, if packages like reflect2 copy this new 3-word Iter type, then we can never grow this struct beyond 3 words, which would be very painful.

We could also mix these ideas, and make clearSeq a pointer to the sequence counter. It is rarely used, so wouldn't hurt performance.

I also propose we add a new function with an open linkname:

//go:linkname newmapiter
func runtime.newmapiter(t *abi.SwissMapType, m *maps.Map) *maps.Iter

This allocates (and initializes) a new iteration type. This would give projects using mapiterinit a migration path where they don't need to define their own iter type (they can just use it as unsafe.Pointer), even if they do keep using mapiternext.

To be clear, they should stop that as well, but this migration would be simpler and would avoid future issues with the iter type definition.

The changes to the internals of the map itself haven't been an issue because maps are allocated with make and users just cast them to unsafe.Pointer to pass to mapassign, etc. Thus the internal structure doesn't matter. This would make iteration similar.

(Sorry for using this issue as brainstorming board :P)

type Iter struct {
    key         unsafe.Pointer
    elem        unsafe.Pointer
    iter        *realIter
}

We could make mapiterinit, mapiternext, mapiterkey, mapiterelem operate on this wrapper type and introduce new functions used by compiler/std that operate on the real iteration type. Then only linkname users will take the indirection performance hit.

The problem with the wrapper, or with newmapiter, is that part of the point of these unsafe shenanigans is to make iteration allocation-free. Both ways of solving this would introduce an allocation. (Unless we could inline newmapiter or mapiterinit somehow? Not sure that's possible when using linkname.)

Actually, maybe reflect2 causes an allocation of an hiter anyway? It is a local variable but I think it escapes.

It does escape (source). I think we'd need quite clever escape analysis to avoid an escape there, even with inlining. It it turns out this function doesn't inline anyway, as it has cost 83...

Regardless, I am more concerned with correctness than performance of these unsafe packages.

In my opinion we should not introduce an extra allocation for all programs in order to support the reflect2 package. That is carrying compatibility too far.

Regardless, I am more concerned with correctness than performance of these unsafe packages.

Sure, so am I. I just want to make sure that we don't add an extra allocation in any new solution that needs adopting, because with that new allocation they won't adopt it.

I am currently prototyping #71408 (comment).

In this approach, normal Go programs will use newly introduced mapiterinit2 and mapiternext2, which behave the same as mapiterinit and mapiternext today. No extra allocation for normal Go programs.

mapiterinit and mapiternext will remain for the sole use of //go:linkname users. mapiterinit will have an extra allocation vs today.

Personally, I think that is a good compromise. No impact on normal Go programs, maintains correctness for users of linkname, and doesn't require any migration.

It's true that linkname users will take a small performance hit. I think that is an acceptable trade-off for maintainability. It's true that there is a question of whether these users will try to work around this compatibility layer. It will be more difficult though, as mapiterinit2 and mapiternext2 won't allow linkname.

The wrapper in #71408 (comment) seems like a good solution that would not slow down normal code (though of course has the cost of needing to be implemented by Michael or whoever).

I think I recall at least one or maybe more of the linkname-related CLs that just redirected the unsafe linkname usage into the normal path, which meant it would function correctly but at "normal" speed (that is, no speed benefit from the linkname). I hunted for a couple of minutes just now to see if I could find an example, but didn't find one right away, but that's my recollection.

Change https://go.dev/cl/643898 mentions this issue: runtime: rename mapiterinit and mapiternext

Change https://go.dev/cl/643899 mentions this issue: runtime: mapiter linkname compatibility layer

FYI Prometheus unit tests pass on all architectures with 1.24RC3. Thanks!