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

Reopening or sharing LibC types properly #13472

Open
HertzDevil opened this issue May 15, 2023 · 8 comments
Open

Reopening or sharing LibC types properly #13472

HertzDevil opened this issue May 15, 2023 · 8 comments

Comments

@HertzDevil
Copy link
Contributor

It is possible to bind to the same C symbol from two lib funs, as long as their signatures are compatible:

lib A
  fun foo(x : Int32) : Void*
end

lib B
  fun foo(x : Int32) : Void* # okay
end

But as long as structs are involved, two different libraries must choose to declare the same bindings in their own libs:

lib A
  struct Foo
    x : Int32
  end

  fun foo(x : Foo*)
end

lib B
  struct Foo
    x : Int32
  end

  fun foo(x : Foo*) # Error: fun redefinition with different signature (was `fun foo(x : ::Pointer(Foo))` at eval:6:3)
end

or in the same lib:

lib A
  struct Foo
    x : Int32
  end

  fun foo(x : Foo*)
end

lib A
  struct Foo # Error: Foo is already defined
    x : Int32
  end

  fun foo(x : Foo*)
end

Normally this shouldn't happen, but it does when:

  • One shard provides outdated bindings, another one is up-to-date. A project depends on both binding shards, possibly indirectly via dependents
  • One shard provides extra bindings for functionality not covered in the standard library's third-party bindings (e.g. Support PCRE MARK verb #11320, Add bindings/API for LibXML SAX parsing #9048)
  • One shard provides extra LibC bindings, the other "shard" is the C runtime or Win32 bindings in the standard library

My main concern is that as soon as Windows support becomes official, there will surely be a huge influx of new bindings for Win32 APIs, for example via https://github.com/microsoft/win32metadata. There is no guarantee that our own bindings are compatible with those shards. So let's see what a shard might do to circumvent those limitations:

# standard library
lib LibC
  struct Foo
    x : Int32
  end

  FOO = 1

  alias Bar = Void*

  fun foo(x : Foo*)
end

Reopening LibC directly:

# shard
lib LibC
  {% unless LibC.has_constant?("Foo") %}
    struct Foo
      x : Int32
    end
  {% end %}

  {% unless LibC.has_constant?("FOO") %}
    FOO = 1
  {% end %}

  {% unless LibC.has_constant?("Bar") %}
    alias Bar = Void*
  {% end %}

  fun foo(x : Foo*)
end

Using a different lib:

# shard
lib LibShard
  {% if LibC.has_constant?("Foo") %}
    alias Foo = LibC::Foo
  {% else %}
    struct Foo
      x : Int32
    end
  {% end %}

  {% if LibC.has_constant?("FOO") %}
    FOO = LibC::FOO
  {% else %}
    FOO = 1
  {% end %}

  {% if LibC.has_constant?("Bar") %}
    alias Bar = LibC::Bar
  {% else %}
    alias Bar = Void*
  {% end %}

  fun foo(x : Foo*)
end

Both approaches encourage people to read LibC's definitions and require a lot of macros just to get it right. This cannot be fixed by simply moving the C runtime or Win32 APIs in our LibC to another private lib.

@BlobCodes
Copy link
Contributor

Maybe we could always represent all lib structs as unnamed structs internally, so all lib structs with the same types in the same positions are compatible to each other?

@straight-shoota
Copy link
Member

@BlobCodes My first thought went into that direction as well. But then this also poses some problems when types are legitimately represented in different ways.
A simple example would be that two implementations cover discrete views of a C union type. Or sometimes a field maybe represented by just a static array of its size instead of going into the depths of nested structs if the value is completely irrelevant to the bindings.
This would suggest to maybe look more into the size of structs than their data layout.

Maybe even that could potentially cause conflict in some cases, for example with flexible array members.

So I'm wondering if it even makes much sense to validate the compatibility of different bindings to the same symbol.
Sure, in the end probably only one will be the most correct. But that doesn't say that others might not be working as well. Or maybe they're never actually used. I guess that could be an argument to clean up that cruft, even if it's somewhere inside a dependency. But does the compiler have to prevent that? It could easily be a footgun and a mistake that the compiler can spot easily. With a high false positive rate we're also preventing a lot of legitimate use cases, though.

If the fun bindings have different names I think it might be fine to just accept anything and not require any validation for compatibility with other definitions.

Overriding a fun inside the same lib is a somewhat different story about redefinitions of lib bindings (related: #7205, #8422).

@HertzDevil
Copy link
Contributor Author

HertzDevil commented May 15, 2023

Some other examples:

  • The body of an opaque pointer in one binding (alias Foo = Void, Foo*) is documented and a different binding accesses it (struct Foo; ...; end)
  • One binding uses a lib enum for a type whereas another binding uses the integer type as indicated by the funs that use the enum members (usually because the C headers define the enum members using #define without creating a real enum type)

@HertzDevil
Copy link
Contributor Author

I am starting to think there should be a public shard for the complete LibC bindings. All shards that use LibC must use the shard, and all fixes to the bindings themselves will happen in the shard rather than the standard library.

To ensure that nothing can ever require the same bindings multiple times, the filenames between the shard and stdlib should match exactly, so that, assuming the default CRYSTAL_PATH is in place and that only absolute requires are used, every file will search in the shard before stdlib, including the files in stdlib themselves.

The standard library then distributes only the subset of the bindings it needs. The files might look like:

# shard: src/lib_c/sys/stat.cr
lib LibC
  struct Stat
    # ...
  end

  fun stat(pathname : Char*, statbuf : Stat*) : Int
  fun fstat(fd : Int, statbuf : Stat*) : Int
  fun lstat(pathname : Char*, statbuf : Stat*) : Int
end

# stdlib: src/lib_c/sys/stat.cr
lib LibC
  struct Stat
    # identical fields
  end

  fun stat(pathname : Char*, statbuf : Stat*) : Int
end

# stdlib: src/crystal/system/unix/file.cr
require "lib_c/sys/stat"

The above omits handling of platform-specific directories for brevity. We should think about whether the current directory layout immediately under src/lib_c makes sense for the shard.

With LibC being a public shard, there would no longer be a need to have a separate Crystal::System::LibC, and third-party code can continue to refer to types like LibC::Int. They should still depend on the LibC shard though, because there is no guarantee that LibC::Int will be part of stdlib's used subset (in practice this is true, but same cannot be said for e.g. SChar).

Actually generating lib_c/sys/stat.cr in the standard library requires a simple syntactic scan over the whole repository for Call and Path nodes that refer to LibC. It is not a lexical scan since we don't want to include bindings that are only mentioned in comments. To reduce false positives, things that are not actually in LibC, e.g. TerminateProcess and _NSGetExecutablePath, should be moved to different libs (and possibly their own complete binding shards).

The tight coupling between the shard LibC and stdlib LibC implies that it most likely has to be maintained by crystal-lang, so that changes to the two sets of bindings can be rolled out in lockstep. Moving to the shard will be the last breaking change; further removals in stdlib's copy do not count as such, as third-party code that already depends on the shard won't be impacted.

@straight-shoota
Copy link
Member

Having a shard for C bindings sounds good. Not sure how useful it would be though. In general, most common functions of C's standard library should be already covered in Crystal's standard library. The only use for C bindings I see are system-specific features that are not implemented in Crystal's stdlib.
But as mentioned in the previous post, these might not actually be libc but other libraries and should be moved to respective lib bindings.

I'm not sure about integration with stdlib bindings. Why do they need to be the same namespace? The C bindings for stdlib could just be completely encapsulated in Crystal::System::LibC (per #13504). They're an isolated internal detail as part of the stdlib implementation.
The only thing we need in order to be able to avoid interference with 3rd party lib definitions is that you can define bindings to the same functions in different libs with potentially slightly different signatures.

@HertzDevil
Copy link
Contributor Author

that you can define bindings to the same functions in different libs with potentially slightly different signatures

Do you think this outweighs the benefits of having compile-time checks for inconsistent signatures?

For the record, in very special cases the same lib fun can indeed be "overloaded" even in the same library. The prime example is objc_msgSend. (However, because it is truly so generic, instead of declaring a new lib fun for each overload it is easier to cast the fun pointer appropriately.)

@straight-shoota
Copy link
Member

straight-shoota commented Jun 15, 2023

Do you think this outweighs the benefits of having compile-time checks for inconsistent signatures?

Probably? But mainly based on the observation that it's not very common that you would have different bindings to the same function. But in the rare case that this happens, I think the compiler should not throw stones in your way. Especially if the other definition is somewhere in code that you do not own and cannot change (such as stdlib internals, or 3rd party shards).

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Oct 28, 2024

I am thinking if, instead of allowing arbitrary lib fun redeclarations, we only allow them if they have the same arity and the corresponding parameter types produce roughly the same LLVM types, to strike a balance between soundness and flexibility. We define layout compatibility as something like below:

  • Every type is compatible with itself.
  • An integer type is compatible with any signed or unsigned variant of itself.
  • Every enum type is compatible with its base integer type. Two enum types are compatible if their respective base types are (i.e. same type, or same size with opposite signs).
  • All pointers are compatible, regardless of their pointee types.
  • All procs are also compatible with pointers. (We might want to pay attention to call conventions.)
  • Two extern / lib structs or (named) tuples are compatible if they have the same element sizes and their elements are pairwise compatible.
  • Extern / lib unions are similar to structs, except that ideally elements can be in any order. (This is not easy to compute.)
  • We could skip the checks for LLVM intrinsics, because LLVM will shout at you anyway if a declaration is incorrect.

Then for LLVM codegen, Crystal should be able to pick any declaration as the primary one, and use the appropriate casts for calls via the remaining declarations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants