Skip to content

Test that __rust_probestack can be linked - #74

Closed
adamrk wants to merge 4 commits into
Rust-for-Linux:rust-rustc-kbuildfrom
adamrk:export-compiler-builtins
Closed

Test that __rust_probestack can be linked#74
adamrk wants to merge 4 commits into
Rust-for-Linux:rust-rustc-kbuildfrom
adamrk:export-compiler-builtins

Conversation

@adamrk

@adamrk adamrk commented Jan 20, 2021

Copy link
Copy Markdown

Addresses #73.

We need to export the symbols from compiler_builtins so that modules can reference __rust_probestack.

@adamrk
adamrk force-pushed the export-compiler-builtins branch from 7b76fde to 5ec3ec0 Compare January 20, 2021 08:30
@adamrk
adamrk force-pushed the export-compiler-builtins branch from 5ec3ec0 to 2485c24 Compare January 20, 2021 08:33
@ojeda

ojeda commented Jan 20, 2021

Copy link
Copy Markdown
Member

Thanks a lot for this -- I saw #61 but didn't have time to look into it yet.

ojeda
ojeda reviewed Jan 20, 2021
View reviewed changes
Comment thread drivers/char/rust_example_3.rs Outdated
println!("[3] my_bool: {}", my_bool.read());
println!("[3] my_i32: {}", my_i32.read());

// Including this large variable on the stack will trigger a call to

@ojeda ojeda Jan 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! The 2/3/4 examples are intended to be copy-pastes of the first one (for the CI). Can you please put this into all of them?

In the future we will probably move these runtime tests somewhere else, but for now it is good to have them here. +1

@adamrk adamrk Jan 20, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@adamrk

adamrk commented Jan 20, 2021

Copy link
Copy Markdown
Author

This doesn't fix #61 by the way.

ojeda
ojeda requested changes Jan 20, 2021
View reviewed changes
Comment thread rust/Makefile Outdated
Comment on lines +6 to +8
extra-$(CONFIG_RUST) += exports_core_generated.h exports_alloc_generated.h
extra-$(CONFIG_RUST) += exports_kernel_generated.h
extra-$(CONFIG_RUST) += exports_compiler_builtins_generated.h

@ojeda ojeda Jan 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Suggested change
extra-$(CONFIG_RUST) += exports_core_generated.h exports_alloc_generated.h
extra-$(CONFIG_RUST) += exports_kernel_generated.h
extra-$(CONFIG_RUST) += exports_compiler_builtins_generated.h
extra-$(CONFIG_RUST) += exports_core_generated.h
extra-$(CONFIG_RUST) += exports_compiler_builtins_generated.h
extra-$(CONFIG_RUST) += exports_alloc_generated.h
extra-$(CONFIG_RUST) += exports_kernel_generated.h

Comment thread rust/Makefile Outdated
Comment on lines +66 to +69
$(objtree)/rust/exports_compiler_builtins_generated.h: exports_target_type := _RUST
$(objtree)/rust/exports_compiler_builtins_generated.h: $(objtree)/rust/compiler_builtins.o FORCE
$(call if_changed,exports)

@ojeda ojeda Jan 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please move these in-between core and alloc? (I keep them like that due to the dependency chain).

Comment thread rust/exports.c Outdated
#include "exports_core_generated.h"
#include "exports_alloc_generated.h"
#include "exports_kernel_generated.h"
#include "exports_compiler_builtins_generated.h"

@ojeda ojeda Jan 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same please.

@ojeda

ojeda commented Jan 20, 2021

Copy link
Copy Markdown
Member

This doesn't fix #61 by the way.

Yeah, I just saw your comment there -- nevertheless, one less thing to sort out!

@bjorn3

bjorn3 commented Jan 20, 2021

Copy link
Copy Markdown
Member

I don't think all symbols in compiler builtins should be exported. Many should already be defined by the compiler-rt used by Linux. Only a few __rust_* functions are rust specific.

@adamrk

adamrk commented Jan 20, 2021

Copy link
Copy Markdown
Author

I don't think all symbols in compiler builtins should be exported. Many should already be defined by the compiler-rt used by Linux. Only a few __rust_* functions are rust specific.

It's only exporting the symbols with the prefix __rust_ or _R. Here's what's currently exported - are there symbols there that shouldn't be included?

@bjorn3

bjorn3 commented Jan 20, 2021

Copy link
Copy Markdown
Member

None of these symbols would cause conflicts, but the _R* symbols should only be used within compiler-builtins and the __rust_*128_* symbols are unused by the LLVM backend. Only my Cranelift backend uses them. (I think the shift functions aren't even used by my Cranelift backend, so they can be removed completely.) That leaves only __rust_probestack as needing an export.

@ojeda

ojeda commented Jan 20, 2021

Copy link
Copy Markdown
Member

It looks like we won't need the __rust_probestack, but nevertheless we should keep the large stack test.

@adamrk adamrk changed the title Export compiler builtins Test that __rust_probestack can be linked Jan 20, 2021
@adamrk

adamrk commented Jan 20, 2021

Copy link
Copy Markdown
Author

Changed this to just include the large stack test. Maybe we should temporarily set probe_stack: false in the x86 target or just include EXPORT_SYMBOL_RUST(__rust_probestack) in rust/exports.c so that things will build while we wait for rust-lang/rust#80838 to land?

bjorn3
bjorn3 reviewed Jan 20, 2021
View reviewed changes
Comment thread drivers/char/rust_example.rs
// `compiler_builtins::probestack::__rust_probestack` on x86_64.
// This will verify that we are able to link modules which call
// `__rust_probestack`.
let x: [u64; 1028] = [5; 1028];

@bjorn3 bjorn3 Jan 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let x: [u64; 1028] = [5; 1028];
let x: [u64; 1028] = core::hint::black_box([5; 1028]);

Requires #![feature(test)], but ensures that it isn't optimized out.

@ojeda ojeda closed this Jan 21, 2021
@ojeda
ojeda deleted the branch Rust-for-Linux:rust-rustc-kbuild January 21, 2021 17:36
@adamrk

ghost commented Jan 23, 2021

Copy link
Copy Markdown
Author

I thought we still wanted to include the test?

@ojeda

ghost commented Jan 23, 2021

Copy link
Copy Markdown
Member

Ah, GitHub closed this automatically because the PR was against a branch that was deleted (?!) -- it wasn't me doing it! :-)

And it does not allow me to reopen it since the branch is gone... Please rebase it against rust and open a new one.

@adamrk adamrk mentioned this pull request Jan 24, 2021
Merged
@adamrk

ghost commented Jan 24, 2021

Copy link
Copy Markdown
Author

Weird - I thought there was some way to change the target branch when that happens...

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

Reviewers

@ojeda ojeda ojeda requested changes
+1 more reviewer
@bjorn3 bjorn3 bjorn3 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adamrk @ojeda @bjorn3