Crystal: Memory corruption(?) after commit "Codegen: use malloc_atomic whenever possible" f1998de8

[splattael]

Hi,

the following largish example corrupts memory(?) of a struct after some iterations:

Reproduction

require "json"
# WORKS: require "gc/null"
# WORKS: GC.disable

abstract struct AbstractEntry
  abstract def as_json

  def to_json(io)
    as_json.to_json(io)
  end
end

struct MyEntry < AbstractEntry
  getter uid : String
  getter country  : String

  def initialize(@uid, @country)
  end

  def as_json
    {
      uid: uid,
      country: country
      # WORKS: if country not exposed
    }
  end
end

class Board
  getter entries

  def initialize
    # WORKS: @entries = Array(Entry).new
    @entries = Array(AbstractEntry).new
  end

  def add(entry)
    @entries << entry
  end
end

private def entry(board, line)
  row = line.split(",")
  entry = MyEntry.new(row[0], row[1])
  board.add(entry)
end

# Main

board = Board.new
line = %{4,GER}
entry(board, line)
# WORKS: Inlined version
# entry = from_csv(line)
# board.add(entry)

previous = nil

10000.times do |i|
  entries = board.entries
  json = entries.to_json

  got = JSON.parse(json)
  #pp got
  pp entries

  if previous && previous != json
    raise "FAILED at ##{i}"
  end

  previous = json
end

p "OK"

Please note the # WORKS comments.
The script bug.cr passes and prints OK:

• if GC is disabled
• if only a single struct field is exposed
• if you avoid AbstractEntry
• if the method entry will inlined

Output

entries # => [MyEntry(@country="GER", @uid="4")]
entries # => [MyEntry(@country="GER", @uid="4")]
entries # => [MyEntry(@country="GER", @uid="4")]
entries # => [MyEntry(@country="GER", @uid="4")]
entries # => [MyEntry(@country="GER", @uid="4")]
entries # => [MyEntry(@country="GER", @uid="4")]
entries # => [MyEntry(@country="", @uid="4")]
entries # => [MyEntry(@country="", @uid="4")]
FAILED at #174 (Exception)
0x466297: *CallStack::unwind:Array(Pointer(Void)) at ??
0x452940: __crystal_main at /home/peter/devel/bug.cr/bug.cr 68:5
0x462e59: main at /opt/crystal/src/main.cr 12:15
0x7eff0e4e5b45: __libc_start_main at ??
0x452069: ??? at ??
0x0: ??? at ??

Note, that MyEntry struct gets modified after 174 (FAILED at #174) iterations:

...
entries # => [MyEntry(@country="GER", @uid="4")]
entries # => [MyEntry(@country="", @uid="4")]
...

:scream:

Git bisect

I was able to git bisect this issue to the commit f1998de8.
To more specific, if I avoid the usage of last = array_malloc_atomic(llvm_type, call_args[1]) (between 0.21.0 and 0.21.1) the memory corruption disappears and bug.cr prints OK, as expected.

Please find the whole story in the following gist:
https://gist.github.com/splattael/d2eca91195da9ef894481a7165b2a9e4

WELP?

This issue might be related to #4319.

Sadly, this bug makes it impossible to use the latest and greatest Crystal versions (> 0.21.0) :cry:

Any hints or suggestions on how fix this bug?
I :heart: Crystal but I am not familiar enough with its implementation (LLVM & other advance computering) to give more insights.

Help is much appreciated :green_heart:

Kind regards,
Peter

[bew]

Note: Changing AbstractEntry and MyEntry to:

abstract struct AbstractEntry
  abstract def to_json(io)
end

struct MyEntry < AbstractEntry
  JSON.mapping uid: String, country: String

  def initialize(@uid, @country)
  end
end

Make it work too, but this is maybe not a solution for you

[splattael]

@bew
Thanks! It might work for me. I have to try it in the real project :+1:

Still, I wonder what I did wrong and how to avoid memory corruptions in the future :fearful:

[asterite]

Abstract structs are buggy and we still didn't have time to figure out why. Just use classes instead for now, or no struct inheritance.

[asterite]

Sorry, I read the above in a haste. I think I have an idea now about why it behaves badly. Thank you!

[splattael]

@asterite Awesome! Right now I am testing @bew's and your solution.
At least avoiding abstract struct (and chosing abstract class as @bew suggested ) solves the issue.

Anyhow, I'd love to see struct being fixed for a better Crystal :smiley:

I hope I did not sound too whiny when I complained about abstract struct in several issues.
At this point, as described above, I was/am lost.

Keep up great work and thank you for all your efforts! :heart: :heart: :heart:

[asterite]

The issue seems to be that malloc_atomic is used for a pointer of an abstract struct.

In any case, I think I'll just revert this whole change. It didn't prove to be more efficient (maybe if you use super large arrays, I don't know) and there's the issue that a struct without pointers isn't guaranteed to not contain real pointers. For example for raising we have a struct that holds the object_id of an exception, so that shouldn't be collected by the GC, but the struct doesn't have pointers. I could keep the change for pointer of ints, but in that case one could also choose to store object ids for some reasons. Since this needs a bit more thought but it produced invalid code, I'll revert it for now.

[asterite]

Unfortunately this doesn't fix #4319

[asterite]

So the advise of not using abstract struct still holds... I think I won't do anything for now, this deserves a bit more discussion and research.

[splattael]

@asterite :+1: Ok, got it. Using abstract class is fine :)

Thank you!

[asterite]

I fixed the case for the abstract struct. At least the code above and similar should work fine now.

[splattael]

@asterite Again, thank you so much! :green_heart:

[asterite]

@splattael No, thank you for the bisect and helping me fix it! 🎉