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

[BOLT] Err when linking objects of different architectures #66770

Merged
merged 3 commits into from
Oct 10, 2023

Conversation

mtvec
Copy link
Contributor

@mtvec mtvec commented Sep 19, 2023

This could happen, for example, when instrumenting an AArch64 binary on an x86 host because the instrumentation library is always built for the host.

Note that this check will probably need to be refined in the future as merely having the same architecture does not guarantee objects can be linked. For example, on RISC-V, the float ABI of all objects should match.

This could happen, for example, when instrumenting an AArch64 binary on
an x86 host because the instrumentation library is always built for the
host.

Note that this check will probably need to be refined in the future as
merely having the same architecture does not guarantee objects can be
linked. For example, on RISC-V, the float ABI of all objects should
match.
bolt/lib/Rewrite/JITLinkLinker.cpp Outdated Show resolved Hide resolved
@aaupov
Copy link
Contributor

aaupov commented Sep 22, 2023

Good catch, thanks for the added check. We should err but that doesn't address the rootcause. In principle, we should support something akin to LLVM_RUNTIME_TARGETS.

Can you please add a test with x86 host (REQUIRES: target=x86_64{{.*}}) instrumenting aarch64 binary?

@mtvec
Copy link
Contributor Author

mtvec commented Sep 25, 2023

Good catch, thanks for the added check. We should err but that doesn't address the rootcause. In principle, we should support something akin to LLVM_RUNTIME_TARGETS.

Indeed, this would be nice. I have been working around this by manually building the runtime library for RISC-V in order to use it on an x86 host. It would be better to integrate this in the build system so I might try this out soon.

Can you please add a test with x86 host (REQUIRES: target=x86_64{{.*}}) instrumenting aarch64 binary?

Done! It was a bit tricky though to produce an AArch64 binary suitable for instrumentation on x86. I had to:

  • Force a dummy relocation;
  • Add a dummy _fini function to force DT_FINI to be emitted;
  • Link with -pie (to avoid a static binary being created).

Please let me know if there's an easier way to accomplish this.

@yota9
Copy link
Member

yota9 commented Sep 25, 2023

Yes, these are the minimum requirements. Usually I prefer to use -Wl,-fini flag though, but it doesn't really matter

@mtvec
Copy link
Contributor Author

mtvec commented Sep 25, 2023

Yes, these are the minimum requirements. Usually I prefer to use -Wl,-fini flag though, but it doesn't really matter

Thanks!

As a side note: is AArch64 using DT_FINI_ARRAY instead of DT_FINI? I have a patch ready to let BOLT use the former if the latter is not available; I'll try to submit it today.

@yota9
Copy link
Member

yota9 commented Sep 25, 2023

The BOLT uses DT_FINI currently only, the DT_FINI_ARRAY tag is not even read right now. If you mean at runtime - I think as for any other platforms all FINI_ARRAY and FINI is getting called in a sequence

@mtvec
Copy link
Contributor Author

mtvec commented Sep 25, 2023

The BOLT uses DT_FINI currently only, the DT_FINI_ARRAY tag is not even read right now.

I was wondering if AArch64 doesn't use DT_FINI by default and you have to work around this in BOLT by forcing the linker to emit it through -fini or something similar. That's the case for RISC-V (it only use DT_FINI_ARRAY by default) so that's why I created the patch I talked about before. But maybe we should continue this discussion once I post that patch :)

@yota9
Copy link
Member

yota9 commented Sep 25, 2023

AFAICT the symbol is came from libc and it does nothing but return. But since BOLT tests doesn't use libc the _fini symbol is absent and that is why we have to create it manually in our tests

@mtvec mtvec merged commit 37a8cfb into llvm:main Oct 10, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants