Page MenuHomePhabricator

[libunwind] Provide inline placement new definition
ClosedPublic

Authored by phosek on Jan 30 2019, 8:43 AM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Jan 30 2019, 8:43 AM

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.

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 accepted this revision.Jan 30 2019, 11:17 AM

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 ;-).

This revision is now accepted and ready to land.Jan 30 2019, 11:17 AM

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.

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).

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?

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.

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

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).

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?

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 added a subscriber: rsmith.Feb 1 2019, 1:49 PM
rsmith added inline comments.
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)?

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

@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 updated this revision to Diff 184895.Feb 1 2019, 10:09 PM

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 updated this revision to Diff 184899.Feb 1 2019, 10:22 PM

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

mgorny accepted this revision.Feb 1 2019, 11:39 PM

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 updated this revision to Diff 184914.Feb 2 2019, 1:09 PM

Rebased

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2019, 1:15 PM