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

Some FSharp.Core constructs don't run on CoreRT #4954

Open
zpodlovics opened this Issue May 20, 2018 · 27 comments

Comments

Projects
None yet
6 participants
@zpodlovics

zpodlovics commented May 20, 2018

I am experimenting with porting some of the F# testsuite cases to CoreRT (dotnet/corert#2057 (comment), dotnet/corert#5827) However right now some of the Core F# modules does not support code paths without dynamic code generation. For example the F# test suite written in a style that contains lot's of printf and sprintf code (egrep -r "printf" tests/|wc -l will found more than 10k cases). Earlier I intentionally avoided printf in the code bases that may also used in fully static compilation (Mono Full AOT). Due the large number of usage everywhere (eg.: testsuite) rewrite the existing printf/sprintf usage to System.String.Format and System.Console.WriteLine would be a huge waste of time.

Please note, some earlier test cases was carefully designed and intentionally avoided this issue by factoring out the printing functionality to the beginning of the test code:

let failures = ref []

let report_failure (s : string) = 
    stderr.Write" NO: "
    stderr.WriteLine s
    failures := !failures @ [s]

let test (s : string) b = 
    stderr.Write(s)
    if b then stderr.WriteLine " OK"
    else report_failure (s)

Is there any earlier Printf module version that could be used for this purpose - but without dynamic code generation (performance does not matter in this case)?

By dynamic code generation I mean code snippets like this:

        | 'A' ->
            let mi = typeof<ObjectPrinter>.GetMethod("GenericToString", NonPublicStatics)
            let mi = mi.MakeGenericMethod(ty)
            mi.Invoke(null, [| box spec |])

https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/printf.fs#L1170

Repro steps

printf.zip

cd printf
dotnet publish -c Release -r linux-x64
./bin/Release/netcoreapp2.0/linux-x64/publish/printf

Expected behavior

Core modules should also work without dynamic code generation path (as a fallback option).

Actual behavior

Due the dynamic code generation the Printf module does not work with CoreRT.

Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.FSharp.Core.PrintfImpl.PrintfBuilder`3.buildPlainFinal(Object[], Type[]) + 0xbd
   at Microsoft.FSharp.Core.PrintfImpl.PrintfBuilder`3.Build[T](String) + 0x61
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey, Func`2) + 0x83
   at Microsoft.FSharp.Core.PrintfImpl.Cache`4.Get(PrintfFormat`4) + 0x65
   at Microsoft.FSharp.Core.PrintfModule.PrintFormatToTextWriterThen[TResult, T](FSharpFunc`2, TextWriter, PrintfFormat`4) + 0x21
   at Program.main(String[]) + 0x3a
   at printf!<BaseAddress>+0x209f83

Aborted

Known workarounds

Rewrite everything to System.Console.WriteLine and System.String.Format or write Printf module without dynamic codegen.

Related information

  • Operating system: Ubuntu 16.04
  • .NET Runtime, CoreCLR or Mono Version:
ii  dotnet-host                                                 2.1.0-rc1-1                                                 amd64        Microsoft .NET Core Host - 2.1.0 Release Candidate 1
ii  dotnet-hostfxr-1.1.0                                        1.1.0-1                                                     amd64        Microsoft .NET Core 1.1.3 - Host FX Resolver 1.1.0
ii  dotnet-hostfxr-2.0.5                                        2.0.5-1                                                     amd64        Microsoft .NET Core Host FX Resolver - 2.0.5 2.0.5
ii  dotnet-hostfxr-2.0.6                                        2.0.6-1                                                     amd64        Microsoft .NET Core Host FX Resolver - 2.0.6 2.0.6
ii  dotnet-hostfxr-2.1.0-rc1                                    2.1.0-rc1-1                                                 amd64        Microsoft .NET Core Host FX Resolver - 2.1.0 Release Candidate 1 2.1.0-rc1
ii  dotnet-runtime-2.0.5                                        2.0.5-1                                                     amd64        Microsoft .NET Core Runtime - 2.0.5 Microsoft.NETCore.App 2.0.5
ii  dotnet-runtime-2.0.6                                        2.0.6-1                                                     amd64        Microsoft .NET Core Runtime - 2.0.6 Microsoft.NETCore.App 2.0.6
ii  dotnet-runtime-2.1.0-rc1                                    2.1.0-rc1-1                                                 amd64        Microsoft .NET Core Runtime - 2.1.0 Release Candidate 1 Microsoft.NETCore.App 2.1.0-rc1
ii  dotnet-runtime-deps-2.1.0-rc1                               2.1.0-rc1-1                                                 amd64        dotnet-runtime-deps-2.1.0-rc1 2.1.0-rc1
ii  dotnet-sdk-2.1.4                                            2.1.4-1                                                     amd64        Microsoft .NET Core SDK - 2.1.4
ii  dotnet-sharedframework-microsoft.netcore.app-1.1.8          1.1.8-1                                                     amd64        Microsoft .NET Core 1.1.8 - Runtime Microsoft.NETCore.App 1.1.8

@dsyme

This comment has been minimized.

Contributor

dsyme commented May 21, 2018

Well, CoreRT is not a complete implementation of .NET. And TBH is very difficult to design, implement and test .NET libraries that run on incomplete implementations of .NET. Also, historically, incomplete implementations of .NET have often eventually been forced to "complete" the implementation w.r.t. the ECMA standards, e.g. by providing backup implementations of generic code that doesn't require further code generation at runtime. For example Mono on Android and iOS. At least to the extent that MakeGenericType and MakeGenericMethod are supported (which is the limit of the code generation that we do).

If .NET provides official ways to conditionally dynamically probe for an official, documented, well-defined set of implementation capabilities then we could use those. At the moment I don't think there is a mechanism to do that - is there any official guidance on this?

@dsyme dsyme changed the title from F# Core modules should support non-dynamic code generation fallback at least for basic functionality eg.: Printf module to Some FSharp.Core constructs don't run on CoreRT May 21, 2018

@charlesroddie

This comment has been minimized.

charlesroddie commented May 21, 2018

I think we should narrow down this issue. It is about the usage of printf & friends in the F# test suite causing problems testing CoreRT (and presumably other AOT compilers).

From browsing the F# test suite, the implementations of printf etc. are generally not themselves being tested. They are only helping to log and debug errors.

To get CoreRT testing going using the F# test suite, we could replace printf etc. with ignore functions. E.g. replacing usage of printf etc. in the test suite with OutOfInk.printf etc. with the natural defintions. Or using a customized FSharp.Core with those modifications.

Does this make sense?

@dsyme

This comment has been minimized.

Contributor

dsyme commented May 21, 2018

Printf is tested in

visualfsharp\tests\fsharp\core\printf
visualfsharp\tests\FSharp.Core.UnitTests\FSharp.Core\Microsoft.FSharp.Core\Printf.fs

To get CoreRT testing going using the F# test suite, we could replace printf etc. with ignore functions. E.g. replacing usage of printf etc. in the test suite with OutOfInk.printf etc. with the natural defintions. Or using a customized FSharp.Core with those modifications.

Yes, for the rest of the test suite perhaps a naive implementation of printf restricted to 10-20 basic format strings would do to get started. You'll need an implementation of sprintf too likewise. Then just #load that implementation or otherwise include it in the test suite.

It would, however, be even better if CoreRT just implemented .NET properly. Can CoreRT cope with typ.MakeGenericType(...) or does it incorrectly implement that too?

@dsyme

This comment has been minimized.

Contributor

dsyme commented May 21, 2018

and presumably other AOT compilers

My understanding is that the Mono AOT compiler doesn't suffer this problem (depending on configuration), because a "last restort" representation of generic code is produced that uses some kind of wide/tagged-representation for naked T values on the stack. This means dynamic instantiations of existing generic code is ok, but dynamic generation of new code is not. That's an important distinction, and FSharp.Core does the former but not the latter.

That's how Mono ends up running on so many devices and it's fundamental to Xamarin. I don't know the details however.

@charlesroddie

This comment has been minimized.

charlesroddie commented May 22, 2018

It looks like there is relevant active work in CoreRT: dotnet/corert#5011

However we shouldn't get sidetracked with getting printf to work. We should just find a way to work around it.

@zpodlovics Can you just copy
https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/printf.fs , replace 1170 and 1174 with .ToString() , and use that when testing?

@MichalStrehovsky

This comment has been minimized.

Member

MichalStrehovsky commented May 22, 2018

Just to give some details about CoreRT's restriction on MakeGenericType/MakeGenericMethod:

  • The main problem is lack of pregenerated code for the particular generic instantiation (we can build other supporting data structures such as type descriptors on the fly)
  • RD.XML (a file passed to the CoreRT compiler) can be used to tell the compiler that particular code needs to be generated (even though it statically looks like it isn't needed). If there's a reasonable bound on what MakeGenericType/MakeGenericMethod gets called with, RD.XML is all that's needed to make this work.
  • Generic code for instantiations that are mostly the same can be shared ("mostly the same" means that e.g. a method that takes a T as a parameter will have the same calling convention (e.g. the T is passed in a CPU register), GC reporting, etc.). This is specifically useful for instantiations over reference types (e.g. methods on List<object> and List<string> will be actually backed by the same code). So if MakeGenericType is only ever called with reference types, you just need to add one instantiation over a reference type to RD.XML and the problem is gone, because we'll have the code, and building the type itself at runtime is not a problem for CoreRT.
@zpodlovics

This comment has been minimized.

zpodlovics commented May 22, 2018

@MichalStrehovsky Thanks for the ideas! Actually, I did try to do pregenerated code with rd.xml for printf module but without success (I did try to use earlier too, when I did the F# helloworld project).

Unfortunately it's not yet clear to me to what assembly / types should I specify (and why!) for method generic calls as MethodInstantiation arguments in rd.xml

Eg.:

The following functions / methods use MakeGenericMethod:

module internal PrintfImpl =
    let private getValueConverter (ty : Type) (spec : FormatSpecifier) : obj = 

Generated IL for method signature:

 // method line 346
    .method assembly static 
           default object getValueConverter (class [mscorlib]System.Type ty, class Microsoft.FSharp.Core.PrintfImpl/FormatSpecifier spec)  cil managed 
    {
type private PrintfBuilder<'S, 'Re, 'Res>() =
    let buildSpecialChained(spec : FormatSpecifier, argTys : Type[], prefix : string, tail : obj, retTy) = 

Generated IL for method singature:

.method assembly static 
           default object buildSpecialChained$cont@1321<S,Re,Res> (class Microsoft.FSharp.Core.PrintfImpl/FormatSpecifier spec, class [mscorlib]System.Type[] argTys, string prefix, object tail, class [mscorlib]System.Type retTy, class [FSharp.Core]Microsoft.FSharp.Core.Unit unitVar)  cil managed 
    {
type private PrintfBuilder<'S, 'Re, 'Res>() =
    let buildPlainFinal(args : obj[], argTypes : Type[]) =

Generated IL for method signature:

.method assembly hidebysig 
           instance default object buildPlainFinal (object[] args, class [mscorlib]System.Type[] argTypes)  cil managed 
    {
type private PrintfBuilder<'S, 'Re, 'Res>() =
    let buildPlainChained(args : obj[], argTypes : Type[]) =

Generated IL for method signature:

    .method assembly hidebysig 
           instance default object buildPlainChained (object[] args, class [mscorlib]System.Type[] argTypes)  cil managed 
    {

Here is my rd.xml

<Directives>
    <Application>
        <Type Name="Microsoft.FSharp.Core.PrintfImpl">
            <MethodInstantiation Name="getValueConverter" Arguments="System.Type,Microsoft.FSharp.Core.PrintfImpl/FormatSpecifier" Dynamic="Required All">
        </Type>  
        <Type Name="Microsoft.FSharp.Core.PrintfImpl/PrintfBuilder`3">
            <MethodInstantiation Name="buildSpecialChained" Arguments="Microsoft.FSharp.Core.PrintfImpl/FormatSpecifier,System.Type[],System.String,System.Object,System.Type,Microsoft.FSharp.Core.Unit" Dynamic="Required All">
            <MethodInstantiation Name="buildPlainFinal" Arguments="System.Object[],System.Type[]" Dynamic="Required All">
            <MethodInstantiation Name="buildPlainChained" Arguments="System.Object[],System.Type[]" Dynamic="Required All">            
        </Type>                              
    </Application>
</Directives>

What should I exactly (and why?) specify in rd.xml ?

Example attached:
printf_rdxml.zip

It still fails with the following message when I ran:

Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.FSharp.Core.PrintfImpl.PrintfBuilder`3.buildPlainFinal(Object[], Type[]) + 0xbd
   at Microsoft.FSharp.Core.PrintfImpl.PrintfBuilder`3.Build[T](String) + 0x61
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey, Func`2) + 0x83
   at Microsoft.FSharp.Core.PrintfImpl.Cache`4.Get(PrintfFormat`4) + 0x65
   at Microsoft.FSharp.Core.PrintfModule.PrintFormatToTextWriterThen[TResult, T](FSharpFunc`2, TextWriter, PrintfFormat`4) + 0x21
   at Program.main(String[]) + 0x3a
   at printf!<BaseAddress>+0x209faf

Aborted

Printf module source code:
https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/printf.fs#L1060

Printf module also use the following library:
https://github.com/Microsoft/visualfsharp/blob/master/src/utils/sformat.fs

@MichalStrehovsky

This comment has been minimized.

Member

MichalStrehovsky commented May 22, 2018

I see you're using the actual documented RD.XML syntax here that the .NET Native for Universal Windows Apps uses - that's not quite the syntax the temporary RD.XML parser in CoreRT uses. The fact that you're not getting a compiler failure also indicates the compiler is not actually looking at this file. We track full reflection story in dotnet/corert#5001 including figuring out the right way to express compilation roots.

  • Make sure you include the file in your RD.XML in the project, like so.
  • The syntax used for Type is ECMA-335 SerString format of the type (we had a parser lying around because it's used elsewhere, so I just reused it). Sample RD.XML that uses it is here. Generic methods can be referenced this way.
@zpodlovics

This comment has been minimized.

zpodlovics commented May 22, 2018

@MichalStrehovsky Thank you for the ideas! I did some experiment with the aspnet sample earlier this year, but I did not checked the different rd.xml before.

The updated rd.xml and checked with this:

<Directives>
    <Application>
        <Assembly Name="FSharp.Core" Dynamic="Required All" />
    </Application>
</Directives>

And later with this:

<Directives>
    <Application>
        <Assembly Name="FSharp.Core" Dynamic="Required All">
            <Type Name="Microsoft.FSharp.Core.PrintfImpl" Dynamic="Required All" />
            <Type Name="Microsoft.FSharp.Core.PrintfImpl/PrintfBuilder`3[[TYPE1,TYPE2,TYPE3]]" Dynamic="Required All" />
        </Assembly>
    </Application>
</Directives>

Until I have a working trivial example compling in the whole FSharp.Core is perfectly fine (the compiled binary will be big). This could be trim down later. However figuring out the generic type type arguments looks harder than it seems - because according to the comments in the code this may depends on the program input format string and the inferred type by the compiler. Please correct me if I am wrong, but based on the comment and the generated IL actual type argument here could be a deeply nested generic tuple / tree-like structure (from code comment :" First it recursively consumes format string up to the end, then during unwinding builds printer using PrintfBuilderStack as storage for arguments.") . I guess unless some tool will be developed to analyze the compiled IL code and extract the actual type instantiation information and generate the rd.xml automatically, because doing it manually seems unfeasible ...

    /// Basic idea of implementation:
    /// Every Printf.* family should returns curried function that collects arguments and then somehow prints them.
    /// Idea - instead of building functions on fly argument by argument we instead introduce some predefined parts and then construct functions from these parts
    /// Parts include:
    /// Plain ones:
    /// 1. Final pieces (1..5) - set of functions with arguments number 1..5. 
    /// Primary characteristic - these functions produce final result of the *printf* operation
    /// 2. Chained pieces (1..5) - set of functions with arguments number 1..5. 
    /// Primary characteristic - these functions doesn not produce final result by itself, instead they tailed with some another piece (chained or final).
    /// Plain parts correspond to simple format specifiers (that are projected to just one parameter of the function, say %d or %s). However we also have 
    /// format specifiers that can be projected to more than one argument (i.e %a, %t or any simple format specified with * width or precision). 
    /// For them we add special cases (both chained and final to denote that they can either return value themselves or continue with some other piece)
    /// These primitives allow us to construct curried functions with arbitrary signatures.
    /// For example: 
    /// - function that corresponds to %s%s%s%s%s (string -> string -> string -> string -> string -> T) will be represented by one piece final 5.
    /// - function that has more that 5 arguments will include chained parts: %s%s%s%s%s%d%s  => chained2 -> final 5
    /// Primary benefits: 
    /// 1. creating specialized version of any part requires only one reflection call. This means that we can handle up to 5 simple format specifiers
    /// with just one reflection call
    /// 2. we can make combinable parts independent from particular printf implementation. Thus final result can be cached and shared. 
    /// i.e when first call to printf "%s %s" will trigger creation of the specialization. Subsequent calls will pick existing specialization

Printf example snippet:

printfn "argv: %d" argv.Length
    /// Parses format string and creates result printer function.
    /// First it recursively consumes format string up to the end, then during unwinding builds printer using PrintfBuilderStack as storage for arguments.
    /// idea of implementation is very simple: every step can either push argument to the stack (if current block of 5 format specifiers is not yet filled) 
    //  or grab the content of stack, build intermediate printer and push it back to stack (so it can later be consumed by as argument) 
    type private PrintfBuilder<'S, 'Re, 'Res>() =

PrintfBuilder instance creation:

    type Cache<'T, 'State, 'Residue, 'Result>() =
        static let generate(fmt) = PrintfBuilder<'State, 'Residue, 'Result>().Build<'T>(fmt)        

Type instatiation I found for Microsoft.FSharp.Core.CoreRT.PrintfImpl/PrintfBuilder`3 in generated IL for the trivial printf example:

    type Cache<'T, 'State, 'Residue, 'Result>() =
        static let generate(fmt) = PrintfBuilder<'State, 'Residue, 'Result>().Build<'T>(fmt)        

Generated IL code:

 .method assembly static 
           default class [mscorlib]System.Tuple`2<class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<class [FSharp.Core]Microsoft.FSharp.Core.Unit, class Microsoft.FSharp.Core.PrintfImpl/PrintfEnv`3<!State, !Residue, !Result>>, !T>, int32> generate (string fmt)  cil managed 
    {
        .custom instance void class [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::'.ctor'() =  (01 00 00 00 ) // ....

        // Method begins at RVA 0x1abfc
	// Code size 12 (0xc)
	.maxstack 8
	IL_0000:  newobj instance void class Microsoft.FSharp.Core.PrintfImpl/PrintfBuilder`3<!State, !Residue, !Result>::'.ctor'()
	IL_0005:  ldarg.0 
	IL_0006:  callvirt instance class [mscorlib]System.Tuple`2<class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<class [FSharp.Core]Microsoft.FSharp.Core.Unit,class Microsoft.FSharp.Core.PrintfImpl/PrintfEnv`3<!0,!1,!2>>,!!0>,int32> class Microsoft.FSharp.Core.PrintfImpl/PrintfBuilder`3<!State, !Residue, !Result>::Build<!0> (string)
	IL_000b:  ret 
    } // end of method Cache`4::generate
@MichalStrehovsky

This comment has been minimized.

Member

MichalStrehovsky commented May 25, 2018

This kind of patterns is the reason why F# is problematic to support in AoT compilers in a performant way.

In an environment that has a JIT, MakeGenericType/MakeGenericMethod and subsequent uses of that code come with a small penalty to JIT-compile the new code (plus a penalty for having to keep the code around in memory, unless the runtime knows how to unload code that hasn't been used in a while).

In an AoT compiler, the code needs to be pregenerated. We either pregenerate code that is efficient (specialized for the given T), but there will be a size on disk penalty from doing that (plus the hassle of having to specify all the possibilities in advance), or we generate code that is "universal", but pretty inefficient at runtime (.NET Native compiler for Windows UWP apps can already do that, and I assume so does Mono; the CoreRT compiler can't do that for now, so it's not an option there). Such universal code is slower and allocates more things on the GC heap than what one would expect.

@zpodlovics

This comment has been minimized.

zpodlovics commented May 25, 2018

A bit old now, but still related publication from @dsyme: Pre-compilation for .NET Generics http://citeseerx.ist.psu.edu/viewdoc/download?rep=rep1&type=pdf&doi=10.1.1.124.3911

@MichalStrehovsky If I remember correctly CoreRT try to solve these pre-generation problems by resurrecting the IL interpreter. Is there any other way exists to solve this more efficiently? Is it event possible to solve more efficiently?

However it seems OCaml was able to solve the typed printing/formatting problem efficiently with their implementation of printf and runtime both in bytecode format and both in native format.

@zpodlovics

This comment has been minimized.

zpodlovics commented May 30, 2018

It seems that F# OCaml compatibility pack printf will not solve the printf AOT problem due the missing format string implementation:

// NOTE : The types and functions in this section
// are already available in the F# Core Library (FSharp.Core).

https://github.com/fsprojects/FSharp.Compatibility/blob/master/FSharp.Compatibility.OCaml/Pervasives.fs#L1297

@dsyme

This comment has been minimized.

Contributor

dsyme commented May 30, 2018

This kind of patterns is the reason why F# is problematic to support in AoT compilers in a performant way.

The code patterns are not so different from many C# ones. Honestly all .NET implementations that use the name ".NET" should implement these correctly before going to V1, otherwise they are not a correct implementation of .NET

Yes, Mono's compilation to iOS appears to handle these cases reasonably performantly through the generation of wide code. I don't know the full details, but it is at least a correct and complete story.

@FoggyFinder

This comment has been minimized.

FoggyFinder commented Jun 14, 2018

@zpodlovics for simple printfn like

let sayHello name = printfn "Hello, %s!" name

the next rd works fine to me:

        <Assembly Name="FSharp.Core" Dynamic="Required All"> 
            <Type Name="Microsoft.FSharp.Core.PrintfImpl+Specializations`3[[System.Object,System.Private.CoreLib],[System.Object,System.Private.CoreLib],[System.Object,System.Private.CoreLib]]" Dynamic="Required All">
                <Method Name="Final1" Dynamic="Required">
                    <GenericArgument Name="System.Object, System.Private.CoreLib" />
                </Method> 
                <Method Name="FinalFastEnd1" Dynamic="Required">
                    <GenericArgument Name="System.Object, System.Private.CoreLib" />
                </Method> 
                <Method Name="FinalFastStart1" Dynamic="Required">
                    <GenericArgument Name="System.Object, System.Private.CoreLib" />
                </Method> 
                <Method Name="FinalFast1" Dynamic="Required">
                    <GenericArgument Name="System.Object, System.Private.CoreLib" />
                </Method> 
                <Method Name="Final2" Dynamic="Required">
                    <GenericArgument Name="System.Object, System.Private.CoreLib" />
                    <GenericArgument Name="System.Object, System.Private.CoreLib" />
                </Method> 
                <Method Name="FinalFastEnd2" Dynamic="Required">
                    <GenericArgument Name="System.Object, System.Private.CoreLib" />
                    <GenericArgument Name="System.Object, System.Private.CoreLib" />
                </Method>
            </Type> 
        </Assembly>

for full use printfn it seems like we need specify all methods of types that are called through MakeGenericMethod

@zpodlovics

This comment has been minimized.

zpodlovics commented Jun 14, 2018

@FoggyFinder Thanks for the idea, but did not work on linux with 1.0.0-alpha-26614-02.

  System.Exception: Exception of type 'System.Exception' was thrown.
     at ILCompiler.RdXmlRootProvider.AddCompilationRoots(IRootingServiceProvider rootProvider)
     at ILCompiler.Compilation..ctor(DependencyAnalyzerBase`1 dependencyGraph, NodeFactory nodeFactory, IEnumerable`1 compilationRoots, DebugInformationProvider debugInformationProvider, DevirtualizationManager devirtualizationManager, Logger logger)
     at ILCompiler.ILScannerBuilder.ToILScanner()
     at ILCompiler.Program.Run(String[] args)
     at ILCompiler.Program.Main(String[] args)

Unfortunately except the highly specialized few cases - you'll probably not be able to specify the (deeply nested) instantiation types especially not for %A, take a look at the generated IL signature at the end of this comment: #4954 (comment)

@FoggyFinder

This comment has been minimized.

FoggyFinder commented Jun 14, 2018

Thanks for the idea, but did not work on linux with 1.0.0-alpha-26614-02.

I got the same exception for another parameter of printfn, but I was able to avoid this with using another methods of Specializations and of ObjectPrinter

Unfortunately except the highly specialized few cases - you'll probably not be able to specify the (deeply nested) instantiation types especially not for %A, take a look at the generated IL signature at the end of this comment:

I think it requires including only two types - Specializations and ObjectPrinter. Maybe I continue investigate it tomorrow.

@FoggyFinder

This comment has been minimized.

FoggyFinder commented Jun 14, 2018

@zpodlovics can you check this sample?

Does it get the exception?

@zpodlovics

This comment has been minimized.

zpodlovics commented Jun 15, 2018

@FoggyFinder Nice trick, your example project compiles and runs (using linux-x64).

However how this supposed to be type (and crash) safe when the generic argument instantiation types forced to be objects? Reference types have shared code for all generic instantiation, so it supposed to works for reference types. But how about the cases for value type generic instantiations?
https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/printf.fs#L220

These type instantiations will be interesting:
https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/printf.fs#L622
https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/printf.fs#L701
https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/printf.fs#L714
https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/printf.fs#L726
https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/printf.fs#L739

I have an out of tree printf module for experiments, I may try add some reference type constraints to these generic methods later. It may help to reveal the paths for the not yet handled value type generic instantiations.

@FoggyFinder

This comment has been minimized.

FoggyFinder commented Jun 15, 2018

Reference types have shared code for all generic instantiation, so it supposed to works for reference types.

Not sure yet how it will handle custom types, I thought System.Object will be enough for reference types, but my simple test have printed empty brackets.

But how about the cases for value type generic instantiations?

Sadly, probably we have to specify all basic types separately.

Well, if someone will write script for generating rd it will be awsome. (I not so good for code analysis). I can only create some helper functions to avoid boring manuall work.

@zpodlovics

This comment has been minimized.

zpodlovics commented Jun 15, 2018

#@FoggyFinder Yes, but I am afraid solving the generic instantiation types to generate rd.xml means reimplementing printf module again. In theory printf could generate some metadata about the static instantiation types created during the specialization, but will still not work with %A (the actual argument types may depend on the program execution eg: if someComplicatedFunction then p o else p v1).

open System

type [<Class>] O = class end
type [<Struct>] V1 = struct end
type [<Struct>] V2 = struct end

[<EntryPoint>]
let main argv =
    let p v = printfn "string: %A" v
    let o = Unchecked.defaultof<O>
    let v1 = Unchecked.defaultof<V1>
    let v2 = Unchecked.defaultof<V2>   

    p o
    p v1
    p v2

    0 // return an integer exit code

.NET Core Run:

string: <null>
string: Program+V1
string: Program+V2

.NET CoreRT Run:

Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.FSharp.Core.PrintfImpl.getValueConverter(Type, PrintfImpl.FormatSpecifier) + 0x515
   at Microsoft.FSharp.Core.PrintfImpl.PrintfBuilder`3.parseFromFormatSpecifier(String, String, Type, Int32) + 0x4b6
   at Microsoft.FSharp.Core.PrintfImpl.PrintfBuilder`3.parseFormatString(String, Type) + 0x99
   at Microsoft.FSharp.Core.PrintfImpl.PrintfBuilder`3.Build[T](String) + 0x61
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey, Func`2) + 0x78
   at Microsoft.FSharp.Core.PrintfImpl.Cache`4.Get(PrintfFormat`4) + 0x65
   at Microsoft.FSharp.Core.PrintfModule.PrintFormatToTextWriterThen[TResult, T](FSharpFunc`2, TextWriter, PrintfFormat`4) + 0x21
   at Program.p@14[a](a) + 0x40
   at Program.main(String[]) + 0x46
   at printf!<BaseAddress>+0x318e43

Aborted

@FoggyFinder

This comment has been minimized.

FoggyFinder commented Jun 15, 2018

but will still not work with %A

rd

            <Type Name="Microsoft.FSharp.Core.PrintfImpl+ObjectPrinter" Dynamic="Required All">
                <Method Name="GenericToString" Dynamic="Required">
                    <GenericArgument Name="System.Int32, System.Private.CoreLib" />
                </Method>  
                <Method Name="GenericToString" Dynamic="Required">
                    <GenericArgument Name="System.Double, System.Private.CoreLib" />
                </Method>  
                <Method Name="GenericToString" Dynamic="Required">
                    <GenericArgument Name="System.Object, System.Private.CoreLib" />
                </Method>  
            </Type>  

.Core RT:

    printfn "%A" [| 10..20 |] // [|10; 11; 12; 13; 14; 15; 16; 17; 18; 19; 20|]
    printfn "%A" 42 //42
    printfn "%A" 25.0 //25.0
    printfn "%A" "string" //"string"
@zpodlovics

This comment has been minimized.

zpodlovics commented Jun 15, 2018

@FoggyFinder It's not accidental that I created my own value types in the example. Yes, you can list the base value types there, and you can also list all value types from F# and from the .NET class libraries, and all the NuGet packages, and so on.

However due the closed world assumption the CoreRT will know all the value types.

@MichalStrehovsky Would it be possible to have something like this to express all the known value types in the rd.xml (at least as a workaround because it will create some code bloat)?

<Type Name="Microsoft.FSharp.Core.PrintfImpl+ObjectPrinter" Dynamic="Required All">
            <Method Name="GenericToString" Dynamic="Required">
                    <ValueTypes Filter="None" />
            </Method>            
 </Type>  
@MichalStrehovsky

This comment has been minimized.

Member

MichalStrehovsky commented Jun 15, 2018

@MichalStrehovsky Would it be possible to have something like this to express all the known value types in the rd.xml (at least as a workaround because it will create some code bloat)?

This will still not help if the value type itself is generic, e.g. KeyValuePair<TKey,TValue> (do we instantiate with all the other value types?). Not sure how common that would be in your scenario.

Still, you might be surprised how much code that will generate.

If you create a pull request that adds the syntax to RdXmlRootProvider, I wouldn't push back too hard against it. But do note that all of the RD.XML processing code is only temporary, like the warning says. At some point in the future, we'll want to replace it with something sustainable.

@charlesroddie

This comment has been minimized.

charlesroddie commented Jul 11, 2018

Message from @dsyme : you can submit a PR to remove the use of sprintf throughout FSharp.Core.UnitTests except where that's the thing explicitly being tested. It's useful anyway to reduce the complexity of what's being executed for different tests.

@zpodlovics

This comment has been minimized.

zpodlovics commented Jul 16, 2018

It seems that %A, %O is usually used to do string based structural comparison in the tests. Can we assume in the testsuite that the F# structural comparison is correct (except tests that target the structural comparison itself)? So instead of using printf generated strings to do structural comparison we can use the F# compiler to do structural comparison eg.: by defining a few specific F# record type. However these f# structural comparisons works without dynamically generated code.

I also have a few examples to remove sprintf and add CoreRT build support:
master...zpodlovics:remove-printf-from-fsharp-core-unit-tests

CoreRT is still work in progress (~alpha) not-yet released product, the F# development / testing should not held back by CoreRT issues. It's not yet clear how the CoreRT support should be added to the automatical builds and to the test runner. However right now it should work independently and disabled by default from the main testsuite as the CoreRT native builds will require platform specific tools installed and the test runner should be able to execute standalone native executable.

@charlesroddie

This comment has been minimized.

charlesroddie commented Jul 16, 2018

Can we assume in the testsuite that the F# structural comparison is correct (except tests that target the structural comparison itself)? So instead of using printf generated strings to do structural comparison we can use the F# compiler to do structural comparison

That makes sense to me. Can you put a sample change here that could be a template?

The function definitions don't seem to provide much benefit to me. The ones that get used many times yes, but the ones that only get used once no. It should be OK to use them, also OK not to.

Can we divide and conquer on this? Alphabetical and reverse alphabetical? Please add to the PR if you can.

We haven't yet decided how to test CoreRT and .Net Native once this is done. But it could just be done manually. sprintf removal (via string.format and structural comparison) will clearly make things easier however it ends up being done. For us the important thing is to test .Net Native thoroughly right now and automated testing is not an immediate goal, although it would be nice to have if it's doable.

@zpodlovics

This comment has been minimized.

zpodlovics commented Jul 16, 2018

@charlesroddie It's still benefically, because it will help other F# implementations too (eg.: Fable, WebSharper, Fez, etc) - it's far easier to replace a few function at the beginng of each test than 1) provide a System.String.Format reimplementation or replace it everywhere (the same problem with printf). Unfortunately my time is farily limited and mostly focused on paid work or my FsHlvm OSS project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment