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

Autodetect triple if the default triple is not compatible with the rootfs #655

Closed
wants to merge 1 commit into from

Conversation

ssvb
Copy link

@ssvb ssvb commented May 12, 2015

This allows to inject a statically built x86_64 Crystal compiler into
a 32-bit x86 Linux rootfs and successfully use it there. The compiler
will see that the x86_64 dynamic loader is not present in the system.
And automatically override the triple configuration with the tuple
string obtained by probing the (hopefully) installed GCC or Clang.

It also makes porting Crystal to ARM, MIPS or other non-x86 systems
much easier. Because the statically built x86_64 Crystal compiler
binary can work nicely in the QEMU user chroot and also pick the
right target triple automatically.

More info:
https://wiki.debian.org/Multiarch/Tuples
https://wiki.debian.org/Multiarch/LibraryPathOverview

…otfs

This allows to inject a statically built x86_64 Crystal compiler into
a 32-bit x86 Linux rootfs and successfully use it there. The compiler
will see that the x86_64 dynamic linker is not present in the system.
And automatically override the triple configuration with the tuple
string obtained by probing the (hopefully) installed GCC or Clang.

It also makes porting Crystal to ARM, MIPS or other non-x86 systems
much easier. Because the statically built x86_64 Crystal compiler
binary can work nicely in the QEMU user chroot and also pick the
right target triple automatically.

More info:
  https://wiki.debian.org/Multiarch/Tuples
  https://wiki.debian.org/Multiarch/LibraryPathOverview
@ssvb ssvb changed the title Autodetect triple if the default triple is not compatible with the rotfs Autodetect triple if the default triple is not compatible with the rootfs May 12, 2015
@asterite
Copy link
Member

Looks good, thanks!

But how can a x86_64 Crystal compiler run in a 32 bits environment? (Sorry, I'm not familiar with rootfs).

As a side note, I found this, which should probably also ask the @program if it's 64 bits or not instead of using an ifdef. I wonder how that isn't causing troubles (probably because 32 bits support is not very used and isn't tested).

@jhass
Copy link
Member

jhass commented Aug 31, 2015

A x86_64 kernel is just fine executing i686 code, thus if you chroot into a i686 rootfs you can run everything just fine.

However it's a valid standpoint that one should use setarch or command line flags for tools that detect the environment in such situations, preferring configuration over a detection or heuristic.

In any case I think we need to get the ARM parts out of this PR until we have do have official ARM support. And re-adding them at that point shouldn't be much of a hassle either.

@ssvb
Copy link
Author

ssvb commented Aug 31, 2015

@asterite

But how can a x86_64 Crystal compiler run in a 32 bits environment? (Sorry, I'm not familiar with rootfs).

If x86_64 Crystal compiler is statically built, then it already contains everything and can run in 32-bit userland, assuming that the system uses x86_64 kernel. However it tries to compile 64-bit binaries and link them with 64-bit libraries by default and this fails (unless crosscompilation related command line options are provided).

Dynamically linked executables in ELF format are relying on dynamic loader (which is listed as INTERP header in the readelf -e <elf-executable> report). The name of this dynamic loader is relatively unique and differs for 32-bit and 64-bit executables, some of them are listed at:
https://wiki.debian.org/Multiarch/LibraryPathOverview#ELF_interpreter
If our statically linked 64-bit x86_64 Crystal compiler is unable to find the /lib64/ld-linux-x86-64.so.2 file in the system, then we know for sure that it has been launched in an "alien" environment and the default llvm options are not good for it. And we can guess a better set of options, depending on which dynamic loader is available and/or what kind of triple is used by GCC/Clang installed in the system.

As a side note, I found this, which should probably also ask the @program if it's 64 bits
or not instead of using an ifdef. I wonder how that isn't causing troubles (probably because
32 bits support is not very used and isn't tested).

That's a good find. Probably this explains why using a 32-bit static Crystal compiler binary in a 64-bit rootfs produced broken 64-bit binaries when I tried this test.

@ssvb
Copy link
Author

ssvb commented Aug 31, 2015

@jhass

In any case I think we need to get the ARM parts out of this PR until we have do
have official ARM support. And re-adding them at that point shouldn't be much
of a hassle either.

This is a chicken-egg problem. Full ARM support needs a set of fixes in various places. This particular part is about the only thing, where we can be reasonably confident that it works as expected on ARM too (assuming that this approach is useful on x86 / x86_64).

@jhass
Copy link
Member

jhass commented Aug 31, 2015

I'd still prefer the patches in one, doing the steps inside a fork shouldn't make a difference.

@asterite
Copy link
Member

@ssvb Not sure about this, but maybe #2124 fixed this? If not, either please open another PR or rebase this one. I'm reviewing old pull requests and in a few days I'll close ones without activity.

@ssvb
Copy link
Author

ssvb commented Apr 13, 2016

@asterite Yes, it looks like @ysbaddaden used a similar approach with a static binary for bootstrapping. But my patch is slightly different in a way that it uses autodetection and does not need any special cross-compilation command line options or code patching. I'm not sure what is the current 32-bit x86 bootstrapping support story, but if everyone is already happy with the way how it works, then my pull request can be just closed.

Since there is an "all or nothing" policy regarding ARM support, it indeed makes more sense to have this code as a part of the ARM patchset in my private branch without showing it to anybody until everything is perfect :-)

@@ -240,7 +240,7 @@ module Crystal

private def codegen_single_unit(unit, target_triple, multithreaded)
unit.llvm_mod.target = target_triple
ifdef x86_64
if target_triple =~ /^x86_64/
Copy link
Contributor

Choose a reason for hiding this comment

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

Flags are now set from the target triple (default or specified). I suppose this is no longer required?

Copy link
Author

Choose a reason for hiding this comment

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

Currently if you have a 64-bit static build of the Crystal compiler binary, then it never uses DataLayout32 regardless of what target triple is set via the command line because the relevant code branch is simply not compiled in. Replacing 'ifdef' with 'if' allows to make the selection between the use of DataLayout32 and DataLayout64 at runtime.

Copy link
Member

Choose a reason for hiding this comment

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

@ssvb @ysbaddaden I understand the issue now. You are right, ifdef shouldn't be used inside the compiler. I just pushed this commit.

At first I thought: if you aren't cross-compiling and you are on a 64-bits machine then you want 64-bits executables. But it never occurred to me that you could have a 64-bits compiler inside a 32-bits machine.

Do you think the above commit solves the problem?

@ysbaddaden
Copy link
Contributor

I had your patch in mind when i made the uname to target triple change, but decided to keep things simple for now and let the developer set --target for cross compilation.

This patch may come handy when we start supporting the Android NDK toolchain, thought.

Hamdiakoguz pushed a commit to Hamdiakoguz/crystal that referenced this pull request Apr 30, 2016
@asterite
Copy link
Member

I'm closing this since a lot has happened since this. It's probably fixed, but I don't know. Please reopen if something still not working.

@asterite asterite closed this Sep 29, 2017
@ssvb
Copy link
Author

ssvb commented Sep 29, 2017

Yes, this already has landed in a bit reworked form. It was necessary for ARM architecture support. I can submit a new pull request if anything is still missing.

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.

None yet

4 participants