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

cmd/link: ABI hash of a shared library changes if any inlineable function changes #23405

Open
bryanpkc opened this Issue Jan 10, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@bryanpkc
Contributor

bryanpkc commented Jan 10, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

Tip.

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

linux_amd64

What did you do?

  1. Install a package containing an exported inlineable function in shared mode.
  2. Build an executable that links against the shared library produced in step 1.
  3. Make a minor change to the package, without changing the ABI, and re-install the shared library.
  4. Run the executable produced in step 2.

You can reproduce this easily with the makefile in my example:

https://github.com/bryanpkc/pkgdef-example

What did you expect to see?

Correct execution of the main executable.

What did you see instead?

$./main
abi mismatch detected between the executable and libgithub.com-bryanpkc-pkgdef-example-arith.so
fatal error: abi mismatch
runtime: panic before malloc heap initialized

Proposed solution

The ABI hash of a shared library (or Go plugin) is computed as a SHA1 sum of the package hashes of all packages included in the shared library, which are in turn computed from the export data of the packages (i.e. the contents of __.PKGDEF in the corresponding package archives). Currently the export data of a package includes the ASTs of all inlineable exported functions in the package. Doing this allows cross-package inlining in the usual static compilation scenario, but also makes the ABI hashes of shared libraries unnecessarily unstable.

This can be fixed by:

  1. When computing the ABI hash for the package, exclude the list of inlineable functions from a package's export data.
  2. Do not allow cross-package inlining from a dynlink package archive into a dependant program. (This may already be the case today, but I am not 100% sure.)
@bryanpkc

This comment has been minimized.

Show comment
Hide comment
@bryanpkc

bryanpkc Jan 10, 2018

Contributor

/cc @mwhudson

Contributor

bryanpkc commented Jan 10, 2018

/cc @mwhudson

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Jan 10, 2018

Contributor

We don't have any mechanism for preventing inlining of a package that is put into a shared library. The problem is that inlining is done by the compiler, but determination of which packages will be pulled from a shared library is determined by the linker. Any fix we make here would have to address that somehow.

Contributor

ianlancetaylor commented Jan 10, 2018

We don't have any mechanism for preventing inlining of a package that is put into a shared library. The problem is that inlining is done by the compiler, but determination of which packages will be pulled from a shared library is determined by the linker. Any fix we make here would have to address that somehow.

@ianlancetaylor ianlancetaylor changed the title from ABI hash of a shared library changes if any inlineable function changes to cmd/link: ABI hash of a shared library changes if any inlineable function changes Jan 10, 2018

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jan 10, 2018

@hirochachacha

This comment has been minimized.

Show comment
Hide comment
@hirochachacha

hirochachacha Jan 11, 2018

Contributor

Related to #21510.

Contributor

hirochachacha commented Jan 11, 2018

Related to #21510.

@bryanpkc

This comment has been minimized.

Show comment
Hide comment
@bryanpkc

bryanpkc Jan 18, 2018

Contributor

As I said in point 1 above, when the compiler is invoked with -dynlink, it can omit the inlineable functions from a package's export data. This will ensure that any other package the imports it will not be able to inline its functions. I do realize that this will hurt the performance of multi-package shared libraries, but multi-package shared libraries are not common outside of libstd.so, and we could try to come up with a special case for the latter.

Contributor

bryanpkc commented Jan 18, 2018

As I said in point 1 above, when the compiler is invoked with -dynlink, it can omit the inlineable functions from a package's export data. This will ensure that any other package the imports it will not be able to inline its functions. I do realize that this will hurt the performance of multi-package shared libraries, but multi-package shared libraries are not common outside of libstd.so, and we could try to come up with a special case for the latter.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Jan 19, 2018

Contributor

That doesn't sound precisely right. -dynlink is used when building with -linkshared, and we don't need to disable inlining in that case. But, otherwise, yes, that sounds good. I didn't understand that was what you were suggesting originally.

Contributor

ianlancetaylor commented Jan 19, 2018

That doesn't sound precisely right. -dynlink is used when building with -linkshared, and we don't need to disable inlining in that case. But, otherwise, yes, that sounds good. I didn't understand that was what you were suggesting originally.

@bryanpkc

This comment has been minimized.

Show comment
Hide comment
@bryanpkc

bryanpkc Jan 19, 2018

Contributor

You're right, I made a mistake when I said -dynlink. -shared might be a more appropriate flag, but currently it is not passed to compile in "shared" mode.

Contributor

bryanpkc commented Jan 19, 2018

You're right, I made a mistake when I said -dynlink. -shared might be a more appropriate flag, but currently it is not passed to compile in "shared" mode.

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