[libunwind] Provide inline placement new definition

[phosek]

Summary

While Clang automatically generates the code for global placement new,
g++ doesn't do that so we need to provide our own definition.

[phosek]

This was reported by @mgorny since r352553 broke NetBSD bots. I've confirmed this locally. Alternatively we could wrap the declaration in the #ifdef __clang__ black and provide definition in the else case if that's preferred.

[mgorny]

If it doesn't break clang, I don't think #ifdef is really necessary. I'm going to test it in an hour or so.

[mgorny]

Actually, it went faster than expected. I can both reproduce the failure locally (on NetBSD) and confirm that the patch fixes it.

It would probably be nice to pass this through some language lawyer but I suppose merging it shouldn't make things any worse than they already are ;-).

[mclow.lists]

Um... I suspect that this is technically UB. Having more than two (one in the standard library, and one other) in an executable context is a bad idea.
Marking it as inline might make it work, but I still think it's UB.

[mclow.lists]

Turns out the placement versions of new are not replaceable, so what I said about UB is just wrong.

However, In [new.delete.placement]/3, it says: These functions are reserved, a C++ program may not define functions that displace the versions in the Standard C++ library (17.6.4).

[phosek]

In D57455#1377727, @mclow.lists wrote:

Turns out the placement versions of new are not replaceable, so what I said about UB is just wrong.

However, In [new.delete.placement]/3, it says: These functions are reserved, a C++ program may not define functions that displace the versions in the Standard C++ library (17.6.4).

That shouldn't be a problem for shared library, for static library it might be it compiler decides not to inline it. Would it be better to use __attribute__((__always_inline__)) to enforce that?

[mgorny]

A little ping, since this is keeping buildbot red for some time already.

I would suggest going for the always-inline version for now, and possibly devising another fix in the future.

Also might be worthwhile to ask on stackoverflow. @phosek, would you do that? If not, I can but I think you can describe the problem better.

[krytarowski]

The NetBSD buildbot is affected and red for some time now.

[ldionne]

In D57455#1377727, @mclow.lists wrote:

Turns out the placement versions of new are not replaceable, so what I said about UB is just wrong.

However, In [new.delete.placement]/3, it says: These functions are reserved, a C++ program may not define functions that displace the versions in the Standard C++ library (17.6.4).

In other words, the only Standard-blessed way of doing this is to #include <new> and use the definition inside the Standard library.

A way of achieving the same effect would be to define the same thing thats in <new>, like:

_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;}

I'll admit I dislike all the solutions here. Also it means there's no such thing as C++ without its standard library, which I didn't think of before.

If Marshall is OK with that, I'd suggest going with the proposed patch but adding a note that this isn't technically Standards-conforming. Otherwise, the only solution I see is to re-introduce a dependency on the libc++ headers (which might not be so bad after all because we really need a separate concept of libc++ headers and libc++ runtime library anyway).

[mclow.lists]

My problem with declaring our own placement new is that I think we're just kicking the can down the road; that the problem might (will) come back to bite us in the future.
It would certainly be conforming for clang to reject this code at compile time.

But it's ok as a short-term fix. Who is going to figure out what we want to do long term?

[ldionne]

In D57455#1381424, @mclow.lists wrote:

My problem with declaring our own placement new is that I think we're just kicking the can down the road; that the problem might (will) come back to bite us in the future.
It would certainly be conforming for clang to reject this code at compile time.

But it's ok as a short-term fix. Who is going to figure out what we want to do long term?

I see a couple of options:

1. Don't use anything that requires the Standard library inside libunwind (basically use C)
2. The classical solution to resolve circular dependencies: split the Standard library into a part that does not require libunwind and a part that does. The part that does not require libunwind can be used inside libunwind, and should include things like this placement new operator.
3. Just give up on eliminating the circular dependency, use #include <new> and be done with it. Then we need to add _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS when building a hermetic static library and the reply to my comment on D57107 just becomes "though luck".

[rsmith]

libunwind/src/Unwind-seh.cpp

54 ↗

(On Diff #184304)

To avoid conflicting with the standard library, how about changing this to something like:

namespace { struct placement_new { void *where; }; }
void *operator new(size_t, placement_new where) { return where.where; }

(and calling it as new (placement_new{addr}) T)?

[mclow.lists]

@rsmith just reminded me (in IRC) that it's perfectly legal to declare operator new in your classes.

So why not add this to UnwindCursor? (warning - untested code):

static UnwindCursor *operator new(size_t, UnwindCursor* p) { return p; }

[ldionne]

In D57455#1381513, @mclow.lists wrote:

@rsmith just reminded me (in IRC) that it's perfectly legal to declare operator new in your classes.

So why not add this to UnwindCursor? (warning - untested code):

static UnwindCursor *operator new(size_t, UnwindCursor* p) { return p; }

Yes, that seems by far like the best suggestion.

[phosek]

I tried the suggestion and while static UnwindCursor<A, R> *operator new(size_t, UnwindCursor<A, R> *p) { return p; } didn't work because Clang complains:

note: candidate function not viable: cannot convert argument of incomplete type 'void *' to 'UnwindCursor<libunwind::LocalAddressSpace, libunwind::Registers_x86_64> *' for 2nd argument

static void *operator new(size_t, void *p) { return p; } inside UnwindCursor does seem to work for both Clang and GCC.

[phosek]

Never mind, it seems to be working fine with appropriate casts.

[mgorny]

Well, it's certainly nicer than being on the edge of violating the spec, and once again I can confirm it builds fine for me.

[phosek]

Rebased

libunwind/trunk/src/Unwind-seh.cpp

 #define STATUS_GCC_THROW MAKE_GCC_EXCEPTION(0) // 0x20474343
 /// SEH exception raised by libunwind to initiate phase 2 of exception
 /// handling.
 #define STATUS_GCC_UNWIND MAKE_GCC_EXCEPTION(1) // 0x21474343
 
 /// Class of foreign exceptions based on unrecognized SEH exceptions.
 static const uint64_t kSEHExceptionClass = 0x434C4E4753454800; // CLNGSEH\0
 
-// libunwind does not and should not depend on C++ library which means that we
-// need our own declaration of global placement new.
-void *operator new(size_t, void*);
-
 /// Exception cleanup routine used by \c _GCC_specific_handler to
 /// free foreign exceptions.
 static void seh_exc_cleanup(_Unwind_Reason_Code urc, _Unwind_Exception *exc) {
   (void)urc;
   if (exc->exception_class != kSEHExceptionClass)
     _LIBUNWIND_ABORT("SEH cleanup called on non-SEH exception");
   free(exc);
 }
_Unwind_GetRegionStart(struct _Unwind_Context *context) {
   _LIBUNWIND_TRACE_API("_Unwind_GetRegionStart(context=%p) => 0x%" PRIxPTR,
                        (void *)context, result);
   return result;
 }
 
 static int
 _unw_init_seh(unw_cursor_t *cursor, CONTEXT *context) {
 #ifdef _LIBUNWIND_TARGET_X86_64
-  new ((void *)cursor) UnwindCursor<LocalAddressSpace, Registers_x86_64>(
+  new (reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_x86_64> *>(cursor))
+      UnwindCursor<LocalAddressSpace, Registers_x86_64>(
           context, LocalAddressSpace::sThisAddressSpace);
   auto *co = reinterpret_cast<AbstractUnwindCursor *>(cursor);
   co->setInfoBasedOnIPRegister();
   return UNW_ESUCCESS;
 #elif defined(_LIBUNWIND_TARGET_ARM)
-  new ((void *)cursor) UnwindCursor<LocalAddressSpace, Registers_arm>(
+  new (reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_arm> *>(cursor))
+      UnwindCursor<LocalAddressSpace, Registers_arm>(
           context, LocalAddressSpace::sThisAddressSpace);
   auto *co = reinterpret_cast<AbstractUnwindCursor *>(cursor);
   co->setInfoBasedOnIPRegister();
   return UNW_ESUCCESS;
 #elif defined(_LIBUNWIND_TARGET_AARCH64)
-  new ((void *)cursor) UnwindCursor<LocalAddressSpace, Registers_arm64>(
+  new (reinterpret_cast<UnwindCursor<LocalAddressSpace, Registers_arm64> *>(cursor))
+      UnwindCursor<LocalAddressSpace, Registers_arm64>(
           context, LocalAddressSpace::sThisAddressSpace);
   auto *co = reinterpret_cast<AbstractUnwindCursor *>(cursor);
   co->setInfoBasedOnIPRegister();
   return UNW_ESUCCESS;
 #else
   return UNW_EINVAL;
 #endif
 }
 

libunwind/trunk/src/UnwindCursor.hpp

public:
   virtual bool        isSignalFrame();
   virtual bool        getFunctionName(char *buf, size_t len, unw_word_t *off);
   virtual void        setInfoBasedOnIPRegister(bool isReturnAddress = false);
   virtual const char *getRegisterName(int num);
 #ifdef __arm__
   virtual void        saveVFPAsX();
 #endif
 
+  // libunwind does not and should not depend on C++ library which means that we
+  // need our own defition of inline placement new.
+  static void *operator new(size_t, UnwindCursor<A, R> *p) { return p; }
+
 private:
 
 #if defined(_LIBUNWIND_ARM_EHABI)
   bool getInfoFromEHABISection(pint_t pc, const UnwindInfoSections &sects);
 
   int stepWithEHABI() {
     size_t len = 0;
     size_t off = 0;

libunwind/trunk/src/libunwind.cpp

 
 
 #if !defined(__USING_SJLJ_EXCEPTIONS__)
 #include "AddressSpace.hpp"
 #include "UnwindCursor.hpp"
 
 using namespace libunwind;
 
-// libunwind does not and should not depend on C++ library which means that we
-// need our own declaration of global placement new.
-void *operator new(size_t, void*);
-
 /// internal object to represent this processes address space
 LocalAddressSpace LocalAddressSpace::sThisAddressSpace;
 
 _LIBUNWIND_EXPORT unw_addr_space_t unw_local_addr_space =
     (unw_addr_space_t)&LocalAddressSpace::sThisAddressSpace;
 
 /// record the registers and stack position of the caller
 extern int unw_getcontext(unw_context_t *);

 #elif defined(__mips__)
 # warning The MIPS architecture is not supported with this ABI and environment!
 #elif defined(__sparc__)
 # define REGISTER_KIND Registers_sparc
 #else
 # error Architecture not supported
 #endif
   // Use "placement new" to allocate UnwindCursor in the cursor buffer.
-  new ((void *)cursor) UnwindCursor<LocalAddressSpace, REGISTER_KIND>(
-                                 context, LocalAddressSpace::sThisAddressSpace);
+  new (reinterpret_cast<UnwindCursor<LocalAddressSpace, REGISTER_KIND> *>(cursor))
+      UnwindCursor<LocalAddressSpace, REGISTER_KIND>(
 #undef REGISTER_KIND
   AbstractUnwindCursor *co = (AbstractUnwindCursor *)cursor;
   co->setInfoBasedOnIPRegister();
 
   return UNW_ESUCCESS;
 }
 
 /// Get value of specified register at cursor position in stack frame.