[fusl] Build libcxx atop fusl

[fusl] Build libcxx atop fusl

Fixes #657

R=viettrungluu@chromium.org, phosek@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/793afc2496c694122344e4ca496d53cbdc334db9

[kulakowski, 2016-01-15 22:31:14]

This isn't yet complete, but I wanted to get some feedback (especially from
Petr) that this is approximately the right direction.

[Petr Hosek, 2016-01-15 22:39:48]

On 2016/01/15 22:31:14, kulakowski wrote:
> This isn't yet complete, but I wanted to get some feedback (especially from
> Petr) that this is approximately the right direction.

Does it mean that we'll be doing a custom build of libc++? The reason I don't
like this is because it makes synchronization with the upstream more painful,
i.e. everytime you update the upstream revision you need to check all the
changes to CMakeLists.txt in both libc++ and libc++abi and make sure that you
replicate any changes in your GN file. It might not sound like a lot, but these
things quickly add up.

[kulakowski, 2016-01-15 22:44:39]

On 2016/01/15 22:39:48, Petr Hosek wrote:
> On 2016/01/15 22:31:14, kulakowski wrote:
> > This isn't yet complete, but I wanted to get some feedback (especially from
> > Petr) that this is approximately the right direction.
> 
> Does it mean that we'll be doing a custom build of libc++? The reason I don't
> like this is because it makes synchronization with the upstream more painful,
> i.e. everytime you update the upstream revision you need to check all the
> changes to CMakeLists.txt in both libc++ and libc++abi and make sure that you
> replicate any changes in your GN file. It might not sound like a lot, but
these
> things quickly add up.

Given that this is intended only to build a static version of these things for
the short term (i.e. only as long as it makes sense to do this in the
domokit/mojo repo), I'm not that worried.

[Petr Hosek, 2016-01-15 22:46:52]

On 2016/01/15 22:44:39, kulakowski wrote:
> On 2016/01/15 22:39:48, Petr Hosek wrote:
> > On 2016/01/15 22:31:14, kulakowski wrote:
> > > This isn't yet complete, but I wanted to get some feedback (especially
from
> > > Petr) that this is approximately the right direction.
> > 
> > Does it mean that we'll be doing a custom build of libc++? The reason I
don't
> > like this is because it makes synchronization with the upstream more
painful,
> > i.e. everytime you update the upstream revision you need to check all the
> > changes to CMakeLists.txt in both libc++ and libc++abi and make sure that
you
> > replicate any changes in your GN file. It might not sound like a lot, but
> these
> > things quickly add up.
> 
> Given that this is intended only to build a static version of these things for
> the short term (i.e. only as long as it makes sense to do this in the
> domokit/mojo repo), I'm not that worried.

Heh, famous last words: "it's only a temporary solution".

[viettrungluu, 2016-01-15 23:41:50]

https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn
File third_party/libcxx/BUILD.gn (right):

https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
third_party/libcxx/BUILD.gn:40: "-D_LIBCPP_HAS_MUSL_LIBC",
Is this only used in the .cpp files?

(Ditto for some of the other -D lines above.)

https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
third_party/libcxx/BUILD.gn:55: cflags += [
Note: A simple "cflags = [ ... ]" here works and is sufficient (and the "cflags
= []" above becomes unnecessary).

https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
third_party/libcxx/BUILD.gn:56: "--sysroot",
Is this really the right way to do this?

Should we have a toolchain definition with this? (Or at least a config with
this?)

https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
third_party/libcxx/BUILD.gn:97: config("libcxxabi_config") {
I wonder if we should share more of this with libcxx_config.

[kulakowski, 2016-01-16 00:22:11]

On 2016/01/15 23:41:50, viettrungluu wrote:
> https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn
> File third_party/libcxx/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
> third_party/libcxx/BUILD.gn:40: "-D_LIBCPP_HAS_MUSL_LIBC",
> Is this only used in the .cpp files?
> 
> (Ditto for some of the other -D lines above.)
> 
>
https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
> third_party/libcxx/BUILD.gn:55: cflags += [
> Note: A simple "cflags = [ ... ]" here works and is sufficient (and the
"cflags
> = []" above becomes unnecessary).
> 
>
https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
> third_party/libcxx/BUILD.gn:56: "--sysroot",
> Is this really the right way to do this?
> 
> Should we have a toolchain definition with this? (Or at least a config with
> this?)
> 
>
https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
> third_party/libcxx/BUILD.gn:97: config("libcxxabi_config") {
> I wonder if we should share more of this with libcxx_config.

Yeah, most of those things are true. I guess the point of me sending this out
early was to verify that you think doing this, as opposed to making CMake work
on linux with fusl for these, is the way to do it. This is just a very literal
transposition of what happens with its CMake => ninja build path.

[Petr Hosek, 2016-01-16 02:55:02]

https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn
File third_party/libcxx/BUILD.gn (right):

https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
third_party/libcxx/BUILD.gn:40: "-D_LIBCPP_HAS_MUSL_LIBC",
The problem with this approach is that everyone who includes headers from your
build of libcxx will need to define _LIBCPP_HAS_MUSL_LIBC as well which is a
pita.

In upstream libcxx, this is defined inside __config_site which is generated by
CMake from __config_site.in and later merged with __config which is then
included from all header files in libcxx.

Ideally, we would follow the same approach in our build except that doing this
in GN will require a custom action which is going to be a bit ugly.

[kulakowski, 2016-01-25 23:38:55]

On 2016/01/16 02:55:02, Petr Hosek wrote:
> https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn
> File third_party/libcxx/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
> third_party/libcxx/BUILD.gn:40: "-D_LIBCPP_HAS_MUSL_LIBC",
> The problem with this approach is that everyone who includes headers from your
> build of libcxx will need to define _LIBCPP_HAS_MUSL_LIBC as well which is a
> pita.
> 
> In upstream libcxx, this is defined inside __config_site which is generated by
> CMake from __config_site.in and later merged with __config which is then
> included from all header files in libcxx.
> 
> Ideally, we would follow the same approach in our build except that doing this
> in GN will require a custom action which is going to be a bit ugly.

A conclusion is that we are going to need a toolchain definition for all this
stuff, so I'm going to get that done, and then see what's left to be addressed.

[kulakowski, 2016-01-25 23:38:57]

On 2016/01/16 02:55:02, Petr Hosek wrote:
> https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn
> File third_party/libcxx/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
> third_party/libcxx/BUILD.gn:40: "-D_LIBCPP_HAS_MUSL_LIBC",
> The problem with this approach is that everyone who includes headers from your
> build of libcxx will need to define _LIBCPP_HAS_MUSL_LIBC as well which is a
> pita.
> 
> In upstream libcxx, this is defined inside __config_site which is generated by
> CMake from __config_site.in and later merged with __config which is then
> included from all header files in libcxx.
> 
> Ideally, we would follow the same approach in our build except that doing this
> in GN will require a custom action which is going to be a bit ugly.

A conclusion is that we are going to need a toolchain definition for all this
stuff, so I'm going to get that done, and then see what's left to be addressed.

[kulakowski, 2016-02-02 23:57:14]

On 2016/01/25 23:38:57, kulakowski wrote:
> On 2016/01/16 02:55:02, Petr Hosek wrote:
> >
https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn
> > File third_party/libcxx/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
> > third_party/libcxx/BUILD.gn:40: "-D_LIBCPP_HAS_MUSL_LIBC",
> > The problem with this approach is that everyone who includes headers from
your
> > build of libcxx will need to define _LIBCPP_HAS_MUSL_LIBC as well which is a
> > pita.
> > 
> > In upstream libcxx, this is defined inside __config_site which is generated
by
> > CMake from __config_site.in and later merged with __config which is then
> > included from all header files in libcxx.
> > 
> > Ideally, we would follow the same approach in our build except that doing
this
> > in GN will require a custom action which is going to be a bit ugly.
> 
> A conclusion is that we are going to need a toolchain definition for all this
> stuff, so I'm going to get that done, and then see what's left to be
addressed.

This is approximately what I'm thinking now, Petr. Config stuff needs cleaned
up/piped through still. I'll probably also try to build libcxx tests.

[kulakowski, 2016-02-03 20:13:58]

On 2016/02/02 23:57:14, kulakowski wrote:
> On 2016/01/25 23:38:57, kulakowski wrote:
> > On 2016/01/16 02:55:02, Petr Hosek wrote:
> > >
> https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn
> > > File third_party/libcxx/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1596483004/diff/1/third_party/libcxx/BUILD.gn...
> > > third_party/libcxx/BUILD.gn:40: "-D_LIBCPP_HAS_MUSL_LIBC",
> > > The problem with this approach is that everyone who includes headers from
> your
> > > build of libcxx will need to define _LIBCPP_HAS_MUSL_LIBC as well which is
a
> > > pita.
> > > 
> > > In upstream libcxx, this is defined inside __config_site which is
generated
> by
> > > CMake from __config_site.in and later merged with __config which is then
> > > included from all header files in libcxx.
> > > 
> > > Ideally, we would follow the same approach in our build except that doing
> this
> > > in GN will require a custom action which is going to be a bit ugly.
> > 
> > A conclusion is that we are going to need a toolchain definition for all
this
> > stuff, so I'm going to get that done, and then see what's left to be
> addressed.
> 
> This is approximately what I'm thinking now, Petr. Config stuff needs cleaned
> up/piped through still. I'll probably also try to build libcxx tests.

Petr, ptal.

In particular I'm not quite sure how to get the linker to add -lc++ and -lc++abi
for C++ but not for C executables, without manually adding them to libs as I did
here. But this works, the two things compile, link, and run.

[Petr Hosek, 2016-02-04 01:05:10]

If you set -stdlib=libc++, Clang driver should automatically pass -lc++ and
-lc++abi to the linker and find them in the location pointed to by --sysroot.

https://codereview.chromium.org/1596483004/diff/100001/fusl/BUILD.gn
File fusl/BUILD.gn (right):

https://codereview.chromium.org/1596483004/diff/100001/fusl/BUILD.gn#newcode1869
fusl/BUILD.gn:1869: executable("vector") {
If this is a test, it should probably be a `test` rather than `executable`.

[kulakowski, 2016-02-04 18:07:24]

On 2016/02/04 01:05:10, Petr Hosek wrote:
> If you set -stdlib=libc++, Clang driver should automatically pass -lc++ and
> -lc++abi to the linker and find them in the location pointed to by --sysroot.
> 

I am setting that (via sysroot_config) and it indeed works if clang++ is the
linker driver. But if clang++, rather than clang, is the linker driver, then it
wants to add those flags even when building C executables (e.g. empty_main). I'd
definitely prefer not to pass in -lc++ for C++, but I don't know how to tell gn
that an executable is C.

[Petr Hosek, 2016-02-04 18:11:26]

On 2016/02/04 18:07:24, kulakowski wrote:
> On 2016/02/04 01:05:10, Petr Hosek wrote:
> > If you set -stdlib=libc++, Clang driver should automatically pass -lc++ and
> > -lc++abi to the linker and find them in the location pointed to by
--sysroot.
> > 
> 
> I am setting that (via sysroot_config) and it indeed works if clang++ is the
> linker driver. But if clang++, rather than clang, is the linker driver, then
it
> wants to add those flags even when building C executables (e.g. empty_main).
I'd
> definitely prefer not to pass in -lc++ for C++, but I don't know how to tell
gn
> that an executable is C.

AFAIK that should be based on the file extension, i.e. if the file has .c
extension
it should be compiled using $cc, and if it's .cc/.cpp it should be $cxx.

[kulakowski, 2016-02-04 18:15:53]

On 2016/02/04 18:11:26, Petr Hosek wrote:
> On 2016/02/04 18:07:24, kulakowski wrote:
> > On 2016/02/04 01:05:10, Petr Hosek wrote:
> > > If you set -stdlib=libc++, Clang driver should automatically pass -lc++
and
> > > -lc++abi to the linker and find them in the location pointed to by
> --sysroot.
> > > 
> > 
> > I am setting that (via sysroot_config) and it indeed works if clang++ is the
> > linker driver. But if clang++, rather than clang, is the linker driver, then
> it
> > wants to add those flags even when building C executables (e.g. empty_main).
> I'd
> > definitely prefer not to pass in -lc++ for C++, but I don't know how to tell
> gn
> > that an executable is C.
> 
> AFAIK that should be based on the file extension, i.e. if the file has .c
> extension
> it should be compiled using $cc, and if it's .cc/.cpp it should be $cxx.

I believe that does work if your invocation looks like

clang -o foo foo1.c foo2.c

but our invocation looks more like

clang -o foo ... @foo.rsp ...

where foo.rsp contains only object file names, and so the C-ness has been lost
by this point.

[kulakowski, 2016-02-04 21:03:35]

On 2016/02/04 18:15:53, kulakowski wrote:
> On 2016/02/04 18:11:26, Petr Hosek wrote:
> > On 2016/02/04 18:07:24, kulakowski wrote:
> > > On 2016/02/04 01:05:10, Petr Hosek wrote:
> > > > If you set -stdlib=libc++, Clang driver should automatically pass -lc++
> and
> > > > -lc++abi to the linker and find them in the location pointed to by
> > --sysroot.
> > > > 
> > > 
> > > I am setting that (via sysroot_config) and it indeed works if clang++ is
the
> > > linker driver. But if clang++, rather than clang, is the linker driver,
then
> > it
> > > wants to add those flags even when building C executables (e.g.
empty_main).
> > I'd
> > > definitely prefer not to pass in -lc++ for C++, but I don't know how to
tell
> > gn
> > > that an executable is C.
> > 
> > AFAIK that should be based on the file extension, i.e. if the file has .c
> > extension
> > it should be compiled using $cc, and if it's .cc/.cpp it should be $cxx.
> 
> I believe that does work if your invocation looks like
> 
> clang -o foo foo1.c foo2.c
> 
> but our invocation looks more like
> 
> clang -o foo ... @foo.rsp ...
> 
> where foo.rsp contains only object file names, and so the C-ness has been lost
> by this point.

PTAL

This now builds both C and C++ executables without having to do anything other
than use the correct config. It might be possible to make those configs share
definitions.

Backing up a bit, I believe I still have to address your comments about the MUSL
macro stuff in libcxx.

[kulakowski, 2016-02-04 23:03:53]

On 2016/02/04 21:03:35, kulakowski wrote:
> On 2016/02/04 18:15:53, kulakowski wrote:
> > On 2016/02/04 18:11:26, Petr Hosek wrote:
> > > On 2016/02/04 18:07:24, kulakowski wrote:
> > > > On 2016/02/04 01:05:10, Petr Hosek wrote:
> > > > > If you set -stdlib=libc++, Clang driver should automatically pass
-lc++
> > and
> > > > > -lc++abi to the linker and find them in the location pointed to by
> > > --sysroot.
> > > > > 
> > > > 
> > > > I am setting that (via sysroot_config) and it indeed works if clang++ is
> the
> > > > linker driver. But if clang++, rather than clang, is the linker driver,
> then
> > > it
> > > > wants to add those flags even when building C executables (e.g.
> empty_main).
> > > I'd
> > > > definitely prefer not to pass in -lc++ for C++, but I don't know how to
> tell
> > > gn
> > > > that an executable is C.
> > > 
> > > AFAIK that should be based on the file extension, i.e. if the file has .c
> > > extension
> > > it should be compiled using $cc, and if it's .cc/.cpp it should be $cxx.
> > 
> > I believe that does work if your invocation looks like
> > 
> > clang -o foo foo1.c foo2.c
> > 
> > but our invocation looks more like
> > 
> > clang -o foo ... @foo.rsp ...
> > 
> > where foo.rsp contains only object file names, and so the C-ness has been
lost
> > by this point.
> 
> PTAL
> 
> This now builds both C and C++ executables without having to do anything other
> than use the correct config. It might be possible to make those configs share
> definitions.
> 
> Backing up a bit, I believe I still have to address your comments about the
MUSL
> macro stuff in libcxx.

Ok, I now think everything is addressed. C++ programs don't have to declare that
macro themselves, just pull in the right config which they need for the sysroot
anyway. I also fixed a typo that was making that action run every time.

It's annoying that C programs need the 2nd config for the -libc stuff, but I
don't see another way to do it, besides defining a whole 'nother toolchain.

[kulakowski, 2016-02-05 21:51:13]

Description was changed from

==========
Build libcxx atop fusl

R=viettrungluu@chromium.org, phosek@chromium.org
==========

to

==========
Build libcxx atop fusl

Fixes #657

R=viettrungluu@chromium.org, phosek@chromium.org
==========

[kulakowski, 2016-02-05 21:51:21]

https://codereview.chromium.org/1596483004/diff/180001/fusl/BUILD.gn
File fusl/BUILD.gn (right):

https://codereview.chromium.org/1596483004/diff/180001/fusl/BUILD.gn#newcode1633
fusl/BUILD.gn:1633: "$target/crtbeginS.o",
Trung, this was the problem with the build that you reported in
https://github.com/domokit/mojo/issues/657

[kulakowski, 2016-02-08 21:14:26]

https://codereview.chromium.org/1596483004/diff/220001/fusl/BUILD.gn
File fusl/BUILD.gn (right):

https://codereview.chromium.org/1596483004/diff/220001/fusl/BUILD.gn#newcode1856
fusl/BUILD.gn:1856: "-nodefaultlibs",
So gn really wasn't built to create C executables, hence this bit of
shenanigans. With the way this patch is structured now, C++ code building
against fusl just needs one config, and C needs this one as well.

If either of you know another way to structure this I can restructure this
patch, but otherwise I think I've addressed all the feedback from previous
review.

[kulakowski, 2016-02-09 22:48:00]

Description was changed from

==========
Build libcxx atop fusl

Fixes #657

R=viettrungluu@chromium.org, phosek@chromium.org
==========

to

==========
[fusl] Build libcxx atop fusl

Fixes #657

R=viettrungluu@chromium.org, phosek@chromium.org
==========

[viettrungluu, 2016-02-10 15:59:31]

lgtm w/nit

https://codereview.chromium.org/1596483004/diff/240001/fusl/tools/copy_libcxx...
File fusl/tools/copy_libcxx_headers.py (right):

https://codereview.chromium.org/1596483004/diff/240001/fusl/tools/copy_libcxx...
fusl/tools/copy_libcxx_headers.py:11: """Copy a tree.
nit: 2-space indents!*#@#@$?! :(

https://codereview.chromium.org/1596483004/diff/240001/fusl/tools/copy_libcxx...
fusl/tools/copy_libcxx_headers.py:21: if os.path.isfile(source):
Things will get sad if "foo" was a file and is changed to being a directory.
Also, I don't know what happens if "foo" is a symlink. <shrug> Let's hope that
doesn't happen.

[kulakowski, 2016-02-10 19:52:35]

https://codereview.chromium.org/1596483004/diff/240001/fusl/tools/copy_libcxx...
File fusl/tools/copy_libcxx_headers.py (right):

https://codereview.chromium.org/1596483004/diff/240001/fusl/tools/copy_libcxx...
fusl/tools/copy_libcxx_headers.py:11: """Copy a tree.
On 2016/02/10 15:59:31, viettrungluu wrote:
> nit: 2-space indents!*#@#@$?! :(

Done.

[kulakowski, 2016-02-10 19:52:45]

Description was changed from

==========
[fusl] Build libcxx atop fusl

Fixes #657

R=viettrungluu@chromium.org, phosek@chromium.org
==========

to

==========
[fusl] Build libcxx atop fusl

Fixes #657

R=viettrungluu@chromium.org, phosek@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/793afc2496c694122344e4ca496...
==========

[kulakowski, 2016-02-10 19:52:46]

Committed patchset #14 (id:260001) manually as
793afc2496c694122344e4ca496d53cbdc334db9 (presubmit successful).