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.

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

Conversation

geetanshjuneja
Copy link
Contributor

Implements shim_sig macro.
More details here - #3842 (comment)

@geetanshjuneja
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label May 23, 2025
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,
                )?;

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

@rustbot author

}

#[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.

Comment on lines +1450 to +1452
($this:ident, extern $abi:literal fn($($arg:tt),*) -> $ret:tt) => {
shim_sig!($this, extern "Rust" fn($($arg),*) -> $ret)
};
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

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

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

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.

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels May 26, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 26, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@geetanshjuneja
Copy link
Contributor Author

@RalfJung I have created the topic #general > Convert ty token to series of tt token on zulip. Can you tag someone who can help with this?

@RalfJung
Copy link
Member

RalfJung commented Jun 4, 2025

If I knew who could help I'd have already done that.^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: Waiting for the PR author to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants