Skip to content
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

fix(c#): strings code generation test #760

Merged
merged 3 commits into from
Dec 1, 2023

Conversation

jsturtevant
Copy link
Collaborator

@jsturtevant jsturtevant commented Nov 18, 2023

This fixes the code generation for the strings wit file. Re-enables a test disabled during #738.

I am much more unsure about this fix. There were there issues in the project:

  • exports where missing returnArea
  • imports had to many returnAreas
  • imports had out int length twice due multiple arguments

I am unsure if ReturnArea needs to be unique per imported function (same for outputs). update: Added a runtime test for string so feel much more confident.

Signed-off-by: James Sturtevant <[email protected]>
Signed-off-by: James Sturtevant <[email protected]>
@jsturtevant
Copy link
Collaborator Author

@silesmo @yowl I added the runtime test for the strings and its passing so I think this is correct

@SteveSandersonMS
Copy link

I tried this out and was able to get the code to work, both for supplying and receiving strings. Great job!

One thing that doesn't seem to work quite right is the current .NET implementation of cabi_realloc. It doesn't seem to start the runtime correctly. So, if you're exporting a function that accepts a string, and the very first call into your preview2 component is to that method, the very first thing it will do is call cabi_realloc which will fail with something like this:

Caused by:
    0: failed to invoke `run` function
    1: error while executing at wasm backtrace:
           0: 0x18fa7d7 - wit-component:shim!indirect-wasi:cli/[email protected]
           1: 0x18ef789 - wit-component:adapter:wasi_snapshot_preview1!wasi_snapshot_preview1::State::get_environment::h3923f56a626a4100
           2: 0x18ef996 - wit-component:adapter:wasi_snapshot_preview1!environ_sizes_get
           3: 0x18fa80d - wit-component:shim!adapt-wasi_snapshot_preview1-environ_sizes_get
           4: 0x145aac8 - <unknown>!__wasi_environ_sizes_get
           5: 0x145b358 - <unknown>!__wasilibc_initialize_environ
           6: 0x145b3f8 - <unknown>!__wasilibc_initialize_environ_eagerly
           7: 0xcc784c - <unknown>!__wasm_call_ctors
           8: 0xcc787f - <unknown>!_initialize
           9: 0xccb10c - <unknown>!InitializeRuntime()
          10: 0xccb4c6 - <unknown>!Thread::ReversePInvokeAttachOrTrapThread(ReversePInvokeFrame*)
          11: 0xccb5e1 - <unknown>!RhpReversePInvokeAttachOrTrapThread2
          12: 0xccb658 - <unknown>!RhpReversePInvoke
          13: 0x13a86eb - Adder_wit_computer_Intrinsics__cabi_realloc

As a workaround, if you first call any other export that only deals with int or other non-heap-objects, that will initialize the runtime correctly, and then cabi_realloc will no longer crash so you can then call exports that accept string.

@jsturtevant
Copy link
Collaborator Author

One thing that doesn't seem to work quite right is the current .NET implementation of cabi_realloc. It doesn't seem to start the runtime correctly. So, if you're exporting a function that accepts a string, and the very first call into your preview2 component is to that method, the very first thing it will do is call cabi_realloc which will fail with something like this:

Thanks for taking a look. This sounds like a bug we should address outside of this PR?

@SteveSandersonMS
Copy link

Thanks for taking a look. This sounds like a bug we should address outside of this PR?

Agreed - I was only reporting it here because this PR seems like the first scenario where the problem can manifest. Prior to this we were limited to working with .NET value types rather than heap objects, so cabi_realloc wasn't used.

Would you recommend this be reported as a general issue on this repo?

Also a further problem I found was that it becomes necessary to declare <IlcExportUnmanagedEntrypoints>true</IlcExportUnmanagedEntrypoints> if you have <OutputType>Exe</OutputType>, otherwise the compiler won't generate any export for cabi_realloc at all and therefore the component wouldn't satisfy the spec.

@jsturtevant
Copy link
Collaborator Author

I've created a few issues and linked in the overall tracking issue: #713

@jsturtevant jsturtevant merged commit acc9533 into bytecodealliance:main Dec 1, 2023
9 checks passed
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

Successfully merging this pull request may close these issues.

2 participants