Skip to content

Newtype doesn't transparently wrap integers when calling FFI from Emscripten #41483

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

Closed
RReverser opened this issue Apr 23, 2017 · 10 comments
Closed

Comments

@RReverser
Copy link
Contributor

RReverser commented Apr 23, 2017

The newtype pattern is useful on FFI structs to have strongly-typed variations of a base integer type when calling FFI functions:

#[repr(C)]
struct MySpecialId(u32);

extern {
  fn get_special_id() -> MySpecialId;
  fn do_smth_with_special_id(id: MySpecialId);
}

pub fn main() {
  unsafe {
    let id = get_special_id();
    do_smth_with_special_id(id); // passes
    do_smth_with_special_id(100); // typecheck fails
  }
}

This works well with native targets, where such calls get lowered to just passing inner value on the stack:

#[repr(C)]
struct MySpecialId(u32);

extern {
  fn do_smth_with_special_id(id: MySpecialId);
}

pub fn main() {
  unsafe {
    do_smth_with_special_id(MySpecialId(100));
  }
}
example::main:
        push    rbp
        mov     rbp, rsp
        mov     edi, 100
        pop     rbp
        jmp     do_smth_with_special_id@PLT

However, when targeting Emscripten (e.g. asmjs), it changes behaviour by stack-allocating the structure and passing a pointer to it:

function __ZN4temp4main17h3462209f0fd3a45cE() {
 var $_2 = 0, $_2$byval_copy = 0, sp = 0;
 sp = STACKTOP;
 STACKTOP = STACKTOP + 16 | 0;
 $_2$byval_copy = sp + 4 | 0;
 $_2 = sp;
 HEAP32[$_2 >> 2] = 100;
 HEAP32[$_2$byval_copy >> 2] = HEAP32[$_2 >> 2];
 _do_smth_with_special_id($_2$byval_copy | 0);
 STACKTOP = sp;
 return;
}

So, for example, linking with a static library generated from the following C code:

#include <stdio.h>

extern void do_smth_with_special_id(unsigned id) {
    printf("%u\n", id);
}

results in printing 100 on every other target, but prints the integer value of a stack pointer (e.g. 15812) on Emscripten targets.

Same happens when linking with Emscripten JS libraries (using --js-library or upcoming #41409).

Of course, as a workaround, I can dereference pointer on callee side every time, but this is both inefficient, and also seems that this inconsistent behaviour is rather a bug that needs to be fixed.

I can see that newtypes actually map to LLVM structures and not integers:

declare void @do_smth_with_special_id({ i64 })

so this might be considered as a bug on Emscripten side which doesn't lower such structures, or lowering could happen on Rust side to ensure consistency - not sure which side is appropriate.

cc @alexcrichton @kripken

@RReverser
Copy link
Contributor Author

FWIW get_special_id (such type in return position) is lowered as expected, which means round-trip where I get newtype MySpecialId from extern function and return it also to extern function, is broken.

@retep998
Copy link
Member

retep998 commented Apr 23, 2017

It is intentional that newtypes act as structs and are not transparent, because they are structs, just with only one field. If you want transparency you need #[repr(transparent)] which is in RFC form and about to enter FCP: rust-lang/rfcs#1758.

@RReverser
Copy link
Contributor Author

@retep998 Thanks for the pointer to that RFC. However, it doesn't seem right that behaviour is inconsistent across platforms even with repr(C) whereas C itself guarantees that such struct will be passed as a copy of its inner data.

It is intentional that newtypes act as structs and are not transparent

The intentional bit sounds strange given how often newtype pattern is recommended in Rust as "transparent" way to apply custom traits onto some type without affecting its real representation and behaviour.

And especially given that it behaves inconsistently in return position and argument position, making roundtrip impossible without manual conversions despite same type being used in both.

@hanna-kruppe
Copy link
Contributor

However, it doesn't seem right that behaviour is inconsistent across platforms even with repr(C) whereas C itself guarantees that such struct will be passed as a copy of its inner data.

Does the equivalent C code produce the same issue on emscripten? That's the reference point for repr(C) and extern "C" fn, not, say, an x86 compiler.

@RReverser
Copy link
Contributor Author

@rkruppe Yeah, it does, which is why I said above

so this might be considered as a bug on Emscripten side which doesn't lower such structures

It's just that, unlike C, it's Rust that often recommends newtypes everywhere where you want to add custom behaviour transparently, so one might expect it to care about lowering on its side (or maybe it's just me and I'm wrong).

I can report issue to Emscripten instead, but figured should start here to see if this is considered as either 1) a bug or 2) an optimization opportunity in Rust itself.

@hanna-kruppe
Copy link
Contributor

If C code does it too, then the C ABI says single-integer structures are different from plain integers. There's not much we can do about that on the Rust side, repr(C) and extern "C" fn need to match the C ABI. The only way out would be to say declare that "newtypes" should not be considered aggregates for the purpose of ABI computation, but there are many reasons not to do that:

  1. it would be a breaking change (since they're currently treated as aggregates)
  2. it would be extremely inconsistent
  3. it's not clear how "newtype" should be delineated — for example, do additional zero-sized fields (e.g., PhantomData) count?
  • If yes, that means struct IntPlus<T>(i32, T) can be an aggregate or not depending on T, which might be very surprising.
  • At the same time, newtypes often use PhantomData to add additional type safety (e.g., like euclid does), so you'd want to at least consider some ZSTs differently.

Finally, two points that make it less urgent to do anything about this:

  1. The above reasons only apply to repr(C), with repr(Rust) we have absolute liberty and in fact exploit that freedom, though not as much as we probably should. (But of course, the same freedom makes those types unsuitable for FFI.)
  2. You always have the option of writing (or writing a macro that generates) the extern declarations with the correct C types, and writing a wrapper function around them that accepts newtypes, unpacks them, performs the call, and packages up the result in a newtype. This is very inconvenient — hence the repr(transparent) RFC — but it mostly works.

@RReverser
Copy link
Contributor Author

RReverser commented Apr 23, 2017

Ok, thanks for the detailed explanation, your points make sense, although it's indeed

very inconvenient

As for

You always have the option of writing (or writing a macro that generates) the extern declarations with the correct C types

^ that's exactly what I'm doing and providing safe interface outside, but due to the amount of functions that have to deal with various "classes" of same-sized integers, wanted a way to ensure internal consistency as well so that I don't accidentally pass integer retrieved from the function that deals with one type of ids to another one. Hence this attempt with newtypes which works everywhere else but not when targetting Emscripten :(

As a workaround, for now I can use separate dummy structs and declare these as pointers to these structs, so that they're still type-checked by default, but that's also 1) quite inconvenient and 2) still too easy to break with casts.

All hopes for the repr(transparent) in this case I guess. Btw, probably better to be discussed on that issue, but wouldn't it be easier / are there plans to just extend repr(u32) and such to newtypes containing integers? I imagine this would be faster to merge than completely new repr (even though it will cover only a small subset of cases for now).

@retep998
Copy link
Member

whereas C itself guarantees that such struct will be passed as a copy of its inner data.

There is definitely a difference between a C struct with a single field and the single field itself. Many calling conventions will distinguish between aggregates and primitives, even if they are the same size and alignment.

@alexcrichton
Copy link
Member

Ah yeah unfortunately as I think others have noted this isn't a bug that we can fix in rustc (yet). It sounds like #[repr(transparent)] will allow this pattern but in the meantime these two functions may or may not have the same literal register-passing mechanism across platforms:

struct Foo(u32);

extern {
    fn as_struct(a: Foo);
    fn as_u32(a: u32);
}

On some platforms (like linux x86) I believe these have the exact same assembly generated when calling, but as you've noticed other platforms (like emscripten) have different code generated. I think for something like arm and/or aarch64 they're also different, but I forget the precise details.

I don't think this is a bug for rustc, though, so closing. Let me know if I'm wrong though!

@RReverser
Copy link
Contributor Author

Yeah, thanks again everyone for elaboration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants