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.

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

Implement -C link-self-contained=(+/-)sanitizers #121420

Closed

Conversation

chriswailes
Copy link
Contributor

This PR adds support for enabling/disabling self-contained sanitizer runtimes.

This is an alternative to #121207.

r? @michaelwoerister

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 22, 2024
@petrochenkov petrochenkov self-assigned this Feb 22, 2024
@@ -1192,6 +1192,8 @@ fn add_sanitizer_libraries(sess: &Session, crate_type: CrateType, linker: &mut d
// are currently distributed as static libraries which should be linked to
// executables only.
let needs_runtime = !sess.target.is_like_android
&& (!sess.opts.cg.link_self_contained.is_sanitizers_disabled()
|| sess.opts.cg.link_self_contained.is_sanitizers_enabled())
Copy link
Member

Choose a reason for hiding this comment

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

This condition is getting a bit too unreadable, I think. Can we split it into multiple guard clauses, each with a separate comment?

if sess.target.is_like_android {
    // sanitizer runtime libraries are provided dynamically
    // on Android targets.
    return; 
}

if /* disabled via link_self_contained */ {
    // ...
    return;
}

if /* disabled for crate type */ { 
   // ...
   return
}

Copy link
Member

Choose a reason for hiding this comment

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

Also, it looks like this should also take the settings from the target spec into account (see fn self_contained_components in this file). Maybe that could also be used to remove the special case for Android?

Although, it looks to me like this check against what the target allows should happen much earlier in the process and not shortly before the linker is invoked. @lqd, what do you think (since this was added by #116035)? Or maybe I'm overlooking something?

Copy link
Member

Choose a reason for hiding this comment

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

It's likely we could move some of the checks earlier in some cases, but other checks/features do depend on the inferred linker flavor and thus couldn't all be moved that much earlier (IIRC sanitizers support is not long after the flavor has been inferred, and does depend on it a bit). Some other components can themselves also be inferred -- e.g. some variants in LinkSelfContainedDefault -- and also currently happen quite "late".

I don't myself have yet a good clear vision on how to structure all these concerns in the future, whether by splitting the currently-tightly-grouped checks into different early and late groups, or something else, but maybe @petrochenkov does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to make any requested changes.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@michaelwoerister
Copy link
Member

This looks good to me. Thanks, @chriswailes!

@lqd, I think I'm a bit confused by the self_contained_components function. It seems to take the CLI arguments into account, but only if it is yes or no. Otherwise it will return the target defaults. Would it make sense for it to merge target defaults and the CLI options at that point?

@chriswailes
Copy link
Contributor Author

Is there anything more you'd like me to do for this change?

@petrochenkov
Copy link
Contributor

I didn't have a chance to look at the changes yet, I'll do it later this week, most likely.

@michaelwoerister
Copy link
Member

I think we mainly need to clarify how this interacts with the defaults specified by the target (I'm not sure these are always "defaults", some parts of the code seem to treat that as restrictions on what options are allowed) -- or decide that we don't need to do this now because the feature isn't stable yet. In both cases, I don't want to approve this without feedback from @petrochenkov or @lqd, who have more ownership of the link-self-contained feature.

@lqd
Copy link
Member

lqd commented Mar 2, 2024

@lqd, I think I'm a bit confused by the self_contained_components function. It seems to take the CLI arguments into account, but only if it is yes or no. Otherwise it will return the target defaults. Would it make sense for it to merge target defaults and the CLI options at that point?

@michaelwoerister yeah that could eventually be part of the whole story

  • the support for self-contained linking components is not finished of course, and I only implemented the linker component of Vadim's vision (and am not familiar with the other components). In this function here, we had a unique yes/no value (which still flows downstream e.g. in get_linker, and all this could need to handle components) so it mostly looks the same as the old -> bool one in order to limit regressions and keep the existing -Clink-self-contained=y stable behavior as-is. I feared our test coverage could be spotty for such uncommon feature and target combinations. I did break some targets in the process of implementing parts of this.
  • I believe merging the CLI and target options is a small part of whether a component is actually enabled, which can also be very different per component. (Here's the self-contained linking linker component as an example, and this PR shows the sanitizers checks for comparison), so I'm not yet sure if it would help a lot. Once we have more components implemented, we could maybe pull out some of the common code between them at the cost of spreading their implementations around more.

Here are a couple questions I had reading this PR:

  • do we expect -Clink-self-contained=+sanitizers with sess.target.link_self_contained.is_disabled() to throw an error about the target preventing self-contained linking?
  • -Clink-self-contained=+sanitizers is a no-op here, right? Only -Clink-self-contained=-sanitizers bails out of adding the libraries, i.e. do we want to bail when !sess.opts.cg.link_self_contained.is_sanitizers_enabled()? And that begs the question, what is +sanitizers supposed to do without -Zsanitizer, and do we need some consistency checks between the two?
  • do we want -Zsanitizer to imply -Clink-self-contained=+sanitizers, or to always require using both? Or similarly, whether we eventually want to check if the sanitizers component was enabled, at the various points in codegen where we take the active sanitizers into account?

I think we mainly need to clarify how this interacts with the defaults specified by the target (I'm not sure these are always "defaults", some parts of the code seem to treat that as restrictions on what options are allowed)

Exactly. I know this PR is only about CLI support, but some similar questions could be discussed about the target specs, e.g. they could enable the sanitizer component but I don't believe there's an easy -Zsanitizer equivalent today, what then? Tbh I'm not sure a target would want to require sanitizers (except maybe CFI), but I wonder if we generally want to have symmetry between the CLI and target specs. So in that sense, it seems fine to me to not take into account the target defaults yet, but I'd also like (probably from Vadim) opinions about what we expect from -Clink-self-contained=+sanitizers, and if/how it should impact -Zsanitizer?

@petrochenkov
Copy link
Contributor

So it looks like -Zsanitizers does 2 things

  • affects codegen
  • affects linking (adds some libraries)

And the effect of both #121420 and this PR is that the linking part is skipped entirely.

If the linking part is just skipped, then it doesn't look like -Clink-self-contained.
With that option

  • -Clink-self-contained=+sanitizers would pass either my-rust-toolchain/subdir/with/sanitizers/librustc_rt.msan.a (like it happens now by default) or something like -L my-rust-toolchain/subdir/with/sanitizers -l rustc_rt.msan to the linker.
  • -Clink-self-contained=-sanitizers on the other hand would pass -l clang_rt.msan, so the sanitizer library is found somewhere on the system (the user may adjust where exactly by also passing -L options to the linker).

@chriswailes
Do you plan to pass the sanitizer libraries manually in your scenario?
By full path, by name?
Will -Clink-self-contained=-sanitizers as described above work for you?

(Also is there some convergence on the names of sanitizer libraries, or they can be named differently?)

@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 Mar 5, 2024
@chriswailes
Copy link
Contributor Author

Do you plan to pass the sanitizer libraries manually in your scenario?

Yes, we link the libraries in explicitly with absolute paths.

-Clink-self-contained=-sanitizers on the other hand would pass -l clang_rt.msan, so the sanitizer library is found somewhere on the system (the user may adjust where exactly by also passing -L options to the linker).

The Android build system uses absolute paths in all possible cases when passing arguments to compilers. While adjusting the search path to find our desired libclang_rt.msan is possible, we'd prefer that no linker flag for the runtime was emitted and satisfying the requirement was left to whomever or whatever invoked the compiler.

(Also is there some convergence on the names of sanitizer libraries, or they can be named differently?)

I'm not sure if the naming of the sanitizer libraries is stabilized and being able to link in different sanitizer runtimes might be helpful to researchers and developers.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2024
@petrochenkov
Copy link
Contributor

It looks like this use-case is different from -Clink-self-contained after all.

Maybe something like -Zsanitizers=msan,nolink or a separate option like #121207 would be more suitable.

@petrochenkov petrochenkov removed their assignment Mar 12, 2024
@michaelwoerister
Copy link
Member

Alright, given the above discussion I'll approve #121207 and close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants