Skip to content

Conversation

dpaoliello
Copy link
Contributor

All existing tests of link kind raw-dylib only validate the ability to link against functions, but it is also possible to link against variables.

This adds tests for linking against a variable using raw-dylib both by-name and by-ordinal.

@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2022
@dpaoliello
Copy link
Contributor Author

r? @michaelwoerister

@chenyukang
Copy link
Member

I have a question,

#![feature(raw_dylib)]

#[link(name = "exporter", kind = "raw-dylib")]
extern {
    #[link_ordinal(1, 2)]
    fn imported_function();
}

fn main() {}

Will trigger an error:

error: incorrect number of arguments to `#[link_ordinal]`
 --> src/test/ui/rfc-2627-raw-dylib/link-ordinal-not-foreign-fn.rs:5:5
  |
5 |     #[link_ordinal(1, 2)]
  |     ^^^^^^^^^^^^^^^^^^^^^
  |
  = note: the attribute requires exactly one argument

error: aborting due to 2 previous errors; 1 warning emitted

But when it's used for a static variable, it won't tigger error:

#![feature(raw_dylib)]

#[link(name = "exporter", kind = "raw-dylib")]
extern {
    #[link_ordinal(1, 2)]
    static mut imported_variable: i32;
}

fn main() {}

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 4, 2022
@dpaoliello
Copy link
Contributor Author

But when it's used for a static variable, it won't tigger error:

Well spotted - looks like there is a bug where we aren't checking attributes on ForeignStatic items at all.

@michaelwoerister
Copy link
Member

But when it's used for a static variable, it won't tigger error:

Well spotted - looks like there is a bug where we aren't checking attributes on ForeignStatic items at all.

I assume this is the fix for that?

Thanks a lot, @dpaoliello!
@bors r+

@bors
Copy link
Collaborator

bors commented Aug 5, 2022

📌 Commit 0a754b3 has been approved by michaelwoerister

It is now in the queue for this repository.

@bors 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 5, 2022
@bors
Copy link
Collaborator

bors commented Aug 5, 2022

⌛ Testing commit 0a754b3 with merge d77da9d...

@bors
Copy link
Collaborator

bors commented Aug 5, 2022

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing d77da9d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 5, 2022
@bors bors merged commit d77da9d into rust-lang:master Aug 5, 2022
@rustbot rustbot added this to the 1.64.0 milestone Aug 5, 2022
@dpaoliello
Copy link
Contributor Author

But when it's used for a static variable, it won't tigger error:

Well spotted - looks like there is a bug where we aren't checking attributes on ForeignStatic items at all.

I assume this is the fix for that?

Correct

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d77da9d): comparison url.

Instruction count

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

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
2.4% 3.1% 32
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
2.8% 2.9% 2
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-4.6% -4.6% 1
All 😿🎉 (primary) N/A N/A 0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@dpaoliello dpaoliello deleted the externvar branch August 15, 2022 17:09
@ehuss ehuss modified the milestones: 1.64.0, 1.65.0 Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants