add test for symbol visibility of `#[naked]` functions by folkertdev · Pull Request #128362 · rust-lang/rust

folkertdev · 2024-07-29T20:02:57Z

tracking issue: #90957

This test is extracted from #128004

That PR attempts to generated naked functions as an extern function declaration, combined with a global asm block that provides the implementation for that declaration.

In order to link declaration and definition together, some flavor of external linking must be used: LLVM will error for other linkage types. Specifically the allowed options are #[linkage = "external"] and #[linkage = "extern_weak"]. That is kind of an implementation detail though: to the user, a naked function should just behave like a normal function.

Hence it should be visible to the linker under the same circumstances as a normal, vanilla function and have the same attributes (Weak, External). Getting this behavior right will require some care, so I think it's a good idea to lock it in now, before making any changes, to make sure we don't regress.

Are there any interesting cases that I missed here? E.g. is checking on different architectures worth it? I don't think the other binary types (rlib etc) are relevant here, but may be missing something.

r? @bjorn3

rustbot · 2024-07-29T20:03:06Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

tgross35 · 2024-07-29T21:30:49Z

Do we already have a test for global? If not, it might be worth adding one.

folkertdev · 2024-07-30T08:04:51Z

Do we already have a test for global? If not, it might be worth adding one.

global what? I don't understand what you're suggesting here.

tgross35 · 2024-07-30T08:15:56Z

I was suggesting that you might as well add a check for global_asm! in this new rmake test, since it doesn't look like we have something for that currently. I.e. something that tests the current behavior of #128071.

folkertdev · 2024-08-03T16:11:12Z

now updated to match the style of #128607 (the aim there is to not use llvm CLI tools, which is a good idea)

bjorn3 · 2024-08-03T19:26:10Z

@bors r+

bors · 2024-08-03T19:26:12Z

📌 Commit 940a109 has been approved by bjorn3

It is now in the queue for this repository.

…isibility, r=bjorn3 add test for symbol visibility of `#[naked]` functions tracking issue: rust-lang#90957 This test is extracted from rust-lang#128004 That PR attempts to generated naked functions as an extern function declaration, combined with a global asm block that provides the implementation for that declaration. In order to link declaration and definition together, some flavor of external linking must be used: LLVM will error for other linkage types. Specifically the allowed options are `#[linkage = "external"]` and `#[linkage = "extern_weak"]`. That is kind of an implementation detail though: to the user, a naked function should just behave like a normal function. Hence it should be visible to the linker under the same circumstances as a normal, vanilla function and have the same attributes (Weak, External). Getting this behavior right will require some care, so I think it's a good idea to lock it in now, before making any changes, to make sure we don't regress. Are there any interesting cases that I missed here? E.g. is checking on different architectures worth it? I don't think the other binary types (rlib etc) are relevant here, but may be missing something. r? `@bjorn3`

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#128305 (improve error message when `global_asm!` uses `asm!` operands) - rust-lang#128362 (add test for symbol visibility of `#[naked]` functions) - rust-lang#128526 (time.rs: remove "Basic usage text") - rust-lang#128531 (Miri: add a flag to do recursive validity checking) - rust-lang#128589 (allow setting `link-shared` and `static-libstdcpp` with CI LLVM) - rust-lang#128615 (rustdoc: make the hover trail for doc anchors a bit bigger) - rust-lang#128620 (Update rinja version to 0.3.0) r? `@ghost` `@rustbot` modify labels: rollup

…isibility, r=bjorn3 add test for symbol visibility of `#[naked]` functions tracking issue: rust-lang#90957 This test is extracted from rust-lang#128004 That PR attempts to generated naked functions as an extern function declaration, combined with a global asm block that provides the implementation for that declaration. In order to link declaration and definition together, some flavor of external linking must be used: LLVM will error for other linkage types. Specifically the allowed options are `#[linkage = "external"]` and `#[linkage = "extern_weak"]`. That is kind of an implementation detail though: to the user, a naked function should just behave like a normal function. Hence it should be visible to the linker under the same circumstances as a normal, vanilla function and have the same attributes (Weak, External). Getting this behavior right will require some care, so I think it's a good idea to lock it in now, before making any changes, to make sure we don't regress. Are there any interesting cases that I missed here? E.g. is checking on different architectures worth it? I don't think the other binary types (rlib etc) are relevant here, but may be missing something. r? ``@bjorn3``

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#128362 (add test for symbol visibility of `#[naked]` functions) - rust-lang#128526 (time.rs: remove "Basic usage text") - rust-lang#128531 (Miri: add a flag to do recursive validity checking) - rust-lang#128578 (rustdoc: Cleanup `CacheBuilder` code for building search index) - rust-lang#128589 (allow setting `link-shared` and `static-libstdcpp` with CI LLVM) - rust-lang#128615 (rustdoc: make the hover trail for doc anchors a bit bigger) - rust-lang#128620 (Update rinja version to 0.3.0) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2024-08-04T09:31:59Z

@bors r- .. ^^

tests/run-make/naked-symbol-visibility/rmake.rs

folkertdev · 2024-08-04T09:41:59Z

that typo has now been resolved (and the commits squashed)

@rustbot ready (we could try a try action though?)

bjorn3 · 2024-08-04T10:05:16Z

I don't think a try build is necessary for now. If it fails again, a try build may be worthwhile.

@bors r+

folkertdev · 2024-08-05T07:35:03Z

so, not even #[no_mangle] pub symbols are actually in the dynamic symbol table of a cdylib with msvc???

--- stderr -------------------------------
thread 'main' panicked at C:\a\rust\rust\tests\run-make\naked-symbol-visibility\rmake.rs:18:5:
thread 'main' panicked at C:\a\rust\rust\tests\run-make\naked-symbol-visibility\rmake.rs:18:5:
symbol public_vanilla occurs 0 times

@ChrisDenton I don't think this does much different to the tests/run-make/symbol-visibility/rmake.rs test, except that that uses exports and not dynamic_symbols that I need here to get some extra data on the symbols. Does dynamic_symbols just not work with msvc? it seems to do fine with x86_64-pc-windows-gnu (which I guess runs under wine on my linux machine)

ChrisDenton · 2024-08-05T14:07:55Z

According to Object::dynamic_symbols.

Only ELF has dynamic linking symbols. Other file formats will return an empty iterator. Consider using Self::exports or Self::imports instead.

I'm not sure why it would work with windows-gnu.

folkertdev · 2024-08-05T14:19:38Z

hmm okay, weird. even based on the source that really should not work on any windows target.

I guess I can filter symbols() by whether they occur in exports() and see if that works out.

jieyouxu · 2024-08-05T15:37:54Z

@bors try

…ibility, r=<try> add test for symbol visibility of `#[naked]` functions tracking issue: rust-lang#90957 This test is extracted from rust-lang#128004 That PR attempts to generated naked functions as an extern function declaration, combined with a global asm block that provides the implementation for that declaration. In order to link declaration and definition together, some flavor of external linking must be used: LLVM will error for other linkage types. Specifically the allowed options are `#[linkage = "external"]` and `#[linkage = "extern_weak"]`. That is kind of an implementation detail though: to the user, a naked function should just behave like a normal function. Hence it should be visible to the linker under the same circumstances as a normal, vanilla function and have the same attributes (Weak, External). Getting this behavior right will require some care, so I think it's a good idea to lock it in now, before making any changes, to make sure we don't regress. Are there any interesting cases that I missed here? E.g. is checking on different architectures worth it? I don't think the other binary types (rlib etc) are relevant here, but may be missing something. r? `@bjorn3` try-job: x86_64-msvc try-job: i686-msvc try-job: i686-mingw try-job: x86_64-mingw

bors · 2024-08-05T15:39:06Z

⌛ Trying commit ee43eef with merge 0280c59...

bors · 2024-08-05T17:29:39Z

💔 Test failed - checks-actions

folkertdev · 2024-08-05T17:35:21Z

well:

thread 'main' panicked at C:\a\rust\rust\tests\run-make\naked-symbol-visibility\rmake.rs:18:5:
symbol public_vanilla occurs 0 times
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

so, I'd propose to skip msvc, maybe all of windows, for now?

folkertdev

I've added //@ only-linux, which combined with //@ only-x86_64

tests/run-make/naked-symbol-visibility/rmake.rs

bjorn3 · 2024-08-05T17:45:03Z

Can you use //@ ignore-windows instead? I don't see any reason why it shouldn't work on say FreeBSD or arm64.

bjorn3 · 2024-08-05T18:06:56Z

tests/run-make/naked-symbol-visibility/rmake.rs

@@ -0,0 +1,98 @@
+//@ ignore-windows
+//@ only-x86_64


Is the only-x86_64 really necessary?

yes, the inline/global assembly will break on aarch64 right?

bjorn3 · 2024-08-05T19:05:11Z

@bors r+

bors · 2024-08-05T19:05:13Z

📌 Commit 1967951 has been approved by bjorn3

It is now in the queue for this repository.

…isibility, r=bjorn3 add test for symbol visibility of `#[naked]` functions tracking issue: rust-lang#90957 This test is extracted from rust-lang#128004 That PR attempts to generated naked functions as an extern function declaration, combined with a global asm block that provides the implementation for that declaration. In order to link declaration and definition together, some flavor of external linking must be used: LLVM will error for other linkage types. Specifically the allowed options are `#[linkage = "external"]` and `#[linkage = "extern_weak"]`. That is kind of an implementation detail though: to the user, a naked function should just behave like a normal function. Hence it should be visible to the linker under the same circumstances as a normal, vanilla function and have the same attributes (Weak, External). Getting this behavior right will require some care, so I think it's a good idea to lock it in now, before making any changes, to make sure we don't regress. Are there any interesting cases that I missed here? E.g. is checking on different architectures worth it? I don't think the other binary types (rlib etc) are relevant here, but may be missing something. r? `@bjorn3`

Rollup of 7 pull requests Successful merges: - rust-lang#128107 (Migrate `raw-dylib-alt-calling-convention`, `raw-dylib-c` and `redundant-libs` `run-make` tests to rmake) - rust-lang#128362 (add test for symbol visibility of `#[naked]` functions) - rust-lang#128417 (Add `f16` and `f128` math functions) - rust-lang#128638 (run-make: enable msvc for `link-dedup`) - rust-lang#128647 (Enable msvc for link-args-order) - rust-lang#128649 (run-make: Enable msvc for `no-duplicate-libs` and `zero-extend-abi-param-passing`) - rust-lang#128766 (Trivial grammar fix in const keyword docs) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#128362 - folkertdev:naked-function-symbol-visibility, r=bjorn3 add test for symbol visibility of `#[naked]` functions tracking issue: rust-lang#90957 This test is extracted from rust-lang#128004 That PR attempts to generated naked functions as an extern function declaration, combined with a global asm block that provides the implementation for that declaration. In order to link declaration and definition together, some flavor of external linking must be used: LLVM will error for other linkage types. Specifically the allowed options are `#[linkage = "external"]` and `#[linkage = "extern_weak"]`. That is kind of an implementation detail though: to the user, a naked function should just behave like a normal function. Hence it should be visible to the linker under the same circumstances as a normal, vanilla function and have the same attributes (Weak, External). Getting this behavior right will require some care, so I think it's a good idea to lock it in now, before making any changes, to make sure we don't regress. Are there any interesting cases that I missed here? E.g. is checking on different architectures worth it? I don't think the other binary types (rlib etc) are relevant here, but may be missing something. r? ``@bjorn3``

rustbot assigned bjorn3 Jul 29, 2024

rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2024

folkertdev force-pushed the naked-function-symbol-visibility branch 2 times, most recently from 8040197 to 940a109 Compare August 3, 2024 16:09

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 Aug 3, 2024

matthiaskrgr mentioned this pull request Aug 4, 2024

Rollup of 7 pull requests #128624

Closed

matthiaskrgr mentioned this pull request Aug 4, 2024

Rollup of 7 pull requests #128629

Closed

folkertdev mentioned this pull request Aug 4, 2024

improve error message when global_asm! uses asm! operands #128305

Merged

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 4, 2024

folkertdev commented Aug 4, 2024

View reviewed changes

tests/run-make/naked-symbol-visibility/rmake.rs Outdated Show resolved Hide resolved

folkertdev force-pushed the naked-function-symbol-visibility branch from 940a109 to 5ee6701 Compare August 4, 2024 09:40

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 Aug 4, 2024

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

folkertdev force-pushed the naked-function-symbol-visibility branch from d7ddc3a to ee43eef Compare August 5, 2024 14:51

This comment has been minimized.

Sign in to view

folkertdev force-pushed the naked-function-symbol-visibility branch from ee43eef to 3462e45 Compare August 5, 2024 17:40

folkertdev commented Aug 5, 2024

View reviewed changes

tests/run-make/naked-symbol-visibility/rmake.rs Show resolved Hide resolved

add test for symbol visibility of #[naked] functions

1967951

folkertdev force-pushed the naked-function-symbol-visibility branch from 3462e45 to 1967951 Compare August 5, 2024 17:55

bjorn3 reviewed Aug 5, 2024

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 5, 2024

This was referenced Aug 7, 2024

Rollup of 9 pull requests #128767

Closed

Rollup of 7 pull requests #128768

Merged

bors merged commit 10a7f93 into rust-lang:master Aug 7, 2024

rustbot added this to the 1.82.0 milestone Aug 7, 2024

Uh oh!

Conversation

folkertdev commented Jul 29, 2024 • edited by bjorn3 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 29, 2024

Uh oh!

tgross35 commented Jul 29, 2024

Uh oh!

folkertdev commented Jul 30, 2024

Uh oh!

tgross35 commented Jul 30, 2024

Uh oh!

folkertdev commented Aug 3, 2024

Uh oh!

bjorn3 commented Aug 3, 2024

Uh oh!

bors commented Aug 3, 2024

Uh oh!

matthiaskrgr commented Aug 4, 2024

Uh oh!

Uh oh!

folkertdev commented Aug 4, 2024

Uh oh!

bjorn3 commented Aug 4, 2024

Uh oh!

folkertdev commented Aug 5, 2024

Uh oh!

ChrisDenton commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

folkertdev commented Aug 5, 2024

Uh oh!

jieyouxu commented Aug 5, 2024

Uh oh!

bors commented Aug 5, 2024

Uh oh!

This comment has been minimized.

bors commented Aug 5, 2024

Uh oh!

folkertdev commented Aug 5, 2024

Uh oh!

folkertdev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bjorn3 commented Aug 5, 2024

Uh oh!

bjorn3 Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

folkertdev Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjorn3 Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

bjorn3 commented Aug 5, 2024

Uh oh!

bors commented Aug 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

folkertdev commented Jul 29, 2024 •

edited by bjorn3

Loading

ChrisDenton commented Aug 5, 2024 •

edited

Loading

folkertdev Aug 5, 2024 •

edited

Loading