fextl: Implements a custom unique_ptr

[Sonicadvance1]

Gets rid of the absolutely obnoxious warning messages due to using offsetof on non-standard-layout type.

/home/buildbot/actions-runner/_work/FEX/FEX/FEXCore/Source/Interface/Core/JIT/Arm64/JIT.cpp:732:77: warning: offset of on non-standard-layout type 'FEXCore::Core::InternalThreadState' [-Winvalid-offsetof]
         offsetof(FEXCore::Core::InternalThreadState, InterruptFaultPage) - offsetof(FEXCore::Core::InternalThreadState, BaseFrameState));
                                                                            ^                                            ~~~~~~~
/usr/lib/llvm-14/lib/clang/14.0.0/include/stddef.h:104:24: note: expanded from macro 'offsetof'

Number of times this message gets spammed in a clean build:

$ ninja | grep "invalid-offsetof" | wc -l
187

Also needed to patch two locations that we forgot to change

• AOTIR: Was accidentally converting fextl::unique_ptr to std::unique_ptr
  • benign but good to keep in in fextl
  • Only worked because fextl::unique_ptr was technically the same type under the hood
• RegisterAllocationPass
  • Taking advantage of a quirk that libstdc++ unique_ptr decomposes default deleters to the size of a pointer
  • new fextl::unique_ptr implementation can't do that, not even sure quite how to do so.

Just a bit of busy work while I'm sick.

[neobrain]

How does it work?

[Sonicadvance1]

How does it work?

Nothing special. It's just a tuple holding the owned pointer and deleter. Matching behaviour with std::unique_ptr as much as possible.

The difference being that we can assure that it is standard layout where std::unique_ptr isn't guaranteed.

I don't know how to implement the optimization that occurs with std::unique_ptr where if the deleter is templated fully then it decomposes down to a single 8-byte pointer instead of 16-bytes.

[neobrain]

$ ninja | grep "invalid-offsetof" | wc -l
187

The vast majority of these stem from a single line in InternalThreadState.h. Why don't we handle this on a case-by-case basis? In that particular header, we can just disable the warning for the affected line, for example.

Nothing special. It's just a tuple holding the owned pointer and deleter. Matching behaviour with std::unique_ptr as much as possible.

The difference being that we can assure that it is standard layout where std::unique_ptr isn't guaranteed.

I don't know how to implement the optimization that occurs with std::unique_ptr where if the deleter is templated fully then it decomposes down to a single 8-byte pointer instead of 16-bytes.

fextl shouldn't deviate from normal std:: behavior since it's primary purpose is to write std::-like code using FEX's internal allocator. Interleaving unrelated interface/implementation changes can lead surprising behavior which in turn makes us more prone to bugs.

Here in particular, we're replacing a trivially correct type alias with a 130-lines-heavy bespoke reimplementation of unique_ptr. While the intent is to preserve the interface, it would need more than a few minutes of review to verify it successfully does so, and we don't have any unit test coverage to complement a more exhaustive review.

[pmatos]

$ ninja | grep "invalid-offsetof" | wc -l
187

The vast majority of these stem from a single line in InternalThreadState.h. Why don't we handle this on a case-by-case basis? In that particular header, we can just disable the warning for the affected line, for example.

While I understand the wish to just disable the warning and forget about it, I would prefer if we spend some time trying to fix this since, we are using offsetof on a non standard layout type which is unsupported and the compiler can go "Here be dragons!" on you whenever it wants. True - it's working now but update the compiler and you don't know what it'll do behind your back in the next version - specially if you disable the warning.

[neobrain]

While I understand the wish to just disable the warning and forget about it, I would prefer if we spend some time trying to fix this since, we are using offsetof on a non standard layout type which is unsupported and the compiler can go "Here be dragons!" on you whenever it wants. True - it's working now but update the compiler and you don't know what it'll do behind your back in the next version - specially if you disable the warning.

Fully agreed. Meanwhile, looking at this specific case, the scenario of a compiler change directly/indirectly breaking the tested assumption seems less likely to me than the risks introduced by replacing unique_ptr with an incompatible and untested custom reimplementation.

As far as I can see the "pure" way to fix the warnings would involve either a larger refactor of the code or a trade-off against other safety guarantees (e.g. by replacing all of these unique_ptrs with raw pointers). Pragmatically, disabling the warning seems like the least evil here.

I agree your general point of fixing warnings properly is the correct default approach, though.