Skip to content

Update to support 2022-07-11 rustc_middle #513

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

Closed
wants to merge 5 commits into from
Closed

Conversation

fw-immunant
Copy link
Contributor

@fw-immunant fw-immunant commented Jul 12, 2022

This will want either a docker redeploy with the new rust-toolchain file or a change to use rust-toolchain.toml so rustup can add the required components dynamically.

@kkysen
Copy link
Contributor

kkysen commented Jul 12, 2022

This is causing some weird compile errors, both in CI and locally (but different ones). Locally, I'm also seeing this from cargo doc:

error[E0308]: mismatched types
   --> /home/kkysen/.cargo/registry/src/github.com-1ecc6299db9ec823/semver-1.0.10/src/lib.rs:525:29
    |
525 |     pub const EMPTY: Self = "";
    |                             ^^ expected struct `BuildMetadata`, found `&str`

error[E0308]: mismatched types
   --> /home/kkysen/.cargo/registry/src/github.com-1ecc6299db9ec823/semver-1.0.10/src/lib.rs:502:29
    |
502 |     pub const EMPTY: Self = "";
    |                             ^^ expected struct `Prerelease`, found `&str`

For more information about this error, try `rustc --explain E0308`.

@kkysen
Copy link
Contributor

kkysen commented Jul 12, 2022

Ah, the CI errors are from old rustup component add rust-src rustc-dev. I'm working right now on the rust-toolchain.toml fix, which should solve that (so that CI passes, but it'll still have to download components every time until we update the docker containers).

let mut extra_statements = None;
if after_call {
let call = blocks[loc.block].terminator_mut();
let ret_value = if let TerminatorKind::Call {
destination: Some((place, _next_block)),
destination: place,
//target: Some(_next_block),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may need to be uncommented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is/was a _* unused variable being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable itself isn't used, but we may care whether the target is Some or None, as we cared before about destination being Some. If we just need a binding for place, we can forget about target.

Comment on lines -677 to +678
if destination.is_some() {
if true {
Copy link
Contributor Author

@fw-immunant fw-immunant Jul 12, 2022

Choose a reason for hiding this comment

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

This logic can now execute unconditionally, but I've left the if true to have a place to hang a discussion off of. When would destination have been None? Do we need to ignore these cases some other way now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Leaving for reference.
It's quite a long if.

if true {
println!("term: {:?}", terminator.kind);
let fn_name = self.tcx.item_name(def_id);
// println!(
// "visiting func {:?}, {:?}",
// fn_name,
// self.tcx.def_path_hash(*def_id)
// );
if HOOK_FUNCTIONS.contains(&fn_name.as_str()) {
let func_def_id = self
.find_instrumentation_def(fn_name)
.expect("Could not find instrumentation hook function");
// Hooked function called; trace args
self.add_instrumentation_point(
location,
func_def_id,
args.iter()
.map(|arg| {
InstrumentationOperand::from_type(
arg.clone(),
&arg.ty(self.body, self.tcx),
)
})
.collect(),
false,
/* after_call: */ true,
EventMetadata {
// TODO: hook-specific sources
source: args
.clone()
.iter()
.map(|op| to_mir_place(&op.place().unwrap()))
.next(),
// FIXME: hooks have sources
destination: Some(to_mir_place(destination)),
transfer_kind: TransferKind::Ret(self.func_hash()),
},
);
} else if destination
.ty(&self.body.local_decls, self.tcx)
.ty
.is_unsafe_ptr()
{
location.statement_index = 0;
location.block = target.unwrap();
// Non-hooked fn with raw-ptr result called; trace destination
self.add_instrumentation_point(
location,
arg_fn,
vec![InstrumentationOperand::RawPtr(Operand::Copy(*destination))],
false,
false,
EventMetadata {
source: Some(to_mir_place(&Local::from_u32(0).into())),
destination: Some(to_mir_place(destination)),
transfer_kind: TransferKind::Ret(
self.tcx.def_path_hash(def_id).convert(),
),
},
);
}
}

@kkysen
Copy link
Contributor

kkysen commented Jul 12, 2022

The semver doc compile error is our cargo dependency causing issues again. I'll update it. We really should get rid of cargo, though. It's caused many, many issues by now.

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

This is good to go once I fix the rust-toolchain.toml thing, and possibly once we address your comments. Upgrading the docker containers would work, too, for this, but then break any old branches we're working on if they don't merge.

@fw-immunant
Copy link
Contributor Author

I'll rebase.

kkysen added a commit that referenced this pull request Jul 30, 2022
…is-rt` instead of the user having to manually do it. However, we have to use `+stable` as `cargo add` was added in 1.62 (on 1.60 now, but upgrading in #513).
@kkysen
Copy link
Contributor

kkysen commented Aug 12, 2022

Superceded by #595.

@kkysen kkysen closed this Aug 12, 2022
kkysen added a commit that referenced this pull request Aug 16, 2022
…is-rt` instead of the user having to manually do it. However, we have to use `+stable` as `cargo add` was added in 1.62 (on 1.60 now, but upgrading in #513).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants