-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Make no_mangle on foreign items explicit instead of implicit #144678
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
Conversation
Some changes occurred in compiler/rustc_codegen_ssa |
Can you please squash the last two commits? |
This comment has been minimized.
This comment has been minimized.
Can do |
1790424
to
5240989
Compare
5240989
to
e75b93d
Compare
e75b93d
to
9003a3b
Compare
@@ -192,6 +192,10 @@ impl CodegenFnAttrs { | |||
/// * `#[export_name(...)]` is present | |||
/// * `#[linkage]` is present | |||
/// | |||
/// Note that this returns true for foreign items. | |||
/// However, in some places that care about `contains_extern_indicator`, foreign items | |||
/// (in an `extern` block) should explicitly be ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to make this not return true for foreign items? Foreign items don't need to be exported and the point of this method is to check if an item needs to be exported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's annoying since we don't have a did or TyCtxt here. We could, but that'd involve adding another flag in the codegen fn attributes. I decided against that but it's perfectly possible.
@bors r+ |
…jorn3 Make no_mangle on foreign items explicit instead of implicit for a followup PR I'm working on I need some foreign items to mangle. I could add a new attribute: `no_no_mangle` or something silly like that but by explicitly putting `no_mangle` in the codegen fn attrs of foreign items we can default it to `no_mangle` and then easily remove it when we don't want it. I guess you'd know about this r? `@bjorn3.` Shouldn't be too hard to review :) Builds on rust-lang#144655 which should merge first.
Rollup of 13 pull requests Successful merges: - #143857 (Port #[macro_export] to the new attribute parsing infrastructure) - #144070 (Implement `hash_map` macro ) - #144322 (Add lint against dangling pointers from local variables) - #144667 (`AlignmentEnum` should just be `repr(usize)` now) - #144678 (Make no_mangle on foreign items explicit instead of implicit) - #144790 (Multiple bounds checking elision failures) - #144794 (Port `#[coroutine]` to the new attribute system) - #144805 (compiletest: Preliminary cleanup of `ProcRes` printing/unwinding) - #144808 (`Interner` arg to `EarlyBinder` does not affect auto traits) - #144816 (Update E0562 to account for the new impl trait positions) - #144822 (Return a struct with named fields from `hash_owner_nodes`) - #144824 (Updated test links in compiler) - #144829 (Use full flag name in strip command for Darwin) r? `@ghost` `@rustbot` modify labels: rollup
Trying to diagnose rollup failure #144846 (comment) @bors try jobs=test-various |
Make no_mangle on foreign items explicit instead of implicit try-job: test-various
This comment has been minimized.
This comment has been minimized.
💔 Test failed (CI). Failed jobs:
|
@bors r- |
@bors2 try jobs=test-various |
Make no_mangle on foreign items explicit instead of implicit try-job: test-various
@bjorn3 that seems promising? |
5aa8fb1
to
7aa8707
Compare
@bors2 try jobs=test-various |
This comment has been minimized.
This comment has been minimized.
Make no_mangle on foreign items explicit instead of implicit try-job: test-various
@bjorn seems ok? |
@rustbot ready |
@bors r+ |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing d9dba3a (parent) -> 8e62bfd (this PR) Test differencesShow 4 test diffs4 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 8e62bfd311791bfd9dca886abdfbab07ec54d8b4 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (8e62bfd): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.3%, secondary 2.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 464.325s -> 465.671s (0.29%) |
sigh, ye that's significant I'm afraid :/ |
@@ -443,6 +443,27 @@ fn apply_overrides(tcx: TyCtxt<'_>, did: LocalDefId, codegen_fn_attrs: &mut Code | |||
if tcx.should_inherit_track_caller(did) { | |||
codegen_fn_attrs.flags |= CodegenFnAttrFlags::TRACK_CALLER; | |||
} | |||
|
|||
// Foreign items by default use no mangling for their symbol name. | |||
if tcx.is_foreign_item(did) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the extra query call here is the cause for the perf regression.
for a followup PR I'm working on I need some foreign items to mangle. I could add a new attribute:
no_no_mangle
or something silly like that but by explicitly puttingno_mangle
in the codegen fn attrs of foreign items we can default it tono_mangle
and then easily remove it when we don't want it.I guess you'd know about this r? @bjorn3. Shouldn't be too hard to review :)
Builds on #144655 which should merge first.