Skip to content

Support enable/disable sanitizers/profiler per target#78354

Merged
bors merged 1 commit intorust-lang:masterfrom
12101111:rustbuild_profiler
Nov 12, 2020
Merged

Support enable/disable sanitizers/profiler per target#78354
bors merged 1 commit intorust-lang:masterfrom
12101111:rustbuild_profiler

Conversation

@12101111
Copy link
Contributor

@12101111 12101111 commented Oct 25, 2020

This PR add options under [target.*] of config.toml which can enable or disable sanitizers/profiler runtime for corresponding target.
If these options are empty, the global options under [build] will take effect.

Fix #78329

@rust-highfive
Copy link
Contributor

rust-highfive commented Oct 25, 2020

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 Oct 25, 2020
Mark-Simulacrum
Mark-Simulacrum reviewed Oct 26, 2020
View reviewed changes
src/bootstrap/lib.rs Outdated
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Oct 26, 2020

Choose a reason for hiding this comment

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

This is presumably meant to be profiler_enabled?

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Oct 26, 2020

Please also squash commits. I'll want to do another read through I think, but I suspect this is good to go modulo the nit.

@Mark-Simulacrum Mark-Simulacrum 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 26, 2020
@JohnCSimon
Copy link
Member

JohnCSimon commented Nov 9, 2020

ping from triage:
@12101111 is this PR ready for review?

@12101111
Copy link
Contributor Author

12101111 commented Nov 10, 2020

ping from triage:
@12101111 is this PR ready for review?

Yes

@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Nov 10, 2020

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Nov 10, 2020

📌 Commit b989d46 has been approved by Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Nov 10, 2020

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 10, 2020
This was referenced Nov 11, 2020
Closed
Closed
@jonas-schievink jonas-schievink mentioned this pull request Nov 11, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2020
@bors
Auto merge of rust-lang#78956 - jonas-schievink:rollup-r53giob, r=jon…
77180db 
…as-schievink

Rollup of 11 pull requests

Successful merges:

 - rust-lang#78216 (Duration::zero() -> Duration::ZERO)
 - rust-lang#78354 (Support enable/disable sanitizers/profiler per target)
 - rust-lang#78417 (BTreeMap: split off most code of append)
 - rust-lang#78832 (look at assoc ct, check the type of nodes)
 - rust-lang#78873 (Add flags customizing behaviour of MIR inlining)
 - rust-lang#78899 (Support inlining diverging function calls)
 - rust-lang#78923 (Cleanup and comment intra-doc link pass)
 - rust-lang#78929 (rustc_target: Move target env "gnu" from `linux_base` to `linux_gnu_base`)
 - rust-lang#78930 (rustc_taret: Remove `TargetOptions::is_like_android`)
 - rust-lang#78942 (Fix typo in comment)
 - rust-lang#78947 (Ship llvm-cov through llvm-tools)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 194b968 into rust-lang:master Nov 12, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 12, 2020
@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Nov 17, 2020
@richkadel
Copy link

richkadel commented Nov 17, 2020

PR #78354 broke the build for users of profiler_builtins.

The PR copies the (commented out) #profiler = false line (among other things) from the [build] section to the sample target-specific (linux in config.toml.example) section, and made related changes to bootstrap.

Intuitively (especially because the [build] profiler = false/true option is still present, I believe that if profiler = true in build, then profiler should also be true in the target-specific setting, if not explicitly set for the target.

Since I always build with profiler = true, I did not expect the build to stop working after rebasing with the change, but with this PR, the profiler_builtins library is no longer built.

I think the new target-specific settings, if not set, need to default to the non-specific setting.

Workaround:

I had to add a target-specific setting on each platform I test with (Linux, Windows, and MacOS). On Mac, for example, I added these two lines in my config.toml:

[target.x86_64-apple-darwin]
profiler = true

It's still building, but it looks like it finally started building the profiler_builtins library.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Nov 17, 2020

Yep, I agree that is the expected behavior. Would be happy to accept a PR fixing this, or if you can file an issue (just copying content in your note here) I can write a patch some time this week.

@richkadel
Copy link

richkadel commented Nov 17, 2020

I'll file an issue. I don't have time to work on it this week unfortunately.

Thanks Mark!

@richkadel richkadel mentioned this pull request Nov 17, 2020
Closed
@12101111 12101111 mentioned this pull request Nov 18, 2020
Merged
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 19, 2020
@Dylan-DPC
Rollup merge of rust-lang#79155 - 12101111:fix-profiler-config, r=Mar…
3b857a5 
…k-Simulacrum

fix handling the default config for profiler and sanitizers

rust-lang#78354 don't handle the case that user don't add any target-specific config in `[target.*]` of `config.toml`:
```toml
changelog-seen = 2
[llvm]
link-shared = true
[build]
sanitizers = true
profiler = true
[install]
[rust]
[dist]
```

The previes code handle the default config in `Config::prase()`:

```rust
target.sanitizers = cfg.sanitizers.unwrap_or(build.sanitizers.unwrap_or_default());
target.profiler = cfg.profiler.unwrap_or(build.profiler.unwrap_or_default());
config.target_config.insert(TargetSelection::from_user(&triple), target);
```

In this case, `toml.target` don't contain any target, so the above code won't execute. Instead, a default `Target` is insert in https://github.com/rust-lang/rust/blob/c919f490bbcd2b29b74016101f7ec71aaa24bdbb/src/bootstrap/sanity.rs#L162-L166

The default value for `bool` is false, hence the issue in rust-lang#79124

This fix change the type of `sanitizers` and `profiler` to `Option<bool>`, so the default value is `None`, and fallback config is handled in  `Config::sanitizers_enabled` and `Config::profiler_enabled`

fix rust-lang#79124

cc `@Mark-Simulacrum` `@richkadel`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Projects

None yet

Milestone

1.49.0

Development

Successfully merging this pull request may close these issues.

Enable profiler=true in config.toml and target = [wasm32-unknown-unknown] will cause bootstrap fail

9 participants

@12101111 @rust-highfive @Mark-Simulacrum @JohnCSimon @Dylan-DPC-zz @bors @richkadel @jyn514 @rustbot