This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Drop the dependency on <algorithm>, add placement new inline
ClosedPublic

Authored by phosek on Jan 25 2019, 3:31 PM.

Details

Summary

We haven't eliminated C++ library dependency altogether in D57251,
UnwindCursor.hpp had an unused dependency on <algorithm> which was
pulling in other C++ headers. Removing that dependency also revealed
(correctly) that we need our own global placement new declaration. Now
libunwind should be independent of the C++ library.

Diff Detail

Repository
rUNW libunwind

Event Timeline

phosek created this revision.Jan 25 2019, 3:31 PM
phosek updated this revision to Diff 183639.Jan 25 2019, 3:35 PM

Comment added.

mcgrathr accepted this revision.Jan 25 2019, 3:38 PM
mcgrathr added a subscriber: mcgrathr.

Should you add -nostdinc++ to the compilation flags to drive home that this code can't depend on any library headers?

This revision is now accepted and ready to land.Jan 25 2019, 3:38 PM
ldionne added inline comments.Jan 25 2019, 3:38 PM
libunwind/src/libunwind.cpp
30 ↗(On Diff #183637)

Why do we need to define this? I thought placement new was handled by Clang directly, without requiring a declaration?

phosek marked an inline comment as done.Jan 25 2019, 4:08 PM
phosek added inline comments.
libunwind/src/libunwind.cpp
30 ↗(On Diff #183637)

That's what I assumed but that doesn't seem to be the case, without this definition the compilation fails because Clang cannot find the operator new.

phosek updated this revision to Diff 183649.Jan 25 2019, 4:10 PM

Should you add -nostdinc++ to the compilation flags to drive home that this code can't depend on any library headers?

Done, we were already using -nostdinc++ but we were also adding libc++ include path explicitly into the list of include directories, so I dropped that logic and made -nostdinc++ unconditional.

phosek updated this revision to Diff 183651.Jan 25 2019, 4:12 PM
phosek updated this revision to Diff 183655.Jan 25 2019, 4:27 PM
phosek marked 2 inline comments as done.
phosek added inline comments.
libunwind/src/libunwind.cpp
30 ↗(On Diff #183637)

Actually I think you're right, if I only provide declaration, Clang handles the definition.

mstorsjo added inline comments.Jan 25 2019, 11:06 PM
libunwind/src/libunwind.cpp
26 ↗(On Diff #183655)

Spelling nitpick: "should not depend on", or "is not dependent on"

This patch breaks compilation of Unwind-seh.cpp, which also needs a declaration of the placement new operator.

phosek updated this revision to Diff 183801.Jan 27 2019, 9:34 PM
phosek edited the summary of this revision. (Show Details)
phosek updated this revision to Diff 183802.Jan 27 2019, 9:37 PM
phosek marked an inline comment as done.

This patch breaks compilation of Unwind-seh.cpp, which also needs a declaration of the placement new operator.

Thanks for checking, this should be fixed now.

mstorsjo accepted this revision.Jan 27 2019, 11:19 PM

This patch breaks compilation of Unwind-seh.cpp, which also needs a declaration of the placement new operator.

Thanks for checking, this should be fixed now.

Thanks, now it does indeed seem to build fine for me.

ldionne accepted this revision.Jan 28 2019, 8:14 AM
ldionne added inline comments.
libunwind/src/libunwind.cpp
15 ↗(On Diff #183802)

Just to confirm -- this is handled by #include <stdlib.h> because we use the global ::getenv() function, right?

phosek marked 2 inline comments as done.Jan 28 2019, 8:32 AM
phosek added inline comments.
libunwind/src/libunwind.cpp
15 ↗(On Diff #183802)

Yes, correct.

This revision was automatically updated to reflect the committed changes.
phosek marked an inline comment as done.
phosek updated this revision to Diff 184012.Jan 28 2019, 8:55 PM

I missed some more C++ library uses in UnwindCursor.hpp and Unwind-EHABI.cpp which broke Arm bots and so I had to revert the change. This should now be addressed. Please let me know if this is okay to submit.

phosek reopened this revision.Jan 28 2019, 8:55 PM
This revision is now accepted and ready to land.Jan 28 2019, 8:55 PM
phosek updated this revision to Diff 184026.Jan 29 2019, 12:11 AM
phosek updated this revision to Diff 184027.
mstorsjo accepted this revision.Jan 29 2019, 12:26 AM

I missed some more C++ library uses in UnwindCursor.hpp and Unwind-EHABI.cpp which broke Arm bots and so I had to revert the change. This should now be addressed. Please let me know if this is okay to submit.

I believe the code looks good, although I haven't tested the EHABI unwinding code after this change to see if it still works.

ldionne accepted this revision.Jan 29 2019, 7:47 AM

As much as I hate duplicating stuff like upper_bound, I find the removal of the circular dependency so nice that I don't care.

As much as I hate duplicating stuff like upper_bound, I find the removal of the circular dependency so nice that I don't care.

I have exactly the same feeling about this one ;)

This revision was automatically updated to reflect the committed changes.