codegen-llvm: never combine DSOLocal and DllImport #104679

dvdhrm · 2022-11-21T15:57:01Z

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] #101656

rustbot · 2022-11-21T15:57:08Z

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

petrochenkov · 2022-11-21T21:40:56Z

Could you add a src/test/codegen test case for this situation?
Static relocation mode can be enabled using // compile-flags even if it's not the default currently.
@rustbot author

petrochenkov · 2022-11-21T21:41:41Z

With this fixed, we can start another attempt at this.

Yeah, it would be nice to try reverting #101413 when this is merged.

dvdhrm · 2022-11-22T11:04:12Z

@petrochenkov

Could you add a src/test/codegen test case for this situation?

I now added static-relocation-mode.rs to the codegen tests. For now, it only tests dso_local linkage in the static relocation model.

I would like to also add foreign linkage to this test, but currently all declarations are marked dso_local when the static relocation model is used. Is that intentional? That is, why can't I write a non-relocatable application which still uses dynamically linked dependencies?

dvdhrm · 2022-11-22T11:11:31Z

@rustbot ready

petrochenkov · 2022-11-22T11:24:24Z

@dvdhrm

I would like to also add foreign linkage to this test, but currently all declarations are marked dso_local when the static relocation model is used. Is that intentional? That is, why can't I write a non-relocatable application which still uses dynamically linked dependencies?

Apparently yes, and it's a relatively recent change (#83592), but I didn't participate in that work and not sure whether the case you mention was considered.
cc @nagisa

src/test/codegen/static-relocation-model.rs

petrochenkov · 2022-11-22T11:30:48Z

@bors r+ rollup=iffy

bors · 2022-11-22T11:30:50Z

📌 Commit 46e0b02 has been approved by petrochenkov

It is now in the queue for this repository.

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 · 2022-11-24T00:22:08Z

@bors r-

#104791 (comment)

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

Add a codegen-test that verifies inter-crate linkage with the static relocation model. We expect all symbols that are part of a rust compilation to end up in the same DSO, thus we expect `dso_local` annotations.

dvdhrm · 2022-11-29T10:29:01Z

CI failed for MacOS, because there the dso_local-optimization is actually not as fine-grained, thus dso_local is not emitted for the test. I would expect other targets to have similar issues in the future, hence I decided to guard the test on x86_64-pc-windows-msvc. Sadly, I couldn't figure out how to run on windows and linux targets, but not anything else.

I accidentally also rebased on master, sorry for that! The only change from before is # only-x86_64-pc-windows-msvc in the test, and a rename from static-relocation-model.rs to static-relocation-model-msvc.rs.

@rustbot ready

petrochenkov · 2022-11-29T19:18:14Z

@bors r+

bors · 2022-11-29T19:18:16Z

📌 Commit 7b9e1a9 has been approved by petrochenkov

It is now in the queue for this repository.

mati865 · 2022-11-29T19:32:15Z

CI failed for MacOS, because there the dso_local-optimization is actually not as fine-grained, thus dso_local is not emitted for the test. I would expect other targets to have similar issues in the future, hence I decided to guard the test on x86_64-pc-windows-msvc. Sadly, I couldn't figure out how to run on windows and linux targets, but not anything else.

I accidentally also rebased on master, sorry for that! The only change from before is # only-x86_64-pc-windows-msvc in the test, and a rename from static-relocation-model.rs to static-relocation-model-msvc.rs.

@rustbot ready

How about // ignore-macos maybe combined with // ignore-ios?

bors · 2022-11-30T04:18:18Z

⌛ Testing commit 7b9e1a9 with merge 8de4b13...

dvdhrm · 2022-11-30T07:24:04Z

How about // ignore-macos maybe combined with // ignore-ios?

Yeah, I thought about this as well, but as I mentioned I expect other targets to possibly have the same shortcuts in the dso_local-handling. Those might not be part of CI now, but might get upgraded to tier-2 at any point. I then decided to go with the only- marker because the way the dso_local-handling in the codegen-module (and llvm, fwiw) is written follows a similar logic. So new targets would get the same behavior as macos, not the same as windows or elf.

Additionally, I would like to test for dllimport in there as well, and that will definitely require a msvc-specific test. So maybe the better approach is to copy the test into static-relocation-model-elf.rs if we want to test ELF as well?

bors · 2022-11-30T07:59:18Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 8de4b13 to master...

rust-timer · 2022-11-30T09:18:28Z

Finished benchmarking commit (8de4b13): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.6%	[-2.1%, -1.1%]	2
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

mati865 · 2022-11-30T18:59:36Z

Additionally, I would like to test for dllimport in there as well, and that will definitely require a msvc-specific test.

Seems more like Windows specific (so could be also tested on windows-gnu).

So maybe the better approach is to copy the test into static-relocation-model-elf.rs if we want to test ELF as well?

If you think it would be beneficial...

rustbot assigned petrochenkov Nov 21, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 21, 2022

bjorn3 added A-linkage Area: linking into static, shared libraries and binaries O-windows Operating system: Windows A-codegen Area: Code generation O-UEFI UEFI labels Nov 21, 2022

dvdhrm force-pushed the rw/dso branch from e917baa to a2da749 Compare November 21, 2022 16:05

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 22, 2022

petrochenkov reviewed Nov 22, 2022

View reviewed changes

src/test/codegen/static-relocation-model.rs Outdated Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 22, 2022

Manishearth mentioned this pull request Nov 23, 2022

Rollup of 6 pull requests #104790

Closed

Manishearth mentioned this pull request Nov 23, 2022

Rollup of 5 pull requests #104791

Closed

bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 24, 2022

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 24, 2022

dvdhrm added 2 commits November 29, 2022 10:57

test/codegen: test inter-crate linkage with static relocation

7b9e1a9

Add a codegen-test that verifies inter-crate linkage with the static relocation model. We expect all symbols that are part of a rust compilation to end up in the same DSO, thus we expect `dso_local` annotations.

dvdhrm force-pushed the rw/dso branch from 46e0b02 to 7b9e1a9 Compare November 29, 2022 10:22

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 29, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 29, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 30, 2022

bors merged commit 8de4b13 into rust-lang:master Nov 30, 2022

rustbot added this to the 1.67.0 milestone Nov 30, 2022

dvdhrm mentioned this pull request Nov 30, 2022

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

codegen-llvm: never combine DSOLocal and DllImport #104679

codegen-llvm: never combine DSOLocal and DllImport #104679

dvdhrm commented Nov 21, 2022 •

edited

Loading

rustbot commented Nov 21, 2022

petrochenkov commented Nov 21, 2022

petrochenkov commented Nov 21, 2022

dvdhrm commented Nov 22, 2022

dvdhrm commented Nov 22, 2022

petrochenkov commented Nov 22, 2022

petrochenkov commented Nov 22, 2022

bors commented Nov 22, 2022

Manishearth commented Nov 24, 2022

dvdhrm commented Nov 29, 2022

petrochenkov commented Nov 29, 2022

bors commented Nov 29, 2022

mati865 commented Nov 29, 2022

bors commented Nov 30, 2022

dvdhrm commented Nov 30, 2022

bors commented Nov 30, 2022

rust-timer commented Nov 30, 2022

mati865 commented Nov 30, 2022

codegen-llvm: never combine DSOLocal and DllImport #104679

codegen-llvm: never combine DSOLocal and DllImport #104679

Conversation

dvdhrm commented Nov 21, 2022 • edited Loading

rustbot commented Nov 21, 2022

petrochenkov commented Nov 21, 2022

petrochenkov commented Nov 21, 2022

dvdhrm commented Nov 22, 2022

dvdhrm commented Nov 22, 2022

petrochenkov commented Nov 22, 2022

petrochenkov commented Nov 22, 2022

bors commented Nov 22, 2022

Manishearth commented Nov 24, 2022

dvdhrm commented Nov 29, 2022

petrochenkov commented Nov 29, 2022

bors commented Nov 29, 2022

mati865 commented Nov 29, 2022

bors commented Nov 30, 2022

dvdhrm commented Nov 30, 2022

bors commented Nov 30, 2022

rust-timer commented Nov 30, 2022

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

mati865 commented Nov 30, 2022

dvdhrm commented Nov 21, 2022 •

edited

Loading