Add additional debug configuration for a better debugging experience + CI update

[duckdoom5]

I've added the DebugOpt config and set it as default for a better debugging experience.
This version enables some optimizations and uses _ITERATOR_DEBUG_LEVEL=0 to remove many of the slowdowns of traditional debugging while still keeping the debuggability of the app intact.
It uses the release versions of llvm and crt.

Additionally I've enabled native debugging when running *.Gen test projects from vs2022 by default

[tritao]

This is also not passing Linux CI, not sure about macOS because unfortunately we're still on verson 12, and GitHub CI recently removed it, so it needs some maintenance work: #1888

[duckdoom5]

Ah np, I'll check it out and get that fixed too

[duckdoom5]

.. filter { "toolset:msc*", "configurations:DebugOpt" } ..

I think that fixed it. You were right about the filter part. It seems that caused the msvc flags to leak into the linux build.

Edit: Ah now the linux test step fails. Let me get that fixed too :)

[duckdoom5]

@tritao Ah, I see what's going on here.

So the CI is building the DebugOpt config rn, but the tests depend on the Release builds to be created.

Should we update the scripts to run a specific config?
Eg. setup CI to pass the desired configuration and make sure the entire pipelin uses that config.

Or do we just set the default config back to release for now?

[tritao]

@tritao Ah, I see what's going on here.

So the CI is building the DebugOpt config rn, but the tests depend on the Release builds to be created.

Should we update the scripts to run a specific config? Eg. setup CI to pass the desired configuration and make sure the entire pipelin uses that config.

Or do we just set the default config back to release for now?

If you don't mind getting that working on the CI, then setting it up to pass a given configuration does sound like a pretty good solution. If not then setting it back to release is ok too.

[duckdoom5]

Sure, sounds good. I'll get that setup tomorrow then :)

[duckdoom5]

@tritao I still had some time before bed, so I fixed these issues.

Everything should now pass. I did have to disable one unit test that crashes in debug builds and I've currently excluded the Debug configuration for linux builds, since the llvm redist wasn't compiled for that yet.

Please review the new changes and let me know what you think :)

Also side note, I think it might be a good idea to report parse errors as build failures. The asserts that are thrown are currently ignored by CI.

Edit: CI seems to be failing on the artifact upload pass. Not sure what that is about

[tritao]

CI is still red, can you clarify if this is ready?

[duckdoom5]

CI is still red, can you clarify if this is ready?

Yeah it's ready. Debug is broken due to #1819. (And I didn't touch the macos part).

Edit: I did push a different PR #1894 which will fix this debug build.

[duckdoom5]

Huh, curious. Debug is still not building correctly on CI..
I thought I did a clean and rebuild locally to verify it built correctly...
I'll have to investigate this tomorrow. Was hoping this was done now, but I guess there's one more issue to fix

[duckdoom5]

Hm, spend all day looking for the issue. Thought it might be strings, but after converting them to use the build-in string marshaling functions, that turned out not to be the issue.

What makes it more difficult is that it builds completely fine on my pc.. Kinda lost here since I can't reproduce

[tritao]

Hm, spend all day looking for the issue. Thought it might be strings, but after converting them to use the build-in string marshaling functions, that turned out not to be the issue.

What makes it more difficult is that it builds completely fine on my pc.. Kinda lost here since I can't reproduce

What makes you think it may be related to the the C++/CLI string marshaling?

[duckdoom5]

Well it was 3 things really, but like I said I can't confirm since I can't reproduce it locally.

1. In CI, all of the failing C# exe's exited with code -1073740791, which is the heap corruption error code (though I think a call to abort() also triggers this error code)
2. One of them fails with the following exception:

Unhandled exception. System.Runtime.InteropServices.SEHException (0x80004005): External component has thrown an exception.
     at CppSharp.Parser.CppParserOptions.__Internal.AddSupportedFunctionTemplates(IntPtr __instance, String s)
     at CppSharp.Parser.CppParserOptions.AddSupportedFunctionTemplates(String s) in D:\a\CppSharp\CppSharp\src\CppParser\Bindings\CSharp\x86_64-pc-win32-msvc\CppSharp.CppParser.cs:line 40907
     at CppSharp.Tests.CSharpTestsGenerator.Setup(Driver driver) in D:\a\CppSharp\CppSharp\tests\dotnet\CSharp\CSharp.Gen.cs:line 28
     at CppSharp.ConsoleDriver.Run(ILibrary library) in D:\a\CppSharp\CppSharp\src\Generator\Driver.cs:line 397
     at CppSharp.Tests.CSharpTestsGenerator.Main(String[] args) in D:\a\CppSharp\CppSharp\tests\dotnet\CSharp\CSharp.Gen.cs:line 96

And AddSupportedFunctionTemplates deals with strings.

3. Issue TestStdStringInField and TestArrayParams fail in debugging configuration on windows because std:string size is wrong. #1889 was talking about a bug with strings in debug builds on windows

So that's why I went in that direction first

[duckdoom5]

Hmm, I did notice something else. The remaining part of that exception:

Fatal error. 0xC0000005
     at CppSharp.Parser.CppParserOptions+__Internal.dtor(IntPtr)
     at CppSharp.Parser.CppParserOptions.Dispose(Boolean, Boolean)
     at CppSharp.Parser.CppParserOptions.Dispose()
     at CppSharp.Driver.Dispose()
     at CppSharp.ConsoleDriver.Run(CppSharp.ILibrary)
     at CppSharp.Tests.CSharpTestsGenerator.Main(System.String[])

And I was messing with the CppParserOptions today and did manage to trigger a similar error, but disregarded it as 'I probably broke something with my edits'. But maybe I should look there

[duckdoom5]

@tritao I might have found something. The size of this struct does not match with the generated parser bindings.
MSVC tells me it's supposed to be 80 instead of 64.

I already tried to regenerate them, but it didn't change anything. How is the size determined? And am I correct in assuming this size should match the native struct size?

(I'm guessing this is the size of the struct in release mode, but in msvc debug mode it's slightly larger)

[duckdoom5]

Okay, fixed that bit. Let's see if that resolves the build error in CI as well.

Edit: Yes! That fixed it. Unfortunately it seems I broke everything else so I'll fix that when I find some time again

[tritao]

Okay, fixed that bit. Let's see if that resolves the build error in CI as well.

Edit: Yes! That fixed it. Unfortunately it seems I broke everything else so I'll fix that when I find some time again

Yes, I think you found the issue, we need to run the Parser.Gen in normal release mode, and use it to generate debug-mode parser bindings.

[duckdoom5]

So the debug build is successful now! We should have all greens.

I see that the tests take a long time in debug builds though. We might want to disable that step for debug builds to reduce compute costs.

Also, due to the amount of hours I spend on this, I did add a lot of changes to this PR.
I could split it up if you'd prefer that.

[tritao]

Are the changes to the C++/CLI string marshaling necessary?

Maybe you can explain a bit more about them, because to be honest I have never really put any time into understanding how it works, so I am unsure about the changes.

[duckdoom5]

I don't think it's strictly necessary, however I do think the new method is more future proof and a more 'correct' implementation for the following three reasons:

1. This uses the standard library implementation for ANSI and UTF-16 strings. Any bug fixes or improvements would be automatically applied. (UTF-8 strings use a copy of that implementation with a forced UTF-8 conversion, so those would still need to be updated if the standard lib implementation ever changes)
2. We are dealing with C++ strings. The previous implementation used .NET's converters, and while I do believe those use a similar (if not exactly the same) method, this method is what is 'supposed' to be used on the c++ side. The reason this is important is mostly for ANSI strings.
3. (Most important) The /utf-8 flag and UTF-8 code page will allow us to use normal std::string's to encode UTF-8 strings in 'ANSI mode'. (Finally! I'm already using this and loving it in my project :))
   In other words, it is very important that we use the ::WideCharToMultiByte(CP_THREAD_ACP, ...) to correctly convert strings to the C++ side. (Which is what the default implementation of msclr's marshal_as does.)

I hope I explained that well enough to make sense :P

[duckdoom5]

@tritao So are we good to merge this or are you still reviewing? :)

(Don't mean to push you or anything, just checking if you didn't accidentally forget)

[tritao]

@tritao So are we good to merge this or are you still reviewing? :)

(Don't mean to push you or anything, just checking if you didn't accidentally forget)

Yeah looks good, just a lot going on lately :)

By the way, about the LLVM debug build, we have some workflows at https://github.com/mono/CppSharp/blob/main/.github/workflows/llvm.yml and https://github.com/mono/CppSharp/blob/main/.github/workflows/llvm-win.yml, maybe we should extend them to do it too.