Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Atomics of pointer types #14401

Merged
merged 5 commits into from
Apr 16, 2024

Conversation

HertzDevil
Copy link
Contributor

This is needed for things like Atomic(LibC::HANDLE) (see #14389).

@straight-shoota
Copy link
Member

straight-shoota commented Mar 26, 2024

Specs are (still) failing

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Mar 27, 2024

The failure happens when compiling spec/std/atomic_spec.cr and spec/std/xml/reader_spec.cr together on Crystal 1.2 or below. Reduction:

require "spec"
require "xml"

describe Atomic do
  describe "#compare_and_set" do
    it "with pointer" do
      atomic = Atomic.new(Pointer(Void).null)
      atomic.get.should eq(Pointer(Void).null)
    end
  end
end

module XML
  describe Reader do
    describe "#to_unsafe" do
      it "returns a pointer to the underlying LibXML::XMLTextReader" do
        reader = Reader.new("<root/>")
        reader.to_unsafe.should be_a(LibXML::XMLTextReader)
      end
    end
  end
end

This is effectively #8544 and has been fixed in #11636.

@straight-shoota
Copy link
Member

Oh what a mess. What options do we have? I suppose we could skip either of those specs when building with an old compiler? 🤔
It would only be necessary when both are combined, but that might be hard to detect? We could look whether XML is defined in the atomic spec though...

@HertzDevil
Copy link
Contributor Author

We could also simply replace the typedefs with aliases on versions before 1.3, or even without a version check

@straight-shoota
Copy link
Member

Related discussion about dropping type entirely from the language: #10031

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exhibits the same issue as #14402: Pointer#address and Pointer#object_id are both UInt64, whatever the actual pointer size, so that will require a DWCAS on 32-bit archs :(

@straight-shoota
Copy link
Member

@ysbaddaden I suppose there's no way around that until we introduce arch-specific pointer sizes. If you need to put a pointer into an atomic, you'll have to eat that. This PR doesn't really change anything about that because the current workaround would be to put Pointer#address in the atomic.

I suppose you could technically downcast the integer type on a arch with smaller pointer size, but unlikely anyone is going this extra length. Anyways, if you want, you can still do that after this PR. It just enables an easier use case without invalidating anything that worked before.

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Apr 4, 2024

Looking at this again, if sizeof(Void*) is target-specific but the type of Pointer#address isn't, then casting a smaller pointer to UInt64* is simply wrong and leads to stack corruption

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @HertzDevil it looks perfect!

Shall we extract the libxml2 fix to have a distinct commit in master, or just squash it here?

@straight-shoota
Copy link
Member

straight-shoota commented Apr 15, 2024

Shall we extract the libxml2 fix to have a distinct commit in master, or just squash it here?

I missed this comment. But it seems like a good idea, it's an unrelated change.
Let's wait for #14494 then.

@straight-shoota straight-shoota merged commit 0361f1f into crystal-lang:master Apr 16, 2024
58 checks passed
@HertzDevil HertzDevil deleted the feature/atomic-ptr branch April 16, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants