Brendan Hansknecht (Sep 16 2024 at 23:14): Brendan Hansknecht (Sep 16 2024 at 23:14):I think I just realized part of the reason we have been having some any issues with tags being passed between rust and roc. Especially around types like RocResult<U64, ()>. We have been representing tag types roc in llvm and that is leading to them being passed incorrectly.
We represent that type as [0xi64, 1xi64, 1xi8, 7xi8]. That is 0xi64 to get the alignment correct. 1xi64 as the payload. 1xi8 as the tag, and 7xi8 as the padding.
The padding being represented in the type messes up c-abi. Let's ignore the 0xi64 for now, llvm doesn't actually admit anything for it to my knowledge.
1xi64, 1xi8 and 1xi64, 1xi8, 7xi8 are not compatible when it comes to c-abi. 1xi64, 1xi8 will be passed in two register. 1xi64, 1xi8, 7xi8 will be passed on the stack due to containing too many values to be passed in registers. So here and likely a few other places, we need to remove the explicit padding from our types. This will also be important in the rust glue. Representing padding explicitly is not valid and will change the c-abi of a type.
Brendan Hansknecht (Sep 16 2024 at 23:14):TLDR, we need to change our tag representation and it should fix some bugs we have been hitting
Brendan Hansknecht (Sep 16 2024 at 23:15):Makes sense why we have been hitting so many confusing bugs since we change to tasks everywhere a lot more. Task SomeType {} that is being passed incorrectly via c-abi currently.
Brendan Hansknecht (Sep 17 2024 at 00:13):This is at least the core of the fix: https://github.com/roc-lang/roc/pull/7084
I am still seeing issues with nqueens in false interpreter, but I'm not sure the cause at this point. Assuming this passes tests, I think it should be good to merge.
Brendan Hansknecht (Sep 17 2024 at 00:20):Oh, I see the next issue:
For u8, u8 which is what is generated by Task U8 {}, Rust is packing both u8 into a single register w0. Roc is expecting them to be put in two registers, w0 and w1....not sure why yet
Brendan Hansknecht (Sep 17 2024 at 00:24):Hmm, maybe it is due to representing the type as { [1xi8], i8} in llvm instead of {i8, i8}. Not sure the best way to fix that....
Brendan Hansknecht (Sep 17 2024 at 00:24):Guess I need to mess around more
Brendan Hansknecht (Sep 17 2024 at 00:58):Even if I change rust to use a [1xi8], it still seems to be packing everything into a single register while llvm with roc is not.... This may be a case where we need more special handling cause llvm doesn't fully handle abi......
Brendan Hansknecht (Sep 17 2024 at 01:01):Luckily an easy workaround is just to make the type bigger Task U64 {} works fine and doesn't pack into a single register, but I should try to figure out a more proper fix.
Brendan Hansknecht (Sep 17 2024 at 01:02):Oh, I think we may be using llvm cc instead of the c calling convention here.
Brendan Hansknecht (Sep 17 2024 at 01:03):Cause it is faster to pass in two regs than pack into one and unpack on the other side
Brendan Hansknecht (Sep 17 2024 at 01:05):hmm...but we set the call conv to c. So that shouldn't be it. But maybe we need to manually deal with something here. Not fully sure.
Brendan Hansknecht (Sep 17 2024 at 01:22):@Luke Boswell, I think https://github.com/roc-lang/roc/pull/7084 fixes your issues with https://github.com/roc-lang/basic-webserver/pull/72
Richard Feldman (Sep 17 2024 at 01:23):I thought at some point @Folkert de Vries and I were looking at an ABI issue and concluded that maybe we couldn't have a RocResult abstraction in Rust and instead needed to have glue generate a tag union every time, but I don't remember what the details were anymore :sweat_smile:
Brendan Hansknecht (Sep 17 2024 at 01:25):I can think of 2 issues:
[] for the err type instead of {}). Cause there will be no tag in that case. Brendan Hansknecht (Sep 17 2024 at 01:25):But neither of those should apply here.
Brendan Hansknecht (Sep 17 2024 at 01:28):For the issue of Result U8 {} being passed in a single register by rust, but two registers by our llvm ir (at least that is what looks to be happening from what I can tell), it may be a case that we need to manually pack certain types when sending things over c abi.
Brendan Hansknecht (Sep 17 2024 at 01:29):So we may need to convert the llvm type from {[1xi8], i8} to i16. Same with any other cases that the c abi would pack into a single register.
Brendan Hansknecht (Sep 17 2024 at 01:30):But that is just a guess from what seems to be happening
Luke Boswell (Sep 17 2024 at 01:36):Nice work Brendan!
Brendan Hansknecht (Sep 17 2024 at 01:40):Yeah, it looks like we don't handle these for aggregate types:
Types less than or equal to 8 bytes are returned in x0.
Types less than or equal to 16 bytes are returned in x0 and x1, with x0 containing the lower-order 8 bytes.
Brendan Hansknecht (Sep 17 2024 at 01:41):So more c call conv cases that aren't covered by llvm and we need to do manually.
Brendan Hansknecht (Sep 17 2024 at 01:59):I'll fix this in a different PR. How things are wired currently, this feels like more of a hassle to fix than I want to deal with at this exact moment. Nothing specifically, hard, but a solid bit of wiring.
Brendan Hansknecht (Sep 17 2024 at 02:03):How this impacts things in practice: Anything that returned from the host, is less than 9 bytes, and is a Task SomeOk SomeErr, probably won't work (at least not on all architectures). Also, if it is less than 17 bytes, it may have issues but that is less likely to be problematic.
So Task U8 {} fails for example, but can be worked around in rust by changing the type to RocResult<u64, ()>.
This is a pre-existing bug, just made obvious and much more common by the Task as builtin PR.
Richard Feldman (Sep 17 2024 at 02:06):great find! :smiley:
Richard Feldman (Sep 17 2024 at 02:07):I guess sending enumerations to the host would run into this
Richard Feldman (Sep 17 2024 at 02:07):because they're U8 under the hood
Luke Boswell (Sep 17 2024 at 02:08):I can have a closer look tonight. This is awesome. Looking forward to JWT in basic-webserver :big_smile:
Brendan Hansknecht (Sep 17 2024 at 02:09):Yeah, this will be hit my anything that is a return type from the host and is an aggregate type under 9/17 bytes depending on the architecture. So a { a: U8, b: U8} would also hit this.
Luke Boswell (Sep 17 2024 at 09:27):Looks like it's working :tada: -- just testing on that JWT PR for basic-webserver
Last updated: Jul 06 2025 at 12:14 UTC