This is an archive of the discontinued LLVM Phabricator instance.

libunwind: Introduce __libunwind_config.h.
ClosedPublic

Authored by logan on Jul 15 2015, 11:14 AM.

Details

Summary

Introduce __libunwind_config.h to avoid cross repository circular
dependency with libcxxabi.

Diff Detail

Event Timeline

logan updated this revision to Diff 29801.Jul 15 2015, 11:14 AM
logan retitled this revision from to libunwind: Introduce __libunwind_config.h..
logan updated this object.
logan added reviewers: asl, joerg, rengolin, danalbert, compnerd.
logan added a subscriber: cfe-commits.
danalbert edited edge metadata.Jul 15 2015, 11:35 AM

Hmm. Looks like there will need to be some cleanup in libcxx/cmake/Modules/HandleLibCXXABI.cmake before this can happen.

Right now the C++ ABI headers get installed as part of the libc++ build, and we adjust that as necessary for each ABI library we support, but there isn't any support to vary it by unwinder (and really the whole thing is a hack).

@danalbert:

May you further elaborate your idea? I don't quite understand why should we install the headers of libunwind for libcxx?

rengolin edited edge metadata.Jul 15 2015, 12:01 PM

Hi Logan,

Is this replacing or complementing the libc++abi definitions?

I was expecting some changes in the CMake files of the projects describing the lack of dependency, or is that done automatically?

cheers,
--renato

danalbert accepted this revision.Jul 16 2015, 10:30 AM
danalbert edited edge metadata.

Actually, I don't think there's an issue here from the libc++ side of things; we already don't install any of the libunwind headers (though libcxxrt does, so maybe we should?).

My concern was: https://github.com/llvm-mirror/libcxx/blob/master/cmake/Modules/HandleLibCXXABI.cmake#L92

We install libc++abi headers there, and for libcxxrt we also install the unwind headers there. Since this isn't already broken, presumably libunwind is already properly separated to not need them there.

Still, as Renato says, we should be installing the libunwind headers somewhere. Since we don't do any of that yet though, I think it's reasonable to save that for a different patch.

This revision is now accepted and ready to land.Jul 16 2015, 10:30 AM
rengolin accepted this revision.Jul 17 2015, 3:33 AM
rengolin edited edge metadata.

Since we don't do any of that yet though, I think it's reasonable to save that for a different patch.

Agreed. LGTM.

asl accepted this revision.Jul 17 2015, 10:41 AM
asl edited edge metadata.

+1

compnerd accepted this revision.Jul 17 2015, 2:12 PM
compnerd edited edge metadata.
logan closed this revision.Jul 19 2015, 8:23 AM

Thanks for reviewing!

Committed as rL242642.