Page MenuHomePhabricator

[gn] Support for building libc++abi
ClosedPublic

Authored by phosek on Apr 6 2019, 7:19 PM.

Details

Summary

This change introduces support for building libc++abi. The library
build should be complete, but not all CMake options have been
replicated in GN. We also don't support tests yet.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Apr 6 2019, 7:19 PM
Herald added a project: Restricted Project. · View Herald Transcript
phosek edited the summary of this revision. (Show Details)Apr 6 2019, 7:19 PM

Basically lgtm, some questions about the declare_args (mirroring https://reviews.llvm.org/D60253#1456632 a bit)

llvm/utils/gn/secondary/libcxxabi/src/BUILD.gn
5 ↗(On Diff #194050)

Does this one have to be settable? That seems like an implementation detail from a distance.

17 ↗(On Diff #194050)

Wouldn't you always want this? What's an example where one would want to statically link c++abi but export its symbols? They only thing I can think of is when linking it into libc++.dylib, which which case we probably want a toggle for that instead?

Why is the default false? Why is this settable?

38 ↗(On Diff #194050)

no trailing "

ldionne requested changes to this revision.Apr 8 2019, 9:04 AM

LLVM uses CMake as a build system, and we should not start checking-in support for other build systems. This sort of thing is what's fine to keep downstream only, otherwise there's an implication that upstream should somehow support that build system, which isn't the case. For example, when we make a change to CMake, we don't want to have to update the gn-based build system.

Similarly, we use a Xcode project to build libc++/libc++abi/libunwind, but we don't check those in upstream (and I've been working hard to switch us over to the CMake build, actually).

This revision now requires changes to proceed.Apr 8 2019, 9:04 AM
ldionne resigned from this revision.Apr 8 2019, 9:08 AM

Hmm, sorry, I missed the fact that there was already this support for other components. Well, as long as there's no assumption that we'll maintain this in any way, I'm not concerned with this.

jfb added a comment.Apr 8 2019, 9:28 AM

For those without context, it would be really helpful if every GN commit had a link to documentation about what GN is, and how LLVM supports it. The initial commit had a note, maybe the LLVM docs should have this?

In D60372#1458540, @jfb wrote:

For those without context, it would be really helpful if every GN commit had a link to documentation about what GN is, and how LLVM supports it. The initial commit had a note, maybe the LLVM docs should have this?

llvm/utils/gn/README.rst has a readme with links etc. It explicitly mentions that this build config is unsupported and that there's no expectation to keep that build up to date, as requested by ldionne.

In D60372#1458540, @jfb wrote:

For those without context, it would be really helpful if every GN commit had a link to documentation about what GN is, and how LLVM supports it. The initial commit had a note, maybe the LLVM docs should have this?

llvm/utils/gn/README.rst has a readme with links etc. It explicitly mentions that this build config is unsupported and that there's no expectation to keep that build up to date, as requested by ldionne.

Yup, my bad, I just spoke a little too fast.

In D60372#1458540, @jfb wrote:

For those without context, it would be really helpful if every GN commit had a link to documentation about what GN is, and how LLVM supports it. The initial commit had a note, maybe the LLVM docs should have this?

llvm/utils/gn/README.rst has a readme with links etc. It explicitly mentions that this build config is unsupported and that there's no expectation to keep that build up to date, as requested by ldionne.

Yup, my bad, I just spoke a little too fast.

Can you clean the "Needs Revision" please? :)

ldionne accepted this revision.Apr 8 2019, 11:05 AM
In D60372#1458540, @jfb wrote:

For those without context, it would be really helpful if every GN commit had a link to documentation about what GN is, and how LLVM supports it. The initial commit had a note, maybe the LLVM docs should have this?

llvm/utils/gn/README.rst has a readme with links etc. It explicitly mentions that this build config is unsupported and that there's no expectation to keep that build up to date, as requested by ldionne.

Yup, my bad, I just spoke a little too fast.

Can you clean the "Needs Revision" please? :)

I thought it would remove it upon resigning from the revision. I accepted the revision since there didn't seem to be a way of just removing my "request for changes". Please ignore my approval, too.

This revision is now accepted and ready to land.Apr 8 2019, 11:05 AM
phosek updated this revision to Diff 196710.Apr 25 2019, 1:05 PM
phosek marked 3 inline comments as done.
phosek retitled this revision from [gn] Support for building libcxxabi to [gn] Support for building libc++abi.
phosek edited the summary of this revision. (Show Details)
phosek added inline comments.Apr 25 2019, 1:05 PM
llvm/utils/gn/secondary/libcxxabi/src/BUILD.gn
5 ↗(On Diff #194050)

We have a concrete use case for this in Fuchsia where we build multiple different variants of libc++ using the multilib layout, including one without exceptions support, see D60926.

17 ↗(On Diff #194050)

I've tried to match the CMake build where this is disabled by default. I agree with your reasoning but I'm not sure whether we want to deviate from the CMake build?

thakis added inline comments.Apr 26 2019, 4:54 AM
llvm/utils/gn/secondary/libcxxabi/src/BUILD.gn
17 ↗(On Diff #194050)

I think it's fine to deviate from CMake where it makes sense.

  • there are many examples of things that are options in cmake but not in gn (usually, because noone needed the option in GN yet, but not always)
  • there are a few examples where variable defaults are different because cmake's defaults are arguably not optimal for the average case (examples: gn gives you release+asserts by default while cmake gives you debug by default, gn only builds host target by default while cmake builds all of them by default, ...)

So if that's the only reason this is the way it is, I think it's fine to change it if you agree that that makes sense.

phosek marked an inline comment as done.Apr 29 2019, 11:41 AM
phosek added inline comments.
llvm/utils/gn/secondary/libcxxabi/src/BUILD.gn
17 ↗(On Diff #194050)

It depends on the expected use case. If you want to build static libc++abi so you can link it into the shared libc++, then you want this option to be disabled. If you want to build static libc++abi so you can distribute it as an archive, you probably want it enabled. It seems like CMake build is optimized for the former, but in practice I'd argue that the latter is more common so I'm fine making that the default.

thakis accepted this revision.May 1 2019, 7:42 AM

Sorry, didn't realize I hadn't accepted this yet. lgtm with the change you had suggested below.

llvm/utils/gn/secondary/libcxxabi/src/BUILD.gn
17 ↗(On Diff #194050)

Sounds good.

This revision was automatically updated to reflect the committed changes.