Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
+217 −167

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.

# to view
@rust-log-analyzer

This comment was marked as resolved.

# 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.

# 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.

# 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.

# to view
@rust-log-analyzer

This comment has been minimized.

# 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
# for free to join this conversation on GitHub. Already have an account? # 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