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

[NativeAOT-LLVM] Remove the workaround for CalendarData.EnumCalendarInfo #2095

Closed
SingleAccretion opened this issue Nov 18, 2022 · 2 comments
Labels
area-NativeAOT-LLVM LLVM generation for Native AOT compilation (including Web Assembly)

Comments

@SingleAccretion
Copy link

The functions need real implementations. Opening an issue for tracking purposes.

Code references:

// TODO-LLVM: look for a WASI random function; investigate ICU and EnumCalendarInfo
public static MethodIL ReplaceStubbedWasmMethods(MethodDesc method, MethodIL methodIL)
{
// stubs for Unix calls which are not available to this target yet
if ((method.OwningType as EcmaType)?.Name == "Interop" && method.Name == "GetRandomBytes")
{
// this would normally fill the buffer parameter, but we'll just leave the buffer as is and that will be our "random" data for now
return new ILStubMethodIL(method, new byte[] { (byte)ILOpcode.ret }, Array.Empty<LocalVariableDefinition>(), null);
}
if ((method.OwningType as EcmaType)?.Name == "CalendarData" && method.Name == "EnumCalendarInfo")
{
// just return false
return new ILStubMethodIL(method, new byte[] { (byte)ILOpcode.ldc_i4_0, (byte)ILOpcode.ret }, Array.Empty<LocalVariableDefinition>(), null);
}
return methodIL;
}

// mono does this via Javascript (pal_random.js), but prefer not to introduce that dependency as it limits the ability to run out of the browser.
// Copy the temporary workaround from the IL->LLVM generator for now.
if (!strcmp(mangledName, "S_P_CoreLib_Interop__GetRandomBytes"))
{
// this would normally fill the buffer parameter, but we'll just leave the buffer as is and that will be our "random" data for now
llvm::BasicBlock* llvmBlock = llvm::BasicBlock::Create(_llvmContext, "", _function);
_builder.SetInsertPoint(llvmBlock);
_builder.CreateRetVoid();
return;
}

@jkotas jkotas added the area-NativeAOT-LLVM LLVM generation for Native AOT compilation (including Web Assembly) label Nov 19, 2022
@SingleAccretion
Copy link
Author

SingleAccretion commented Jan 13, 2023

I am working on #2165 and the last warning to be dealt with is the missing symbol for dotnet_browser_entropy, which is referenced by System.Native now. I see two possible solutions:

1) Properly plumb the JS through our build and use it.
2) Add another workaround (either in compiler or System.Native) to silence the warning.

I like doing 1 now more because:
a) It means we get properly working random data.
b) The concern that we'll tie ourselves to JS-providing WASM hosts seems to no longer be as relevant, as upstream is adding support for WASI builds directly into System.Native, so, presumably, we'll be able to pick that up as well.

@yowl what do you think?

@yowl
Copy link
Contributor

yowl commented Jan 13, 2023

Yes, makes sense, i.e. point 1

@SingleAccretion SingleAccretion changed the title [NativeAOT-LLVM] Remove the workarounds for Interop.GetRandomBytes/CalendarData.EnumCalendarInfo [NativeAOT-LLVM] Remove the workaround for CalendarData.EnumCalendarInfo Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-LLVM LLVM generation for Native AOT compilation (including Web Assembly)
Projects
None yet
Development

No branches or pull requests

3 participants