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

Remove most unused LibC bindings #11955

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

HertzDevil
Copy link
Contributor

Those are:

  • c/stdlib.cr: atof, div, putenv, DivT
  • c/time.cr: clock_settime, gmtime_r, localtime_r, mktime, timegm, timezone, tzset, Tm, $daylight, $timezone, $tzname
  • c/netdb.cr: getnameinfo
  • c/sys/select.cr: select, FdSet
  • c/sys/stat.cr: mkfifo, mknod, umask

These functions are not removed despite also being unused:

@didactic-drunk
Copy link
Contributor

  • mkfifo, mknod, umask

I have plans for those.

@straight-shoota
Copy link
Member

straight-shoota commented Apr 21, 2022

I'm not sure about how to proceed with this. It's definitely nice to remove unused code.

But we can't exclude that these bindings are used in some Crystal code, despite lib bindings being undocumented. Removing them would risk break existing code that is just using C bindings provided in stdlib in good faith.

So I believe this is a breaking change that should only happen in a major release. So we'd have to schedule this for 2.0.

In the mean time, we could consider adding support for @[Deprecated] to fun bindings. I'm not sure that's really very useful because lib bindings are not intended to be part of a public API. But even internal uses could be tracked with deprecation warnings.

I think the underlying problem is that libc is just a huge library and stdlib contains bindings for some of it in LibC (mostly things that we need for stdlib, but also some extras as seen here), but it's far from complete (and we'd never aim for completeness).
When a shard uses some other libc functions, they'll probably declare additional bindings for LibC. At this point it becomes unclear which parts of LibC are the custom bindings and which are provided by stdlib. If somebody want's to use some of the functions removed here, they wouldn't add them themselves but just re-use the existing declarations from stdlib.

Perhaps we should move all of stdlib's lib bindings to a different name than LibC to make it more clear that they're meant for internal use. Crystal::System::LibC would be explicit about that.

@didactic-drunk
Copy link
Contributor

The major problem I face is platform specific or variant libc calls. Crystal has a platform specific file/dir structure for libc. Shards are left to reimplement platform specific binding on their own. The most common patterns I see are {% if flags(:linux) %} or attempting to compile a c file.

If only there was a standardized way to query a lib for functions or conditionally bind them

lib LibC
  fun spork?(prongs : Int)
  bind? spork(prongs : Int)
end

Alternatively;

lib LibC
  {% if LibC.c_function_exists? %}
    fun spork(prongs : Int)
  {% end %}
end

Bonus if someone comes up with a way to query for variant params or types. See sendfile on Linux vs BSD.

@straight-shoota
Copy link
Member

Detection of symbols in linked libraries is not really possible at compile time.
Shards can actually use the same mechanism as stdlib, placing libc bindings in paths containing the target triple: lib_c/x86_64-linux-gnu. This should work in the same way as it does in stdlib.

@asterite
Copy link
Member

Check my proposal for how to more easily do C bindings... I think that solves all issues.

@asterite
Copy link
Member

Ah, nevermind. It's something I different. But I think we could have a macro call for this.

@HertzDevil
Copy link
Contributor Author

Related discussion of Crystal::System::LibC: #7339 (comment)

I think that change is fine, except for the C primitive aliases like LibC::Char.

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.

4 participants