Ensure LibUnwind::Exception struct is not atomic

[waj]

Currently any structure not containing any pointer is allocated with GC.malloc_atomic. This makes the GC faster by avoiding scan some memory regions unnecessary.

LibUnwind::Exception actually contains a pointer but it was being casted to UInt64 hidding it from the compiler.

When the ensure block for example, contains calls that ends running the GC, the exception object would be cleaned. Later the exception is re-raised but now with a dangling pointer.

The problem was easily reproduced running this code in release mode:

begin
  raise "Oh no!"
ensure
  GC.collect
end

[bcardiff]

On the arm code there is another exception_object : UInt64 but since pr_cache : ControlBlock_PrCache has nested pointer we are safe there.

[waj]

On the arm code there is another exception_object : UInt64 but since pr_cache : ControlBlock_PrCache has nested pointer we are safe there.

Good point, so this PR might not compile in arm, right?

[straight-shoota]

Does the test code need to be in a separate file? It would be more clear to have the code directly in the spec.
Also is it on purpose that the file has no extension?
The spec should also be disabled for win32 because Process.run does not yet work.

[asterite]

Another possibility is always doing malloc instead of malloc_atomic because others might be storing addresses like that. I know malloc_atomic is supposedly faster or easier for the GC but in my benchmarks that was never the case. Maybe it only matters for really big objects.

[asterite]

I mean, anyone can use UInt64 as an address and we shouldn't just work in a wrong way. Prove me. wrong and I'll change my mind.

[bcardiff]

Pointer should be used. I see no reason UInt64 could not be replaced by Pointer. And if we ever improve to use bitmask for the GC it's better to have clearer semantics. I should not use the overconservativeness of the GC as a feature.

[asterite]

All I'm saying is this:

• We can't prevent users from storing object addresses as UInt64
• I couldn't prove malloc_atomic produced faster code. I just did that optimization because it was "easy" to compute, but, as you can see, it isn't "easy".
• If we remove this optimization the compiler is simplified because we don't need to compute that "has internal pointer" stuff.

So if we want to keep malloc_atomic I would first write a benchmark that proves that it's worth using that at the cost of having this super hard to find and debug issues.

[yxhuvud]

While it certainly is good practice to use Pointer, there are also cases when interacting C libraries where structs can have fields defined as UInt64, but where that size where chosen INTENTIONALLY so that the user should be free to put a pointer there. Do we want to force people that implement bindings for such cases to use Void * or a more specified pointer type, or should it be possible to actually use the C type as it is defined?

I suppose an alternative solution would be to add some sort of directive that tells the compiler to treat it as a pointer. That way open-ended C bindings could be handled. Of course, this may also be nice to have in the very related area of precise GC.

Example: The user_data fields in liburing. https://github.com/axboe/liburing/blob/master/src/include/liburing/io_uring.h#L41. It works by submitting events on one queue and getting back the result on a different queue, with user_data being the way to connect the result to where it came from. In the particular case of liburing it would probably not matter how it is allocated though - there still needs to be something else preventing things from being removed as the events may be hidden away in private copies in kernel memory while the event is being processed.

(liburing is also fun to make bindings for in that it is a heavy user of static inline)

[oprypin]

The "Windows CI" failure is real, btw

[waj]

As @bcardiff said, I wouldn't relay on the conservative feature of the GC when writing bindings and other low level code. All we need to do is write proper documentation.
The performance improvement might not be noticeable on most benchmarks but I'm pretty sure it's possible to design a benchmark to demonstrate the difference. The GC will just skip scanning some memory blocks and we want every bit of CPU time we can reduce from it. So I still think using malloc_atomic when possible it's a good idea. We could add a custom attribute to attach to structures that indicates the runtime what kind of memory block has to be used. But I think the default we have right now does the right thing most of the time.

I'll check the tests running in Windows and ARM and update the PR.

[waj]

Does the test code need to be in a separate file? It would be more clear to have the code directly in the spec.
Also is it on purpose that the file has no extension?

TBH, just because there was other tests doing the same thing :). We can refactor later all these tests and replace with something more elegant. The file has no extension to avoid being picked up as a spec file.

[straight-shoota]

The file has no extension to avoid being picked up as a spec file.

For the record, crystal spec only recognizes files ending with _spec.cr.

[ysbaddaden]

GC.malloc_atomic shouldn't produce noticeably faster code until you allocate lots of memory with it (there can be such cases), since the GC still has to scan stacks and follow all found objects.

Note that a semi-precise GC will skip all attributes (including UInt64) and only follow actual pointers, so this patch is needed nonetheless.