pnkfelix / cee-scape Goto Github PK

Like the title says: The current crate is hard-wired to x86_64 sys-v assembly, but it shouldn't be.

Right now, I am using doc tests with compile_fail marking to confirm that we reject certain bad patterns.

The problem is that this is a weak form of testing. Those tests might be failing for independent reasons (i.e. unrelated borrow-check errors) and thus we might not actually be checking the thing we think we are checking.

The usual way that the Rust project has dealt with this is to have UI tests that actually confirm the output diagnostics, so that we are more assured that the compiler is rejecting the erroneous code for the reason we expect it to. So I should consider doing the same here.

#9 pointed out that PIC was likely broken.

I ended up fixing the hypothesized reason for the brokenness (by removing use of sym in the inline asm), but it would be nice to have a test case demonstrating that PIC actually does work here.

I assume Github actions would suffice for my most immediate testing needs, depending on what I can assume/observe about the machine and ABI that the CI runs atop.

Calling (or often using) sym in asm directly generally doesn't work in position independent code (and honestly a bunch of other cases -- sym is a very hard feature to use in my experience).

For example, for pic on ELF usually you'll need something like {}@PLT, but sometimes you'll need a rip-relative load and indirect call. To me the details are pretty tedious to get right everywhere (especially on 32 bit x86, where I'm not sure if it's possible). I think it's pretty portable to just call via a function pointer, and I doubt in these cases the performance difference is significant, are you open to this?

That said I haven't tried to produce a failure. Maybe this works for some reason I'm not realizing.

I also suspect clobber should be clobber_abi("C") (being specific about the ABI doesn't seem beneficial, but I suppose maybe it's a form of documentation?), although it doesn't really matter a ton since I think this only currently supports Linux.

Like the title says: The current crate is hard-wired to x86_64 sys-v assembly.

I have never attempted to make a crate that posts to doc.rs. It would be a good step forward for me. :)

The current code moves values into argument position explicitly, but doesn't reflect that in its clobber directives.

I had thought the clobber_abi("sysv64") would be good enough, since it reflects writes to all the relevant registers that I am overwriting.

However, experts from the inline asm project group pointed out to me this text:

if the calling convention does not fully preserve the value of a register across a call then lateout("...") _ is implicitly added to the operands list (where the ... is replaced by the register's name).

Notable: that says lateout, not out.

The docs for lateout in turn say:

• Identical to out except that the register allocator can reuse a register allocated to an in.
• You should only write to the register after all inputs are read, otherwise you may clobber an input.

This implies that I cannot rely on my clobber_abi directive to do all the heavy lifting here; I have a write to a register that occurs before a read from a compiler-assigned register, and I should fix it so that the register that is written early has an out(<reg>) _ in the directive list.

More registers are left undefined after a lonjmp() return (setjmp returning nonzero) than a normal 0 return from setjmp.

Please verify that the right registers are clobbered, and/or add a comment describing what's happening.

Feedback from zulip is questioning how I'm handling the forgetting of the callback type, namely Elichai Turkel notes:

Doesn't this moves the callback type from the stack and thus invalidating the pointer? IMHO it's easiest to use a ManuallyDrop

74e3290 added a named argument that follows explicit register arguments.

This apparently causes a compile failure, at least in Rust 1.69 on Mac OS X and Linux.

The code currently does closure_env_ptr.read() to extract the closure's environment before doing the call into the callback.

But the read() method is unsafe for a reason; see https://doc.rust-lang.org/std/ptr/fn.read.html#ownership-of-the-returned-value

Namely, you can end up with multiple ownership paths to the same owned data, which can then lead to badness/UB from double-drops etc.

I need to double-check the behavior here, and fix things if necessary.

rust-lang/unsafe-code-guidelines#256 discusses issues with how one might choose to encode the common C pattern of transmuting back and forth from a pointer to a struct and a pointer to a prefix of that struct.

glibc setjmp inherently does this.

I don't think its actually a real issue for this crate, because the JmpBufFields and SigJmpBufFields should really be extern types, not accessible on the Rust side of things.

But at the same time, I am passing a &'a _ around, in order to prevent people from using the reference outside of its scope. And so I just want to double check that my usage here is allowed.

(unfortunately I don't expect miri to be any help for us checking this, since it doesn't support foreign code nor inline assembly. It would be pretty cool if I could get krabcake far enough along for it to make a statement one way or another about what I'm doing here.)

I want to prototype a variant of the crate that totally avoids inline asm and uses ("portable"?) C instead.

I ended up posting this crate without that in place because I wasn't sure the best architecture to use for that C dependency. (E.g. should it be a separate crate? Or can I make it a separate file that is conditionally-compiled?)

From https://doc.rust-lang.org/rust-by-example/unsafe/asm.html#labels

Moreover, on x86 when using the default Intel syntax, due to an LLVM bug, you shouldn't use labels exclusively made of 0 and 1 digits, e.g. 0, 11 or 101010, as they may end up being interpreted as binary values.

I hadn't mean for call_with_sigsetjmp to be part of the default API for this crate. It just was something that seemed like some users would need and that would be "easy" to add (but maybe not as easy as you might think...)