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

Investigate why the UEFI targets produce executables with DLLImport annotations #101656

Closed
nicholasbishop opened this issue Sep 10, 2022 · 3 comments
Labels
A-linkage Area: linking into static, shared libraries and binaries O-UEFI UEFI T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nicholasbishop
Copy link
Contributor

This is a followup to #101377.

Originally posted by @dvdhrm in #101377 (comment):

It feels wrong to produce executables with DLLImport annotations, knowing that UEFI loaders will never read .idata sections. While technically they could, I am not aware of any such implementations and the spec clearly states All other linkage to and from an UEFI image is done programmatically.

@nicholasbishop nicholasbishop changed the title Investigate why the UEFI targets produce executables with DLLImport annotatiosn Investigate why the UEFI targets produce executables with DLLImport annotations Sep 10, 2022
@bjorn3 bjorn3 added A-linkage Area: linking into static, shared libraries and binaries T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. O-UEFI UEFI labels Sep 10, 2022
@dvdhrm
Copy link
Contributor

dvdhrm commented Nov 21, 2022

I tracked this down. The error we get is in the LLVM IR verification. Globals (code, data, ...) are called GlobalValue in the IR and can have a set of attributes. One of those is called dso_local (the linker / lto-compiler can rely on the symbol to be local and not interposable), another is called dllimport which just tells the compiler to use indirect-calls as is common for dll imports.

The llvm-codegen always marks rust symbols from crates other than the current compilation as DllImport (search for DllImport in compiler/rustc_codegen_llvm/src/consts.rs). This allows dynamic linking between rust crates, while relying on the linker to correctly fix things up when linking statically.

At the same time, when dropping PIC and using the static relocation model, all symbols in a compilation are marked as dso_local, because the assumption is that nothing is ever linked dynamically. LLVM now correctly complains that the combination makes no sense.

I think the underlying issue is that LLVM does not silently ignore DllImport, while rustc assumes that it is safe to set DllImport even when linking statically. One possible solution would be to never set DllImport if the target uses a static relocation model. This should fix the issue, but... I don't have enough insight into the LLVM codegen right now to evaluate whether this is the correct approach. I will try to file a PR and then we can continue discussion there.

dvdhrm added a commit to dvdhrm/rust that referenced this issue Nov 21, 2022
Prevent DllImport from being attached to DSOLocal definitions in the
LLVM IR. The combination makes no sense, since definitions local to the
compilation unit will never be imported from external objects.

Additionally, LLVM will refuse the IR if it encounters the
combination (introduced in [1]):

  if (GV.hasDLLImportStorageClass())
    Assert(!GV.isDSOLocal(),
           "GlobalValue with DLLImport Storage is dso_local!", &GV);

Right now, codegen-llvm will only apply DllImport to constants and rely
on call-stubs for functions. Hence, we simply extend the codegen of
constants to skip DllImport for any local definitions.

This was discovered when switching the EFI targets to the static
relocation model [2]. With this fixed, we can start another attempt at
this.

[1] https://smlnj-gitlab.cs.uchicago.edu/manticore/llvm/commit/509132b368efed10bbdad825403f45e9cf1d6e38
[2] rust-lang#101656
Manishearth added a commit to Manishearth/rust that referenced this issue Nov 23, 2022
codegen-llvm: never combine DSOLocal and DllImport

Prevent DllImport from being attached to DSOLocal definitions in the LLVM IR. The combination makes no sense, since definitions local to the compilation unit will never be imported from external objects.

Additionally, LLVM will refuse the IR if it encounters the combination (introduced in [1]):

```
  if (GV.hasDLLImportStorageClass())
    Assert(!GV.isDSOLocal(),
           "GlobalValue with DLLImport Storage is dso_local!", &GV);
```

Right now, codegen-llvm will only apply DllImport to constants and rely on call-stubs for functions. Hence, we simply extend the codegen of constants to skip DllImport for any local definitions.

This was discovered when switching the EFI targets to the static relocation model [2]. With this fixed, we can start another attempt at this.

[1] https://smlnj-gitlab.cs.uchicago.edu/manticore/llvm/commit/509132b368efed10bbdad825403f45e9cf1d6e38
[2] rust-lang#101656
Manishearth added a commit to Manishearth/rust that referenced this issue Nov 23, 2022
codegen-llvm: never combine DSOLocal and DllImport

Prevent DllImport from being attached to DSOLocal definitions in the LLVM IR. The combination makes no sense, since definitions local to the compilation unit will never be imported from external objects.

Additionally, LLVM will refuse the IR if it encounters the combination (introduced in [1]):

```
  if (GV.hasDLLImportStorageClass())
    Assert(!GV.isDSOLocal(),
           "GlobalValue with DLLImport Storage is dso_local!", &GV);
```

Right now, codegen-llvm will only apply DllImport to constants and rely on call-stubs for functions. Hence, we simply extend the codegen of constants to skip DllImport for any local definitions.

This was discovered when switching the EFI targets to the static relocation model [2]. With this fixed, we can start another attempt at this.

[1] https://smlnj-gitlab.cs.uchicago.edu/manticore/llvm/commit/509132b368efed10bbdad825403f45e9cf1d6e38
[2] rust-lang#101656
dvdhrm added a commit to dvdhrm/rust that referenced this issue Nov 29, 2022
Prevent DllImport from being attached to DSOLocal definitions in the
LLVM IR. The combination makes no sense, since definitions local to the
compilation unit will never be imported from external objects.

Additionally, LLVM will refuse the IR if it encounters the
combination (introduced in [1]):

  if (GV.hasDLLImportStorageClass())
    Assert(!GV.isDSOLocal(),
           "GlobalValue with DLLImport Storage is dso_local!", &GV);

Right now, codegen-llvm will only apply DllImport to constants and rely
on call-stubs for functions. Hence, we simply extend the codegen of
constants to skip DllImport for any local definitions.

This was discovered when switching the EFI targets to the static
relocation model [2]. With this fixed, we can start another attempt at
this.

[1] https://smlnj-gitlab.cs.uchicago.edu/manticore/llvm/commit/509132b368efed10bbdad825403f45e9cf1d6e38
[2] rust-lang#101656
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 30, 2022
codegen-llvm: never combine DSOLocal and DllImport

Prevent DllImport from being attached to DSOLocal definitions in the LLVM IR. The combination makes no sense, since definitions local to the compilation unit will never be imported from external objects.

Additionally, LLVM will refuse the IR if it encounters the combination (introduced in [1]):

```
  if (GV.hasDLLImportStorageClass())
    Assert(!GV.isDSOLocal(),
           "GlobalValue with DLLImport Storage is dso_local!", &GV);
```

Right now, codegen-llvm will only apply DllImport to constants and rely on call-stubs for functions. Hence, we simply extend the codegen of constants to skip DllImport for any local definitions.

This was discovered when switching the EFI targets to the static relocation model [2]. With this fixed, we can start another attempt at this.

[1] https://smlnj-gitlab.cs.uchicago.edu/manticore/llvm/commit/509132b368efed10bbdad825403f45e9cf1d6e38
[2] rust-lang#101656
@dvdhrm
Copy link
Contributor

dvdhrm commented Nov 30, 2022

This is now fixed with a0771bd (issue #104679). @nicholasbishop you want to close this?

@nicholasbishop
Copy link
Contributor Author

Closing, thanks for fixing that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries O-UEFI UEFI T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants