Skip to content

krabcake: Successfully send intrinsics::assume data to Valgrind #1

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

Open
wants to merge 1 commit into
base: master-before-krabcakeeeee-squash
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions compiler/rustc_hir/src/lang_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ pub fn extract(attrs: &[ast::Attribute]) -> Option<(Symbol, Span)> {

language_item_table! {
// Variant name, Name, Getter method name, Target Generic requirements;
KrabcakeRequest, sym::KrabcakeRequest, krabcake_request, Target::Enum, GenericRequirement::None;
InsertKrabcakeRequest, sym::insert_krabcake_request, insert_krabcake_request_fn, Target::Fn, GenericRequirement::None;
Sized, sym::sized, sized_trait, Target::Trait, GenericRequirement::Exact(0);
Unsize, sym::unsize, unsize_trait, Target::Trait, GenericRequirement::Minimum(1);
/// Trait injected by `#[derive(PartialEq)]`, (i.e. "Partial EQ").
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_interface/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,7 @@ fn test_unstable_options_tracking_hash() {
tracked!(inline_mir_hint_threshold, Some(123));
tracked!(inline_mir_threshold, Some(123));
tracked!(instrument_coverage, Some(InstrumentCoverage::All));
tracked!(instrument_krabcake, false);
tracked!(instrument_mcount, true);
tracked!(instrument_xray, Some(InstrumentXRay::default()));
tracked!(link_directives, false);
Expand Down
12 changes: 12 additions & 0 deletions compiler/rustc_middle/src/mir/patch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,4 +179,16 @@ impl<'tcx> MirPatch<'tcx> {
};
Self::source_info_for_index(data, loc)
}

pub fn terminator_for_location(
&self,
body: &Body<'tcx>,
loc: Location,
) -> Option<Terminator<'tcx>> {
let data = match loc.block.index().checked_sub(body.basic_blocks.len()) {
Some(new) => &self.new_blocks[new],
None => &body[loc.block],
};
data.terminator.clone()
}
}
6 changes: 5 additions & 1 deletion compiler/rustc_middle/src/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -680,7 +680,11 @@ where
| ty::GeneratorWitnessMIR(..)
| ty::Foreign(..)
| ty::Dynamic(_, _, ty::Dyn) => {
bug!("TyAndLayout::field({:?}): not applicable", this)
bug!(
"TyAndLayout::field({:?}): not applicable for kind {:?}",
this,
this.ty.kind()
)
}

// Potentially-fat pointers.
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_mir_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ mod simplify_comparison_integral;
mod sroa;
mod uninhabited_enum_branching;
mod unreachable_prop;
mod valgrind_client_request;

use rustc_const_eval::transform::check_consts::{self, ConstCx};
use rustc_const_eval::transform::promote_consts;
Expand Down Expand Up @@ -588,6 +589,7 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
&large_enums::EnumSizeOpt { discrepancy: 128 },
// Some cleanup necessary at least for LLVM and potentially other codegen backends.
&add_call_guards::CriticalCallEdges,
&valgrind_client_request::ValgrindClientRequest,
// Dump the end result for testing and debugging purposes.
&dump_mir::Marker("PreCodegen"),
],
Expand Down
112 changes: 112 additions & 0 deletions compiler/rustc_mir_transform/src/valgrind_client_request.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
use crate::MirPass;

use rustc_index::vec::Idx;
use rustc_middle::mir::patch::MirPatch;
use rustc_middle::mir::{
AggregateKind, BasicBlockData, Body, Constant, ConstantKind, Location, NonDivergingIntrinsic,
Operand, Place, Rvalue, StatementKind, TerminatorKind,
};
use rustc_middle::ty::InternalSubsts;
use rustc_middle::ty::TyCtxt;
use rustc_target::abi::VariantIdx;

pub struct ValgrindClientRequest;

impl<'tcx> MirPass<'tcx> for ValgrindClientRequest {
#[instrument(skip(self, tcx, body))]
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
if !tcx.sess.opts.unstable_opts.instrument_krabcake {
info!("Not instrumenting for krabcake");
return;
}
info!("Instrumenting for krabcake now...");
let mut patch = MirPatch::new(body);

for (block_index, block_data) in body.basic_blocks.iter_enumerated() {
for (stmt_index, stmt) in block_data.statements.iter().enumerate() {
match &stmt.kind {
StatementKind::Intrinsic(box NonDivergingIntrinsic::Assume(operand)) => {
let loc = Location { block: block_index, statement_index: stmt_index };
info!("Found assume intrinsic (operand={operand:?}. At {loc:?}");
patch = call_ikr(tcx, patch, body, loc, operand);
}
_ => (),
}
}
}

patch.apply(body);
}
}

fn call_ikr<'tcx>(
tcx: TyCtxt<'tcx>,
mut patch: MirPatch<'tcx>,
body: &Body<'tcx>,
loc: Location,
_operand: &Operand<'tcx>,
) -> MirPatch<'tcx> {
let span = patch.source_info_for_location(body, loc).span;

let op = |flag: bool| {
Operand::Constant(Box::new(Constant {
span,
user_ty: None,
literal: ConstantKind::from_bool(tcx, flag),
}))
};

let (place, rvalue) = {
let krabcake_req_did = tcx.lang_items().krabcake_request().unwrap();
let krabcake_req_substs = InternalSubsts::identity_for_item(tcx, krabcake_req_did);
let krabcake_req_def = tcx.adt_def(krabcake_req_did);
let krabcake_req_ty = tcx.mk_adt(krabcake_req_def, krabcake_req_substs);
let rvalue = Rvalue::Aggregate(
Box::new(AggregateKind::Adt(
krabcake_req_did,
VariantIdx::new(0),
&krabcake_req_substs,
None,
None,
)),
vec![op(true)],
Copy link
Owner Author

Choose a reason for hiding this comment

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

TODO: Get the bool from the existing MIR intrinsics::assume itself

Copy link
Collaborator

Choose a reason for hiding this comment

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

namely, extract it from the _operand parameter you are passing above, right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

correct

);
let temp = patch.new_temp(krabcake_req_ty, span);
let place = Place::from(temp);
(place, rvalue)
};

patch.add_assign(loc, place, rvalue);

let krabcake_req_operand = Operand::Copy(place);

let orig_terminator = patch.terminator_for_location(body, loc);
let ikr_did = tcx.lang_items().insert_krabcake_request_fn().unwrap();
let ikr_substs = InternalSubsts::identity_for_item(tcx, ikr_did);
let ikr_ty = tcx.mk_fn_def(ikr_did, ikr_substs);

let func = Operand::Constant(Box::new(Constant {
span,
user_ty: None,
literal: ConstantKind::zero_sized(ikr_ty),
}));
let storage = patch.new_temp(tcx.mk_mut_ptr(tcx.types.unit), span);
let storage = Place::from(storage);
let fn_call_terminator_kind = TerminatorKind::Call {
func,
args: vec![krabcake_req_operand],
destination: storage,
target: Some(loc.block + 1),
cleanup: None,
from_hir_call: false,
fn_span: span,
};

patch.patch_terminator(loc.block, fn_call_terminator_kind);

let new_bb =
BasicBlockData { statements: vec![], terminator: orig_terminator, is_cleanup: false };

patch.new_block(new_bb);
patch
}
2 changes: 2 additions & 0 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1473,6 +1473,8 @@ options! {
`=except-unused-generics`
`=except-unused-functions`
`=off` (default)"),
instrument_krabcake: bool = (false, parse_bool, [TRACKED],
"insert Valgrind client requests to communicate with Krabcake"),
instrument_mcount: bool = (false, parse_bool, [TRACKED],
"insert function instrument code for mcount-based tracing (default: no)"),
instrument_xray: Option<InstrumentXRay> = (None, parse_instrument_xray, [TRACKED],
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ symbols! {
ItemContext,
Iterator,
IteratorItem,
KrabcakeRequest,
Layout,
Left,
LinkedList,
Expand Down Expand Up @@ -824,6 +825,7 @@ symbols! {
inline_const,
inline_const_pat,
inout,
insert_krabcake_request,
instruction_set,
integer_: "integer",
integral,
Expand Down
95 changes: 95 additions & 0 deletions library/core/src/krabcake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
use core::arch::asm;

mod vg_c_compatible {
const fn vg_userreq_tool_base(a: u64, b: u64) -> u64 {
((a) & 0xff) << 24 | ((b) & 0xff) << 16
}

/// blah
#[allow(dead_code)] // For currently unused variants
#[derive(Debug, Clone, Copy)]
#[repr(u64)]
pub(super) enum ValgrindClientRequestCode {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. Question: Should we be extending libcore with stuff like this?

Here are our options, as I see them:

  1. Client code code carries its own copy of the relevant enum (that's what we're doing now in https://github.com/pnkfelix/krabcake/blob/f353b59e7af59d055935022c6c3f72adf3a1e53e/sb_rs_port/src/krabcake.rs#L7 ). This is fine for demos, but a non-starter for our target audience for krabcake (where its supposed to be "just recompile and go")
  2. Rustc Compiler has access to all the integers that are used for the relevant enum, and embeds those values directly into the object code that gets generated. Demo code cannot refer to the enum itself from its source code. This is what I had originally envisaged.
  3. libcore defines the enum, and either the Rustc Compiler includes accesses to the elements of that enum in the code it injects (which is what I expect for the majority of the cases), or the client code can access the values itself for its own purposes.

I'm trying to envisage the cases where we would want to do option (3). The main benefit of option (3) AFAICT is that you get to write source code in your demos that reference the enum values, so you have more control over fine-tuning (if you wanted to e.g. experiment with alternative protocols for how you interact with the valgrind krabcake tool).

But I think in all such cases, I would opt to actually include a copy of the relevant enum in the source code (i.e. option (1.) over option (3.)

@bryangarza did you adopt this strategy because it was simpler to implement than option (2.)? Or am I missing some benefit of option (3.)?

Copy link
Owner Author

Choose a reason for hiding this comment

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

embeds those values directly into the object code that gets generated

Not sure I quite follow what you mean in option 2; do you mean basically being able to have the MirPass insert the krabcake request into the MIR but without needing a Lang Item? (example)

Or do you mean some method that doesn't use a MirPass?


did you adopt this strategy because it was simpler to implement than option (2.)? Or am I missing some benefit of option (3.)?

The reason I made it a lang item is so that it would be more maintainable to have most of the logic defined as Rust code instead of needing to handwrite a lot of MIR. Also, I created a separate data-type KrabcakeRequest so that we don't couple the C enum to the enum that is used in rustc, since as you said, we could in the future change the format or approach that is used in rustc itself independently of what the request looks like when sent to Valgrind (or we could even use something other than/in addition to Valgrind)

/// blah
BorrowMut = vg_userreq_tool_base('K' as u64, 'C' as u64),
/// blah
BorrowShr,
/// blah
AsRaw,
/// blah
AsBorrowMut,
/// blah
AsBorrowShr,
/// blah
RetagFnPrologue,
/// blah
RetagAssign,
/// blah
RetagRaw,
/// blah
IntrinsicsAssume,
/// blah
KrabcakeRecordOverlapError = vg_userreq_tool_base('K' as u64, 'C' as u64) + 256,
}

/// Ultimately, this should map to a [u64; 6], which is what Valgrind expects
#[derive(Debug, Clone, Copy)]
#[repr(C)]
struct ValgrindClientRequest<T>
where
T: Into<u64>,
{
req_code: ValgrindClientRequestCode,
args: [T; 5],
}
}

/// blah
/// #[doc(hidden)]
// There is no issue created for krabcake yet so just using a placeholder
#[unstable(feature = "instrument_krabcake", issue = "1")]
#[cfg_attr(not(bootstrap), lang = "KrabcakeRequest")]
#[derive(Debug, Clone, Copy)]
#[repr(C)]
pub enum KrabcakeRequest {
/// blah
IntrinsicsAssume {
/// blah
flag: bool,
},
}

#[doc(hidden)]
// There is no issue created for krabcake yet so just using a placeholder
#[unstable(feature = "instrument_krabcake", issue = "1")]
#[inline(never)]
#[cfg_attr(not(bootstrap), lang = "insert_krabcake_request")]
pub fn insert_krabcake_request(req: KrabcakeRequest) {
// `res` not used for IntrinsicsAssume request
let mut res = false as u64;
let args: [u64; 6] = match req {
KrabcakeRequest::IntrinsicsAssume { flag } => [
vg_c_compatible::ValgrindClientRequestCode::IntrinsicsAssume as u64,
flag.into(),
0,
0,
0,
0,
],
};

// SAFETY:
// This is a valid safety comment
unsafe {
asm!(
"rol rdi, 3",
"rol rdi, 13",
"rol rdi, 61",
"rol rdi, 51",
"xchg rbx, rbx",
inout("di") res,
in("ax") &args,
);
Copy link

Choose a reason for hiding this comment

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

We could have a terminator or intrinsic for emitting valgrind client requests, right? That would probably be more efficient as you can avoid the overhead of a non-inlineable call. Valgrind is relatively slow, so every bit helps I guess.

}
let _ = res;
}
5 changes: 5 additions & 0 deletions library/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,11 @@ pub mod task;
#[allow(missing_docs)]
pub mod alloc;

/// blah
// There is no issue created for krabcake yet so just using a placeholder
#[unstable(feature = "instrument_krabcake", issue = "1")]
pub mod krabcake;

// note: does not need to be public
mod bool;
mod tuple;
Expand Down