This is an archive of the discontinued LLVM Phabricator instance.

[gn build] Create abi-breaking.h, config.h, llvm-config.h, and add a build file for llvm/lib/Support.
ClosedPublic

Authored by thakis on Nov 18 2018, 11:15 AM.

Details

Summary

The comments at the top of llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn and llvm/utils/gn/build/write_cmake_config.py should explain the main bits happening in this patch. The main parts here are that these headers are generated at build time, not gn time, and that currently they don't do any actual feature checks but just hardcode most things based on the current OS, which seems to work well enough. If this stops being enough, the feature checks should each be their own action writing the result to somewhere, and the config write step should depend on those checks (so that they can run in parallel and as part of the build) -- utils/llvm/gn/README.rst already has some more words on that in "Philosophy".

(write_cmake_config.py is also going to be used to write clang's clang/include/clang/Config/config.h)

This also adds a few files for linking to system libraries in a consistent way if needed in llvm/utils/gn/build/libs (and moves pthread to that model). This might be overkill; I'm not married to that part (but I kind of like it).

I'm also adding llvm/utils/gn/secondary/llvm/lib/Target/targets.gni in this patch because $native_arch is needed for writing llvm-config.h -- the rest of it will be used later, when the build files for llvm/lib/Target get added. That file describes how to select which archs to build.

As a demo, also add a build file for llvm-undname and make it the default build target (it depends on everything that can currently be built).

Diff Detail

Repository
rL LLVM

Event Timeline

thakis created this revision.Nov 18 2018, 11:15 AM
thakis updated this revision to Diff 174547.Nov 18 2018, 3:17 PM

fix a comment

phosek added inline comments.Nov 21 2018, 4:06 PM
llvm/utils/gn/build/libs/zlib/enable.gni
1 ↗(On Diff #174547)

Shouldn't this be an arg as well? It's an option today and various builds disable it.

llvm/utils/gn/build/write_cmake_config.py
48 ↗(On Diff #174547)

Can you pass the documentation above as description and epilog to the ArgumentParser so it gets printed out with --help?

49 ↗(On Diff #174547)

Shouldn't this be just a positional argument?

50 ↗(On Diff #174547)

Would it be possible to add description for all arguments?

51 ↗(On Diff #174547)

What if this argument is unset?

62 ↗(On Diff #174547)

Make it a global constant?

64 ↗(On Diff #174547)

Wrap in with statement so it gets closed automatically.

102 ↗(On Diff #174547)

Ditto

llvm/utils/gn/secondary/llvm/lib/Target/targets.gni
43 ↗(On Diff #174547)

This whole block seems unused.

llvm/utils/gn/secondary/llvm/triples.gni
10 ↗(On Diff #174547)

Definitely, we always set host triple to x86_64-linux-gnu in our build (because nobody has time to type unknown)

thakis updated this revision to Diff 175008.Nov 21 2018, 6:17 PM
thakis marked 4 inline comments as done.

Thanks for the review! Implemented most suggestions.

llvm/utils/gn/build/libs/zlib/enable.gni
1 ↗(On Diff #174547)

Done. I agree it should match llvm_enable_libxml2 which I did make a declare_arg. I actually think it'd make more sense to say "llvm requires libxml2 and zlib on non-Win" instead of dynamically disabling features, but probably makes sense to match cmake.

Out of interest, do builds you care about disable this?

llvm/utils/gn/build/write_cmake_config.py
49 ↗(On Diff #174547)

Is there some guideline what to use for what? Changed; I had it the current way because I found the command easier to read when looking at it with ninja -v.

51 ↗(On Diff #174547)

Made it required.

62 ↗(On Diff #174547)

Why? It's only used here.

64 ↗(On Diff #174547)

It's a short and short-running script, the string will be garbage-collected, and I don't need to indent everything 4 more (or introduce a function that's called from just one place.

102 ↗(On Diff #174547)

This is for consistency with the previous place :-)

llvm/utils/gn/secondary/llvm/lib/Target/targets.gni
43 ↗(On Diff #174547)

From the phab description: "I'm also adding llvm/utils/gn/secondary/llvm/lib/Target/targets.gni in this patch because $native_arch is needed for writing llvm-config.h -- the rest of it will be used later, when the build files for llvm/lib/Target get added. That file describes how to select which archs to build."

Want me to defer adding the bits I need later until I need them?

phosek accepted this revision.Nov 26 2018, 7:10 PM

Few more nits, but otherwise LGTM.

llvm/utils/gn/secondary/llvm/lib/Target/targets.gni
34 ↗(On Diff #175008)

Move this assert into an else branch below? That way you don't have repeat all targets here and in the if conditions below.

53 ↗(On Diff #175008)

Could you also add an error message to make it more clear what's wrong?

43 ↗(On Diff #174547)

I missed that part, I guess this is fine then.

This revision is now accepted and ready to land.Nov 26 2018, 7:10 PM
thakis marked 4 inline comments as done.Nov 26 2018, 9:22 PM

Thanks! Landing.

llvm/utils/gn/secondary/llvm/lib/Target/targets.gni
34 ↗(On Diff #175008)

Much nicer, thanks!

This revision was automatically updated to reflect the committed changes.