Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] Native library linking changes for crater testing #102832

Closed
wants to merge 3 commits into from
Closed

[WIP] Native library linking changes for crater testing #102832

wants to merge 3 commits into from

Conversation

petrochenkov
Copy link
Contributor

  • Enable -Zpacked-bundled-libs by default
  • Preserve order of -l options from upstream crates, including dynamic libraries

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 9, 2022
@rust-highfive
Copy link
Collaborator

r? @estebank

(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 Oct 9, 2022
@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 9, 2022
@rust-log-analyzer

This comment was marked as resolved.

Sign in to view
@rust-log-analyzer

This comment was marked as resolved.

Sign in to view
@petrochenkov
Copy link
Contributor Author

I cannot reproduce the failure locally, maybe dist build will succeed.
@bors try

@bors
Copy link
Contributor

bors commented Oct 9, 2022

⌛ Trying commit 4579af7dd31c08a717685fbc2f5d1c8d646764ef with merge 2e96086b2dc45bc1b58a5c3be7a391950c90cfdf...

@bors

This comment was marked as resolved.

Sign in to view
@bors 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-crater Status: Waiting on a crater run to be completed. labels Oct 9, 2022
@rust-log-analyzer

This comment was marked as resolved.

Sign in to view
@petrochenkov
[WIP] Native library linking changes for crater testing
3e3c23b 
- Enable `-Zpacked-bundled-libs` by default
- Preserve order of `-l` options from upstream crates, including dynamic libraries
@petrochenkov
Copy link
Contributor Author

cc @belovdv

@petrochenkov
Copy link
Contributor Author

The issue reproduces only when

[build]
optimized-compiler-builtins = true

is enabled in config.toml, like on CI.

@petrochenkov
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Oct 11, 2022

⌛ Trying commit c44fe8863a6c1871ca5648601357e50e58e376ca with merge b1aeaf7c92f3eda15a80768bcb9977eb3a2fa8a4...

@bors bors mentioned this pull request Oct 11, 2022
Merged
@bors

This comment was marked as resolved.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Oct 11, 2022

⌛ Trying commit 4504add with merge ba15852773e3f50ad6384f12a3bff8d8b9b7e087...

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 13, 2022
@petrochenkov petrochenkov 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 Oct 13, 2022
@petrochenkov
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Oct 14, 2022

⌛ Trying commit 1ec9629 with merge 0c94de7c1a8f4b82b92d562847c35f513f520c7a...

@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 14, 2022
@bors
Copy link
Contributor

bors commented Oct 14, 2022

☀️ Try build successful - checks-actions
Build commit: 0c94de7c1a8f4b82b92d562847c35f513f520c7a (0c94de7c1a8f4b82b92d562847c35f513f520c7a)

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-13 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
........................................................................................ 8008/13670
........................................................................................ 8096/13670
........................................................................................ 8184/13670
......ii...............i......i..ii..................................................... 8272/13670
..................................F...F................................................. 8360/13670
........................................................................................ 8536/13670
........................................................................................ 8624/13670
.......................................................i..ii............................ 8712/13670
..................................ii.................................................... 8800/13670
---
---- [ui] src/test/ui/native-library-link-flags/mix-bundle-and-whole-archive-link-attr.rs stdout ----

error: test compilation failed although it shouldn't!
status: exit status: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/native-library-link-flags/mix-bundle-and-whole-archive-link-attr.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Cstrip=debuginfo" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/native-library-link-flags/mix-bundle-and-whole-archive-link-attr" "-A" "unused" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--crate-type" "rlib" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/native-library-link-flags/mix-bundle-and-whole-archive-link-attr/auxiliary"
stdout: none
--- stderr -------------------------------
error: the linking modifiers `+bundle` and `+whole-archive` are not compatible with each other when generating rlibs
error: aborting due to previous error
------------------------------------------



---- [ui] src/test/ui/native-library-link-flags/mix-bundle-and-whole-archive.rs stdout ----
Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu

error: test compilation failed although it shouldn't!
status: exit status: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/native-library-link-flags/mix-bundle-and-whole-archive.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Cstrip=debuginfo" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/native-library-link-flags/mix-bundle-and-whole-archive" "-A" "unused" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--crate-type" "rlib" "-l" "static:+bundle,+whole-archive=mylib" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/native-library-link-flags/mix-bundle-and-whole-archive/auxiliary"
stdout: none
--- stderr -------------------------------
error: the linking modifiers `+bundle` and `+whole-archive` are not compatible with each other when generating rlibs
error: aborting due to previous error
------------------------------------------

@petrochenkov
Copy link
Contributor Author

@craterbot run mode=build-only p=1 crates=https://crater-reports.s3.amazonaws.com/pr-102832/retry-regressed-list.txt

@craterbot
Copy link
Collaborator

👌 Experiment pr-102832-1 created and queued.
🤖 Automatically detected try build 0c94de7c1a8f4b82b92d562847c35f513f520c7a
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🚧 Experiment pr-102832-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-102832-1 is completed!
📊 31 regressed and 1 fixed (490 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 15, 2022
@petrochenkov petrochenkov 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 Oct 15, 2022
@petrochenkov
Copy link
Contributor Author

Non-spurious regressions from changing the linking order of dynamic libraries (3 root regressions and their reasons).

  • llvm-sys doesn't declare dependency on libffi

    • 3c1u.bf-rs.cdce5dc2032db2a86d85de99a596e31c13f02c0d
    • EdwardGarmon.Crabby_Compiler.6bacab2b797d9e679979fb137efa052d8670eb92
    • Equilibris.Stack_base_langauge.3c6ee0dbdca892fc9c7791d0f4ed721561d4caf8
    • Hacker-007.EnviousProgrammingLanguage.72581b1158685a1ec6b1544e548349a6ba60246b
    • JoNil.chibi-rs.ee845fe3800425d345728328ed90fea36196c5af
    • MysteryJump.tapl-rust.1899d78b048b5b4b5dd94ca296609b8d1d99fc3a
    • TheDan64.limonite.b4ffbf950b22279c1d633c5c9f63dad05447fe4c
    • antonholmberg.kaleidoscope-compiler.611216055decf8715c02ba97b63746b9f2e5210a
    • ddadaal.kaleidoscope-rust.1d7cb57061315c8ad6424fcb360b5781729226c1
    • fangerm.gelixrs.b2c5bd4cecb5d11346030f0a2d3a4fd62daa5968
    • faraazahmad.tisp.3ff5af0271654f53f23fe31aabf4b88b57e81842
    • hayeb.loz.7dd5b38c6578388ae0620225f73b0e00fcac9cd7
    • maekawatoshiki.rucc.0ab8dc404e5ec3849e43274049c2fb932657ff49
    • mingyli.mrust.c87e65902a4c4e88240728bd825d667ac7ac96cc
    • mrLSD.llvm-sample.e3677474457a1548d53598c7c8b11c120d0beb0f
    • peter-fogg.huck.1104d593c6d09a7b9ec4e6028466fe7b9f65913d
    • phodal.llvm-rust-helloworld.cd4c8407e59a14fbb070bf1cc4b7edf3f300d879
    • reese.rox.d5570e71d1b1b51c7b55738daf742272c48042ac
    • tanin47.lilit.cb20b46c045ca64e99fd3e7edf4ce2d81a8d48b5
    • xiaoxigua-1.ZX.1f3d818fa75368156dd4ae490b085b9c28be3ce0

  • lapacke-sys doesn't declare dependency on liblapacke

    • lopez86.rust-mlearn.71198456a7207590b9c519b552464feab0f57d1f

  • libmimalloc-sys doesn't declare dependency on libc

    • mimalloc-0.1.30
    • mimalloc2-0.1.0

I think this is a change that we can make.

@petrochenkov
Copy link
Contributor Author

As for -Zpacked-bundled-libs there are regressions that are less trivial.
For example, libbz2 expect its users to define bz_internal_error for it (reverse dependency) and bzip2-sys crate does that.
So object files from bzip2-sys crate and libbz2 bundled native library should all reside in the same static library otherwise bz_internal_error may be dropped (libbz2 requiring bz_internal_error is not declared in any way).
We probably need some new language attribute for declaring this kind of reverse dependencies explicitly.

In the meantime, I think implicitly enabling -Zpacked-bundled-libs for the cases where it's really necessary (#99429 - combining bundle and whole-archive, combining bundle and link-time cfg) would be the best course of action.

@petrochenkov
Copy link
Contributor Author

Closing the PR, I'll later submit different parts in different PRs.

@petrochenkov petrochenkov mentioned this pull request Oct 22, 2022
Open
This was referenced Nov 11, 2022
Merged
Closed
Open
Closed
@petrochenkov petrochenkov mentioned this pull request Feb 15, 2023
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@estebank estebank

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@petrochenkov @rust-highfive @rust-log-analyzer @bors @craterbot @estebank @rustbot