Skip to content

implement shim_sig macro #4342

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.

Sign up for GitHub

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 2 commits into
base: master
Choose a base branch
from
Open

implement shim_sig macro #4342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0099300
implement shim_sig macro
geetanshjuneja May 22, 2025
21bd1b5
fmt
geetanshjuneja May 22, 2025
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
33 changes: 33 additions & 0 deletions src/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1435,3 +1435,36 @@ impl ToU64 for usize {
self.try_into().unwrap()
}
}

#[macro_export]
macro_rules! shim_sig {
($this:ident, extern $abi:literal fn($($arg:tt),*) -> $ret:tt) => {
(
std::str::FromStr::from_str($abi).expect("incorrect abi specified"),
[$(layout!($this, $arg)),*],
layout!($this, $ret)
)
};

// default Rust abi
($this:ident, extern $abi:literal fn($($arg:tt),*) -> $ret:tt) => {
shim_sig!($this, extern "Rust" fn($($arg),*) -> $ret)
};
Comment on lines +1450 to +1452
Copy link
Member

Choose a reason for hiding this comment

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

Having to use :tt instead of :ty here is unfortunate. I guess that's because *const _ is not a valid type? Maybe we can use *const T instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I won't be able to match libc types.

Copy link
Member

Choose a reason for hiding this comment

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

Why not? Please explain the problem.

Copy link
Contributor Author

@geetanshjuneja geetanshjuneja May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

shim_sig!(this, extern "C" fn() -> "pid_t")

"pid_t" won't be able to match ty

Copy link
Member

Choose a reason for hiding this comment

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

Why not? That's a valid type, syntactically.

Copy link
Contributor Author

@geetanshjuneja geetanshjuneja May 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

macro_rules! layout_and_ty {
    ($this:ident, *const T) => {
        $this.machine.layouts.const_raw_ptr.ty
    };
    ($this:ident, *mut T) => {
        $this.machine.layouts.mut_raw_ptr.ty
    };
    ($this:ident, i32) => {
        $this.tcx.types.i32
    };
    ($this:ident, usize) => {
        $this.tcx.types.usize
    };
    ($this:ident, isize) => {
        $this.tcx.types.isize
    };
    ($this:ident, $x:ty) => {
        $this.libc_ty_layout(stringify!($x)).ty
    };
}

When I call this macro layout_and_ty($this, i32), it matches with ($this:ident, $x:ty) not ($this:ident, i32)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the macro arm of layout_and_ty, is it taking i32 as a keyword not a rust type?

Copy link
Member

Choose a reason for hiding this comment

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

i32 is not a keyword though...

But I would anyway say the fallback arm should be

    ($this:ident, libc::$x:ident) => {
        $this.libc_ty_layout(stringify!($x)).ty
    };

Does that help?

Also your macro should probably return the entire layout, not just the type.

Copy link
Contributor Author

@geetanshjuneja geetanshjuneja May 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i32 is not a keyword though...

I think the issue is that when I pass already matched ty token in shim_sig to layout_and_ty, then I can only match $x:ty arm. But if I would have directly called layout_and_ty macro then I would have been able to match i32 arm. See this.
Am I correct here?
So I think I have do if-else inside x:ty arm and return the correct layout.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I don't know if there is a trick to "degenerate" a ty token back to a series of tt. Maybe ask on Zulip? I'm not a macro expert unfortunately.

}

#[macro_export]
macro_rules! layout {
Copy link
Member

Choose a reason for hiding this comment

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

Please give this macro a more clear name, such as layout_for_ty.

($this:ident, ) => {};
($this:ident, (*const _)) => {
$this.machine.layouts.const_raw_ptr.ty
};
($this:ident, (*mut _)) => {
$this.machine.layouts.mut_raw_ptr.ty
};
($this:ident, $arg:ident) => {
$this.tcx.types.$arg
};
($this:ident, $arg:literal) => {
$this.libc_ty_layout($arg).ty
};
}
37 changes: 12 additions & 25 deletions src/shims/unix/foreign_items.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use self::shims::unix::solarish::foreign_items as solarish;
use crate::concurrency::cpu_affinity::CpuAffinityMask;
use crate::shims::alloc::EvalContextExt as _;
use crate::shims::unix::*;
use crate::*;
use crate::{shim_sig, *};

pub fn is_dyn_sym(name: &str, target_os: &str) -> bool {
match name {
Expand Down Expand Up @@ -112,26 +112,18 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
match link_name.as_str() {
// Environment related shims
"getenv" => {
let [name] = this.check_shim_abi(
link_name,
abi,
ExternAbi::C { unwind: false },
[this.machine.layouts.const_raw_ptr.ty],
this.machine.layouts.mut_raw_ptr.ty,
args,
)?;
let (callee_abi, arg_types, ret_type) =
shim_sig!(this, extern "C" fn((*const _)) -> (*const _));
let [name] =
this.check_shim_abi(link_name, abi, callee_abi, arg_types, ret_type, args)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Outsider question: How many places is check_shim_abi used so far? Could abi, args, and return get packaged into one argument so you could just use the macro directly in the parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How many places is check_shim_abi used so far?

It is used in shims/unix/foreign_items.rs.

Could abi, args, and return get packaged into one argument so you could just use the macro directly in the parameters?

Yes, I'll package them into one argument .

Copy link
Member

Choose a reason for hiding this comment

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

Yes that is what I suggested in the first place:

                let [name, value, overwrite] = this.check_shim_abi(
                    shim_sig!(extern "C" fn(*const _, *const _, i32) -> i32),
                    link_name,
                    abi,
                    args,
                )?;

let result = this.getenv(name)?;
this.write_pointer(result, dest)?;
}
"unsetenv" => {
let [name] = this.check_shim_abi(
link_name,
abi,
ExternAbi::C { unwind: false },
[this.machine.layouts.const_raw_ptr.ty],
this.tcx.types.i32,
args,
)?;
let (callee_abi, arg_types, ret_type) =
shim_sig!(this, extern "C" fn((*const _)) -> i32);
let [name] =
this.check_shim_abi(link_name, abi, callee_abi, arg_types, ret_type, args)?;
let result = this.unsetenv(name)?;
this.write_scalar(result, dest)?;
}
Expand Down Expand Up @@ -177,14 +169,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
this.write_scalar(result, dest)?;
}
"getpid" => {
let [] = this.check_shim_abi(
link_name,
abi,
ExternAbi::C { unwind: false },
[],
this.libc_ty_layout("pid_t").ty,
args,
)?;
let (callee_abi, arg_types, ret_type) = shim_sig!(this, extern "C" fn() -> "pid_t");
let [] =
this.check_shim_abi(link_name, abi, callee_abi, arg_types, ret_type, args)?;
let result = this.getpid()?;
this.write_scalar(result, dest)?;
}
Expand Down