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

Environ shim #1147

Closed
wants to merge 4 commits into from
Closed
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
27 changes: 23 additions & 4 deletions src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,21 @@ pub struct MemoryExtra {

/// Whether to enforce the validity invariant.
pub(crate) validate: bool,
/// Contains the `environ` static.
pub(crate) environ: Option<Allocation<Tag, AllocExtra>>,
}

impl MemoryExtra {
pub fn new(rng: StdRng, validate: bool, tracked_pointer_tag: Option<PtrId>) -> Self {
pub fn new(
rng: StdRng, validate: bool,
Copy link
Member

Choose a reason for hiding this comment

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

That's some odd formatting... either all arguments should be on one line, or they should all have their own line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I have rustfmt disabled to avoid huge diffs. I'll do the formatting when I have a more concise PR

tracked_pointer_tag: Option<PtrId>,
) -> Self {
MemoryExtra {
stacked_borrows: Rc::new(RefCell::new(GlobalState::new(tracked_pointer_tag))),
intptrcast: Default::default(),
rng: RefCell::new(rng),
validate,
environ: None,
}
}
}
Expand Down Expand Up @@ -266,7 +272,9 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
fn find_foreign_static(
tcx: TyCtxt<'tcx>,
def_id: DefId,
) -> InterpResult<'tcx, Cow<'tcx, Allocation>> {
id: AllocId,
memory_extra: &MemoryExtra,
) -> InterpResult<'tcx, Cow<'tcx, Allocation<Tag, AllocExtra>>> {
let attrs = tcx.get_attrs(def_id);
let link_name = match attr::first_attr_value_str_by_name(&attrs, sym::link_name) {
Some(name) => name.as_str(),
Expand All @@ -278,11 +286,22 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
// This should be all-zero, pointer-sized.
let size = tcx.data_layout.pointer_size;
let data = vec![0; size.bytes() as usize];
Allocation::from_bytes(&data, tcx.data_layout.pointer_align.abi)
// We got tcx memory. Let the machine initialize its "extra" stuff.
let (alloc, tag) = Self::init_allocation_extra(
memory_extra,
id, // always use the ID we got as input, not the "hidden" one.
Cow::Owned(Allocation::from_bytes(&data, tcx.data_layout.pointer_align.abi)),
Self::STATIC_KIND.map(MemoryKind::Machine),
);
debug_assert_eq!(tag, Self::tag_static_base_pointer(memory_extra, id));
alloc
}
"environ" => {
Cow::Owned(memory_extra.environ.as_ref().cloned().unwrap())
}
_ => throw_unsup_format!("can't access foreign static: {}", link_name),
};
Ok(Cow::Owned(alloc))
Ok(alloc)
}

#[inline(always)]
Expand Down
20 changes: 20 additions & 0 deletions src/shims/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::ffi::{OsString, OsStr};
use std::env;

use crate::stacked_borrows::Tag;
use crate::rustc_target::abi::LayoutOf;
use crate::*;

use rustc::ty::layout::Size;
Expand All @@ -20,15 +21,34 @@ impl EnvVars {
ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>,
excluded_env_vars: Vec<String>,
) {
let mut vars = Vec::new();
if ecx.machine.communicate {
// Put each environment variable pointer in `EnvVars`, collect pointers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Put each environment variable pointer in `EnvVars`, collect pointers.
// Put each environment variable pointer in `EnvVars`, collect pointers.

for (name, value) in env::vars() {
if !excluded_env_vars.contains(&name) {
let var_ptr =
alloc_env_var_as_c_str(name.as_ref(), value.as_ref(), ecx);
ecx.machine.env_vars.map.insert(OsString::from(name), var_ptr);
vars.push(var_ptr.into());
}
}
}
// add trailing null pointer
vars.push(Scalar::from_int(0, ecx.pointer_size()));
// Make an array with all these pointers, in the Miri memory.
let tcx = ecx.tcx;
let environ_layout =
ecx.layout_of(tcx.mk_array(tcx.mk_imm_ptr(tcx.types.u8), vars.len() as u64)).unwrap();
let environ_place = ecx.allocate(environ_layout, MiriMemoryKind::Env.into());
for (idx, var) in vars.into_iter().enumerate() {
pvdrz marked this conversation as resolved.
Show resolved Hide resolved
let place = ecx.mplace_field(environ_place, idx as u64).unwrap();
ecx.write_scalar(var, place.into()).unwrap();
}
ecx.memory.mark_immutable(environ_place.ptr.assert_ptr().alloc_id).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this prevent later uses of set_env?

Copy link
Member

Choose a reason for hiding this comment

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

I guess the question is, do we try to be smart and recycle those lists, or do we just allocate a new one on each change?

But either way, we'll have to construct such a list multiple times -- so it probably should have its own function.

// A pointer to that place corresponds to the `environ` static.
let environ_ptr = ecx.force_ptr(environ_place.ptr).unwrap();
let environ_alloc = ecx.memory.get_raw(environ_ptr.alloc_id).unwrap().clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is suboptimal as the allocation will be left in Memory forever even if it's never going to get used. I'm not sure if this is feasible, but you may be able to create the alloc with Allocation::undef(size, align) and then write to it directly instead of going through all the extra accessors above.

Copy link
Member

Choose a reason for hiding this comment

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

Having the allocation last forever makes sense to me, it's a static after all.

We'll also need to update this allocation when the environment changes (whenever environ_place above gets reallocated).

ecx.memory.extra.environ = Some(environ_alloc);
}
}

Expand Down