refactor: c++ 17

[ShortDevelopment]

LibICU now requires c++ 17 but CC currently uses c++ 11:

ChakraCore/CMakeLists.txt

Line 375 in 36becec

set(CMAKE_CXX_STANDARD 11)

This causes MacOS ci to fail.

📣 Changes

• Upgrade catch to v1.12.2

⚠️ Deprecations

• Dynamic exception specification thow(...)
• bool std::uncaught_exception() is replaced by int uncaught_exceptions() see uncaught_exception

💥 Breaking changes

• ...

---

@ppenzin

Fixes #3147
Fixes #6378

[rhuanjl]

Is this one next up? Do you know what caused the test fails?

[ShortDevelopment]

@rhuanjl I updated the test framework used in NativeTests. I still need some time to fix this.
Could you have a look at #7042 in the meantime?

[ShortDevelopment]

We still get a test failure for x64 NativeTests which likely means GC is not working any more (not good).

ChakraCore/bin/NativeTests/JsRTApiTest.cpp

Line 136 in c6a9549

CHECK(valueRefAfterGC == JS_INVALID_REFERENCE);

So far, I cannot reproduce this issue with a local build.

Edit: The issue is reproducable using the ci build from Azure Artifacts.
The issues seems to be within ChakraCore.dll (using my local build of ChakraCore.dll fixes the issue)

Edit: Only happens in 'Test' (likely in 'Release' aswell) not 'Debug' configuration.

[rhuanjl]

We still get a test failure for x64 NativeTests which likely means GC is not working any more (not good).

ChakraCore/bin/NativeTests/JsRTApiTest.cpp

Line 136 in c6a9549

CHECK(valueRefAfterGC == JS_INVALID_REFERENCE);

So far, I cannot reproduce this issue with a local build.

Edit: The issue is reproducable using the ci build from Azure Artifacts. The issues seems to be within ChakraCore.dll (using my local build of ChakraCore.dll fixes the issue)

Edit: Only happens in 'Test' (likely in 'Release' aswell) not 'Debug' configuration.

Can you reproduce with a local Test build? Or only by downloading a test build? If this dependent on compiler versions it may be awkward to solve, hopefully it's a simple configuration issue somehow...

[ShortDevelopment]

Can you reproduce with a local Test build?

That's the problem: I can't 🙈
(I can with the x64 Test binaries from Azure Artifacts though)

If this dependent on compiler versions it may be awkward to solve

I fear that might be the case...

hopefully it's a simple configuration issue somehow...

Here's what I did:

• Download zip of this branch from github
• Unzip
• Open VS Command-Prompt (Vs 2022 v17.14.16)
• .\test\ci.buildone.cmd x64 test
• .\test\runnativetests.cmd -d true -x64 -test
• All Tests pass

[ShortDevelopment]

I created a small test c program based on the failing native test to test the behavior on linux

• this PR: fail
• last available ci build (22f1619): fail
• last ms release (v1.11.24): pass

POC

Removing the puts line lets the test fail:

#include <string.h>
#include <stdio.h>
#include "ChakraCommon.h"
#include "ChakraCore.h"

#define REQUIRE(condition) if (!(condition)) return -1;
#define CHECK(condition) if (!(condition)) return -1;

int main(){
JsRuntimeAttributes attributes = 0;

JsRuntimeHandle runtime = JS_INVALID_RUNTIME_HANDLE;

JsValueRef context = JS_INVALID_REFERENCE;
JsValueRef setContext = JS_INVALID_REFERENCE;

// Create runtime, context and set current context
REQUIRE(JsCreateRuntime(attributes, NULL, &runtime) == JsNoError);
REQUIRE(JsCreateContext(runtime, &context) == JsNoError);
REQUIRE(JsSetCurrentContext(context) == JsNoError);
REQUIRE(((JsGetCurrentContext(&setContext) == JsNoError) || setContext == context));

JsValueRef valueRef = JS_INVALID_REFERENCE;
REQUIRE(JsCreateString("test", strlen("test"), &valueRef) == JsNoError);

JsWeakRef weakRef = JS_INVALID_REFERENCE;
REQUIRE(JsCreateWeakReference(valueRef, &weakRef) == JsNoError);

// JsGetWeakReferenceValue should return the original value reference.
JsValueRef valueRefFromWeakRef = JS_INVALID_REFERENCE;
CHECK(JsGetWeakReferenceValue(weakRef, &valueRefFromWeakRef) == JsNoError);
CHECK(valueRefFromWeakRef != JS_INVALID_REFERENCE);
CHECK(valueRefFromWeakRef == valueRef);

// Clear the references on the stack, so that the value will be GC'd.
valueRef = JS_INVALID_REFERENCE;
valueRefFromWeakRef = JS_INVALID_REFERENCE;

puts("This line is the fix?!");
CHECK(JsCollectGarbage(runtime) == JsNoError);

// JsGetWeakReferenceValue should return an invalid reference after the value was GC'd.
JsValueRef valueRefAfterGC = JS_INVALID_REFERENCE;
CHECK(JsGetWeakReferenceValue(weakRef, &valueRefAfterGC) == JsNoError);
printf("valueRefAfterGC = %ld\n", (int64_t)valueRefAfterGC);
CHECK(valueRefAfterGC == JS_INVALID_REFERENCE);

printf("Pass\n");
}

[ppenzin]

Is this a MSVC version issue - what is your local version vs Azure? The CI says "MSBuild version 17.14.23+b0019275e for .NET Framework"

[ShortDevelopment]

I can check tomorrow.

The weird thing is: I get the same symptoms when I test the latest Linux build and when I build the v11 branch on Ubuntu 18.04!
But the latest official Microsoft Release on Linux does not show this behavior.

[rhuanjl]

I created a small test c program based on the failing native test to test the behavior on linux

• this PR: fail
• last available ci build (22f1619): fail
• last ms release (v1.11.24): pass

POC

Something that's dependent on compiler Or OS versions is very awkward; also would be good to work out whether it's GC not triggering for some reason or GC not collecting the objects it ought to collect.

The "fix" in this is interesting... Could the Compiler be re-ordering operations and the output prevents it doing it?

[rhuanjl]

Interesting that the tests now pass do you think the catch framework update was the problem?

[ShortDevelopment]

@rhuanjl According to this little sanity-check it actually seems like the catch-update "caused" the ci failures (good intuition on your side!)

In my head there might be two root causes:

1. The new "catch" version captures the js-object references on the stack ⇾ GC might work fine
   I actually checked the disassembly but might have missed sth
2. There might be a bug in GC making it flaky (Similar to the behavior I observed on Linux)

It's still strange that I could not reproduce the issue locally.
I'll check the disassembly again later today...

[ShortDevelopment]

Okay, I did some debugging of the NativeTests.exe and ChakraCore.dll binaries from the failing ci runs.

ChakraCore/bin/NativeTests/JsRTApiTest.cpp

Lines 113 to 137 in 2dba810

	void WeakReferenceTest(JsRuntimeAttributes attributes, JsRuntimeHandle runtime)
	{
	JsValueRef valueRef = JS_INVALID_REFERENCE;
	REQUIRE(JsCreateString("test", strlen("test"), &valueRef) == JsNoError);
	JsWeakRef weakRef = JS_INVALID_REFERENCE;
	REQUIRE(JsCreateWeakReference(valueRef, &weakRef) == JsNoError);
	// JsGetWeakReferenceValue should return the original value reference.
	JsValueRef valueRefFromWeakRef = JS_INVALID_REFERENCE;
	CHECK(JsGetWeakReferenceValue(weakRef, &valueRefFromWeakRef) == JsNoError);
	CHECK(valueRefFromWeakRef != JS_INVALID_REFERENCE);
	CHECK(valueRefFromWeakRef == valueRef);
	// Clear the references on the stack, so that the value will be GC'd.
	valueRef = JS_INVALID_REFERENCE;
	valueRefFromWeakRef = JS_INVALID_REFERENCE;
	CHECK(JsCollectGarbage(runtime) == JsNoError);
	// JsGetWeakReferenceValue should return an invalid reference after the value was GC'd.
	JsValueRef valueRefAfterGC = JS_INVALID_REFERENCE;
	CHECK(JsGetWeakReferenceValue(weakRef, &valueRefAfterGC) == JsNoError);
	CHECK(valueRefAfterGC == JS_INVALID_REFERENCE);
	}

Turns out: The CC-GC (Conservative Mark'n'Sweep) actually finds a reference to the created js-string (see Test / POC above) on the stack. But the reference seems to originate from a stack-frame that's inside the GC handling stuff (JsCollectGarbage); not the test case.

Here's my theory:

AFAIK, stack memory does not get zeroed before or after a function call.
One of the previous calls to JsCreateString, JsCreateWeakReference or JsGetWeakReferenceValue will have a reference to our string on its stack-frame. The following calls will reuse the same memory area, not zero the whole frame and not write to the same address; Maybe the following functions will use that area as padding to align other locals.

So we will end up with a ghost-reference to out object that will be found by our conservative GC when scanning the whole stack.
To fix this the Boehm GC actually has a method that's used to clear the "unused" stack space:
https://github.com/bdwgc/bdwgc/blob/e1042aa86d9403f433a2ab38ee2aab081984fca8/misc.c#L260-L285

I added a new JSRT api (JsGarbageCollectionClearStack) to do the same thing in CC.
It does not use memset etc. because that was optimized away.
Of cause, we need to discuss how we actually want to handle this.

This might also explain the behavior during my linux tests:

The call to puts might have written to the same address a previous function used for the js-object.
Using the JsGarbageCollectionClearStack let's the linux POC pass.

I'm still not sure though, why I could not reproduce the windows behavior locally.
Maybe my compiler version did arrange the stack slightly different?

Also the last Microsoft build of the linux version does not show the test failures.
Maybe a different stack layout again?
Or they used some (security-)flags to instruct the compiler to zero the stack?

[ShortDevelopment]

I added a new simple testcase to NativeTests but that might be very flaky based on (future) compiler optimizations.
Not sure how to handle this...