Fix MacOS build

[ShortDevelopment]

TL;DR

MacOs builds using latest icu4c fail because the "placement new operator" is defined in the standard-library and in Allocator.h.

Issue

As far as I understand the issue (might be wrong)

• CC source defines a new operator that used an already existing allocation (previousAllocation)
  

  ChakraCore/lib/Common/Memory/Allocator.h

  Lines 451 to 474 in e26c81f

  	// For the debugger extension, we don't need the placement news
  	#ifndef __PLACEMENT_NEW_INLINE
  	#define __PLACEMENT_NEW_INLINE
  	_Ret_notnull_
  	NO_EXPORT(inline void *) __cdecl
  	operator new(
  	DECLSPEC_GUARD_OVERFLOW size_t byteSize,
  	_In_ void * previousAllocation) throw()
  	{
  	return previousAllocation;
  	}
  	NO_EXPORT(inline void) __cdecl
  	operator delete(
  	void * allocationToFree, // Allocation to free
  	void * previousAllocation // Previously allocated memory
  	) throw()
  	{
  	}
  	#endif

• This code was added in the Initial commit
• This might exist to speed-up builds because the compiler does not have to parse the full new header for every compilation unit that uses the custom CC Allocator.h.
• It might also have forced compilers to inline this no-op call (Seems like they do now)
• See Add _LIBCPP_PLACEMENT_NEW_DEFINED macro to allow for user defined placement new operators llvm/llvm-project#70538

Std lib

MSVC

The __PLACEMENT_NEW_INLINE flag can be used to disable placement new declaration in MSVC (v14.43.34808)
See vcruntime_new.h (imported by new):

#ifndef __PLACEMENT_NEW_INLINE
    #define __PLACEMENT_NEW_INLINE
    _VCRT_EXPORT_STD _NODISCARD _MSVC_CONSTEXPR _Ret_notnull_ _Post_writable_byte_size_(_Size) _Post_satisfies_(return == _Where)
    inline void* __CRTDECL operator new(size_t _Size,
        _Writable_bytes_(_Size) void* _Where) noexcept
    {
        (void)_Size;
        return _Where;
    }

    _VCRT_EXPORT_STD inline void __CRTDECL operator delete(void*, void*) noexcept
    {
        return;
    }
#endif

MacOs

This is from the MacOSX14.5 sdk (There seems no way to disable it llvm/llvm-project/pull/70538)

_LIBCPP_NODISCARD_AFTER_CXX17 inline _LIBCPP_INLINE_VISIBILITY void* operator new  (std::size_t, void* __p) _NOEXCEPT {return __p;}
_LIBCPP_NODISCARD_AFTER_CXX17 inline _LIBCPP_INLINE_VISIBILITY void* operator new[](std::size_t, void* __p) _NOEXCEPT {return __p;}
inline _LIBCPP_INLINE_VISIBILITY void  operator delete  (void*, void*) _NOEXCEPT {}
inline _LIBCPP_INLINE_VISIBILITY void  operator delete[](void*, void*) _NOEXCEPT {}

Why did it work before?

• The definition of __PLACEMENT_NEW_INLINE solved the problem for MSVC
• The problem did not happen on other platforms / using other compilers because there was likely no compilation unit that used libcxx new and CC Allocator.h at the same time.
• Different compilation units don't collide because their definitions are made "private" using the CC custom NO_EXPORT macro
  

  ChakraCore/lib/Common/Memory/Allocator.h

  Lines 443 to 449 in e26c81f

  	#ifndef _WIN32
  	#define NO_EXPORT(x) \
  	__attribute__((visibility("hidden"))) \
  	x
  	#else
  	#define NO_EXPORT(x) x
  	#endif

What's different now?

• libicu on MacOS does now include new into a compilation unit that also uses Allocator.h
• This leads to 2 different declarations of the same operator inside the same compilation unit
  ⇒ Error

StackTrace

In file included from /Users/test/Documents/Projects/ChakraCore/lib/Backend/Encoder.cpp:5:
In file included from /Users/test/Documents/Projects/ChakraCore/lib/Backend/Backend.h:12:
In file included from /Users/test/Documents/Projects/ChakraCore/lib/Backend/../Runtime/Runtime.h:334:
In file included from /Users/test/Documents/Projects/ChakraCore/lib/Backend/../Runtime/PlatformAgnostic/ChakraPlatform.h:7:
In file included from /Users/test/Documents/Projects/ChakraCore/lib/Backend/../Runtime/PlatformAgnostic/UnicodeText.h:8:
In file included from /Users/test/Documents/Projects/ChakraCore/lib/Backend/../Runtime/PlatformAgnostic/ChakraICU.h:38:
In file included from /usr/local/opt/icu4c/include/unicode/ucol.h:1524:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/functional:521:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/__functional/bind.h:20:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/tuple:1868:
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/new:277:70: error: redefinition of 'operator new'
_LIBCPP_NODISCARD_AFTER_CXX17 inline _LIBCPP_INLINE_VISIBILITY void* operator new  (std::size_t, void* __p) _NOEXCEPT {return __p;}
                                                                     ^
/Users/test/Documents/Projects/ChakraCore/lib/Common/Memory/Allocator.h:457:1: note: previous definition is here
operator new(
^

Potential Solution

As this specific new overload is part of the standard-library it might be okay to just remove the custom declaration in CC and always #include <new> instead.

Risks

As the implementation is identical there should be no immediate risks, but...

• We might face slower build-times due to more required parsing

I don't think these 2 are true but we need prove for that

• The compiler might not inline this no-op function call
  ⇒ Additional function call might reduce runtime performance
• Including new could have side-effects

---

Fixes #7007

[ppenzin]

It looks like new wrapper is intentional, maybe we need to override including the system one?

[ShortDevelopment]

@ppenzin This code snippet exists since the "First Commit" and caused some build issues before (See 11a6851 and 4398657).

I don't understand why it exists in the first place because according to this gcc docs:

void* operator new(std::size_t, void*) noexcept;
    Non-allocating, “placement” single-object new, which does nothing except return its argument.
    This function cannot be replaced.

[ppenzin]

These declarations do look like a kludge, for sure. @ShortDevelopment, do you have an idea why wasm spec test failed with this change?

I wonder if there is more to this story, @rhuanjl, do you have any recollection on whether or not it is really needed for debugging? There are better methods to get placement new and delete operators if that is what this was supposed to achieve..

[rhuanjl]

These declarations do look like a kludge, for sure. @ShortDevelopment, do you have an idea why wasm spec test failed with this change?

I wonder if there is more to this story, @rhuanjl, do you have any recollection on whether or not it is really needed for debugging? There are better methods to get placement new and delete operators if that is what this was supposed to achieve..

It definitely looks like a fudge - I'm afraid I don't know its intent.

I may be misunderstanding- but I think the comment is actually saying this is Not Needed for the debugger extension (a different compile target?) but, implicitly, is needed for a normal CC build.

The test fail at first glance seems to be a real error of some kind, though would need to dig to understand it.

[rhuanjl]

@ppenzin This code snippet exists since the "First Commit" and caused some build issues before (See 11a6851 and 4398657).

I don't understand why it exists in the first place because according to this gcc docs:

void* operator new(std::size_t, void*) noexcept;

    Non-allocating, “placement” single-object new, which does nothing except return its argument.

    This function cannot be replaced.

Chakracore has never compiled with GCC, it's always needed clang on Linux/macos - I don't know if that's relevant to this issue but could be. (Thought prompted by your ref to the GCC docs)

[ShortDevelopment]

If I correctly understand this PR on clang a custom declaration of the inline placement new operator could lead to (slightly) lower build / debug times because the stl <new> header isn't parsed.

[ppenzin]

These macros aid compiler in various ways, in libcxx PR they are defining one that tells compiler placement new has been defined, the one Allocator.h has seems to imply inlining of the operator (though I don't know which compiler that is for). I think deleting custom placement operator and using a standard one is OK as long as the rest of the CI is passing.

[ShortDevelopment]

All checks are passing now although I did only add a comment and move the #include down.
I also could not reproduce the crash with the build artifacts during a quick test in a vm.

Might this be an existing bug in the codebase?

[ppenzin]

Placement of include can have some effect, though this might be just a spurious failure.

[ppenzin]

I think this should be fine. @rhuanjl what do you think?

[ppenzin]

Poking a bit more, __PLACEMENT_NEW_INLINE macro seems to prevent global placement new declaration, which seems to be exactly the original problem, curious why it didn't work.

[rhuanjl]

Sorry we should have resolved this by now; I'd really like to know why this definition existed in the first place before going along with deleting it; are we sure its identical to the version in the std lib?

[ShortDevelopment]

@rhuanjl I just updated the PR description to include a lot more detail.
@ppenzin I also added a section about __PLACEMENT_NEW_INLINE.

But I still don't have a definitive answer to why this code exists (only guesses).
Can we maybe ask former developers?

[rhuanjl]

@rhuanjl I just updated the PR description to include a lot more detail. @ppenzin I also added a section about __PLACEMENT_NEW_INLINE.

But I still don't have a definitive answer to why this code exists (only guesses). Can we maybe ask former developers?

Thanks for the extra explanation and thank you for looking into this I'm happy that this needs to go on mac; but without checking further I'd like to keep it on windows.

Please could you update to leave the old code #ifdef _WIN32 and #else include the header then I'll merge it.

[ShortDevelopment]

@rhuanjl Good idea! I checked for MSVC instead of Win32 cause this behavior seems to depend on the compiler.
But now I got a failing allocation test: Could be just out-of-memory?
(In theory nothing should have changed for MSVC, but I might be wrong...)

ChakraCore/test/typedarray/allocation.js

Lines 6 to 12 in e26c81f

	for (i = 0; i < 1000; i++) {
	var a = new ArrayBuffer(10000000);
	for (j = 0; j < 20; j++) {
	var b = new Object();
	}
	}
	WScript.Echo("pass");

[rhuanjl]

@rhuanjl Good idea! I checked for MSVC instead of Win32 cause this behavior seems to depend on the compiler. But now I got a failing allocation test: Could be just out-of-memory? (In theory nothing should have changed for MSVC, but I might be wrong...)

ChakraCore/test/typedarray/allocation.js

Lines 6 to 12 in e26c81f

	for (i = 0; i < 1000; i++) {
	var a = new ArrayBuffer(10000000);
	for (j = 0; j < 20; j++) {
	var b = new Object();
	}
	}
	WScript.Echo("pass");

I re-ran the pipeline and the test passed, that specific test has been flaky before as it uses a very high level of ram - near the limit on the test VM I think. Hence merged. Thanks for this contribution.

(The checks showed as a fail because publishing artefacts on a re-run results in a naming clash and hence always fails.)

[ShortDevelopment]

Nice, thanks @rhuanjl

I'll try to add a fix for the artifacts and finally finish the optional chaining (has been partially blocked by this) once I got more time in a view weeks.

@ppenzin
What do you think of having a scheduled pipeline run on main every week or so to catch such issues early?

Also just out of curiosity: Is there a particular reason why we don't use GitHub Actions instead of 3rd party CI providers?

[rhuanjl]

Sorry we let this sit so long; my life is busier these days than it used to be; I'm going to try and be a bit more active again from now but we'll see - I'd certainly be happy to review the optional chaining PR when you progress it.

RE:pipeline providers:

1. we used Azure pipelines because when we took over the repo from MS they were using azure and it was easier to transition to our own free azure account than do something totally different.
2. we use Cirrus CI for Mac because when I got CC to run on apple silicon, Cirrus CI was the only provider to do free Apple Silicon pipelines.

I note that both have usage limits and using two providers actually means the tests complete faster than if they were on the same provider.

[ShortDevelopment]

@rhuanjl

Sorry we let this sit so long; my life is busier these days than it used to be; I'm going to try and be a bit more active again from now but we'll see

No problem, by "partially blocked" I honestly mean the failing pipeline gave me a good excuse to work on something else 🙈.
This is more of a hobby to me anyway.

Thank you for all your work!

I note that both have usage limits and using two providers actually means the tests complete faster than if they were on the same provider.

Good point!