Add `Crystal::Loader` #11434

straight-shoota · 2021-11-11T21:50:16Z

This patch adds a linker/loader component to the compiler. We need this for interpreted mode and it imitates the behaviour of ld.so/ld for linking and loading dynamic libraries at runtime.

It's a preparatory step for #11159 and adds nothing by itself. But it's an isolated non-trivial component that we can discuss it on its own.

It provides the following functionality:

parse linker arguments (e.g. -L/usr/local/mylibs -lfoo /path/to/libbar.so)
locate the specified libraries and load them into memory (using libdl)
find symbols in the opened libraries and return their addresses

The symbol addresses serve as a point to call in via libffi (not included in this PR).

There are a number of differences compared to linking object files in a normal
object file linking process. Most are caused by limitations of libdl.

Only dynamic libraries can be loaded. Static libraries and object files
are unsupported.
All libraries are loaded into the same namespace. That means libraries
loaded in the compiler program itself may provide symbols to libraries
loaded with Crystal::Loader. Symbols may be available without explicitly
mentioning their libraries. It might be impossible to link against other
verions of the libraries that the compiler is linked against.
A fully statically linked compiler may help dealing with the previous
issue. But using libld in a non-dynamically loaded executable might cause
other issues.

Probably we'll discover more limitations and differences over time.

Resolves #11173

src/compiler/crystal/loader/unix.cr

Co-authored-by: Sijawusz Pur Rahnama <[email protected]>

asterite · 2021-11-12T11:34:56Z

That means libraries
loaded in the compiler program itself may provide symbols to libraries
loaded with Crystal::Loader

Will Crystal::Loader be publicly exposed? I ask because if you use Crystal::Loader in a program other than the compiler, then that sentence is not quite right. It should be "That means libraries loaded in the program that uses Crystal::Loader may provide..."

(I saw this comment in the source code too, that's why I ask)

straight-shoota · 2021-11-12T12:23:02Z

I don't think it should be part of the public API. It's in the compiler source tree. Maybe that could change at some point, if there's demand for it. But that might need other alternations as well.
But I guess I wouldnt't hurt to generalize the comment.

asterite · 2021-11-12T12:38:28Z

Oh, if it's not meant to be publicly exposed than it's fine to leave the comment as is.

maxfierke

This looks awesome! One suggestion on automatically supporting Homebrew-based libs on ARM64

maxfierke · 2021-11-14T15:02:04Z

src/compiler/crystal/loader/unix.cr

+
+    {% if flag?(:darwin) %}
+      default_search_paths << "/usr/lib"
+      default_search_paths << "/usr/local/lib"


Should we add the /opt/homebrew/lib path for ARM64 Darwin? Or do that as a patch on the Homebrew-side? (This would mirror the behavior of the FFI gem, for example)

We should keep the internals exactly as in ld to avoid confusion between linking in interpreted and compiled mode. Any user-defined paths must be configured via DYLD_LIBRARY_PATH or CRYSTAL_LIBRARY_PATH.

j8r · 2021-11-16T00:01:21Z

How the @[Link] annotation will be handled at runtime, will this loader be used too?

asterite · 2021-11-16T09:20:07Z

Yes. Check the interpreter branch to see how it's done now.

beta-ziliani

I left some minor comments

src/compiler/crystal/loader.cr

src/compiler/crystal/loader/unix.cr

beta-ziliani · 2021-11-17T20:40:06Z

src/compiler/crystal/loader/unix.cr

+#   verions of the libraries that the compiler is linked against.
+# * A fully statically linked compiler may help dealing with the previous
+#   issue. But using `libdl` in a non-dynamically loaded executable might cause
+#   other issues.


What kind of issues? Or is it an unknown?

It's unknown (at least to me).

There are projects such as https://github.com/mittorn/custom-linker to mitigate this. I did not encounter any problems in my (limited) tests with statically linked binaries, however. I guess we'll see how this plays out.

src/compiler/crystal/loader/unix.cr

beta-ziliani · 2021-11-18T13:49:24Z

src/compiler/crystal/loader/unix.cr

+    File.each_line(path) do |line|
+      next if line.empty? || line.starts_with?("#")
+
+      if include_path = line.lchop?("include ")


Is include <path> (space before and/or after include) considered invalid in ld.so.conf?

There is a space between include and the path. I'm not sure if more whitespace is allowed or not. Couldn't find a proper format specification (if there even is one). All examples I've looked at have no extra whitespace.

beta-ziliani · 2021-11-18T13:50:37Z

src/compiler/crystal/loader.cr

+  # def find_symbol?(name : String) : Handle?
+  #   raise NotImplementedError.new("find_symbol?")
+  # end
+
+  # def load_file(path : String | ::Path) : Handle
+  #   raise NotImplementedError.new("load_file")
+  # end
+
+  # private def open_library(path : String) : Handle
+  #   raise NotImplementedError.new("open_library")
+  # end
+
+  # def self.default_search_paths : Array(String)
+  #   raise NotImplementedError.new("close_all")
+  # end


This commented code should be removed, right?

I added it there to define the interface that is expected to be implemented in the platform-specific files.

We do the same thing in with platform-specific implementations in Crystal::System.

Co-authored-by: Beta Ziliani <[email protected]>

Sija · 2021-11-19T02:20:38Z

src/compiler/crystal/loader.cr

+      if File.exists?(path)
+        yield path
+      end


Small stylistic suggestion:

Suggested change

if File.exists?(path)

yield path

end

yield path if File.exists?(path)

beta-ziliani

🚀

Add Crystal::Loader

f6a40a7

straight-shoota added kind:feature topic:compiler:interpreter labels Nov 11, 2021

straight-shoota mentioned this pull request Nov 11, 2021

Extract self-contained components from crystal i #11177

Closed

4 tasks

Sija reviewed Nov 11, 2021

View reviewed changes

src/compiler/crystal/loader/unix.cr Outdated Show resolved Hide resolved

Update src/compiler/crystal/loader/unix.cr

ae69b68

Co-authored-by: Sijawusz Pur Rahnama <[email protected]>

straight-shoota mentioned this pull request Nov 12, 2021

Add support for static linking on macOS #11439

Open

maxfierke reviewed Nov 14, 2021

View reviewed changes

straight-shoota added 2 commits November 14, 2021 21:46

Add order spec

e5c5a4f

Check in foo2.c

8e398a6

straight-shoota marked this pull request as ready for review November 16, 2021 14:11

straight-shoota mentioned this pull request Nov 18, 2021

Implement FFI bindings #11475

Merged

beta-ziliani reviewed Nov 18, 2021

View reviewed changes

straight-shoota and others added 2 commits November 18, 2021 20:04

Apply suggestions from code review

ce0952c

Co-authored-by: Beta Ziliani <[email protected]>

Move SHARED_LIBRARY_EXTENSION to unix implementation

bedbe1e

Sija reviewed Nov 19, 2021

View reviewed changes

beta-ziliani approved these changes Nov 23, 2021

View reviewed changes

straight-shoota added this to the 1.3.0 milestone Nov 23, 2021

HertzDevil mentioned this pull request Nov 24, 2021

On making Windows Compilation less convoluted #11486

Closed

straight-shoota merged commit 8f0830c into crystal-lang:master Nov 25, 2021

straight-shoota deleted the feature/loader branch November 25, 2021 12:13

straight-shoota mentioned this pull request Dec 13, 2021

Use Crystal::Loader in the interpreter #11579

Merged

elbywan mentioned this pull request Jun 2, 2022

Crystalline does not build on freebsd. elbywan/crystalline#40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Crystal::Loader` #11434

Add `Crystal::Loader` #11434

straight-shoota commented Nov 11, 2021 •

edited

Loading

asterite commented Nov 12, 2021

straight-shoota commented Nov 12, 2021

asterite commented Nov 12, 2021

maxfierke left a comment

maxfierke Nov 14, 2021

straight-shoota Nov 14, 2021

j8r commented Nov 16, 2021

asterite commented Nov 16, 2021

beta-ziliani left a comment

beta-ziliani Nov 17, 2021

straight-shoota Nov 18, 2021

beta-ziliani Nov 18, 2021

straight-shoota Nov 18, 2021

beta-ziliani Nov 18, 2021

straight-shoota Nov 18, 2021

straight-shoota Nov 18, 2021

Sija Nov 19, 2021 •

edited

Loading

beta-ziliani left a comment

Add Crystal::Loader #11434

Add Crystal::Loader #11434

Conversation

straight-shoota commented Nov 11, 2021 • edited Loading

asterite commented Nov 12, 2021

straight-shoota commented Nov 12, 2021

asterite commented Nov 12, 2021

maxfierke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j8r commented Nov 16, 2021

asterite commented Nov 16, 2021

beta-ziliani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

beta-ziliani left a comment

Choose a reason for hiding this comment

Add `Crystal::Loader` #11434

Add `Crystal::Loader` #11434

straight-shoota commented Nov 11, 2021 •

edited

Loading

Sija Nov 19, 2021 •

edited

Loading