Page MenuHomePhabricator

Add initial scaffolding for the GN build.
ClosedPublic

Authored by thakis on Nov 9 2018, 12:31 PM.

Details

Summary

See "GN build roundtable summary; adding GN build files to the repo" on llvm-dev and cfe-dev for discussion.

In particular, this build is completely unsupported. People adding new files to LLVM are not expected to update the GN build files, and reviewers are not supposed to request the gn build files to be updated.

This adds just enough to be able to build llvm/lib/Demangle. It requires using a monorepo.

This adds a few build config options you can set in args.gn (gn args out/foo --list for all):

  • is_debug = true to enable debug builds (defaults to release)
  • llvm_enable_assertions to toggle assertions (defaults to true)
  • clang_base_path, if set an absolute path to a locally-built clang to be used as host compiler, e.g. clang_base_path = getenv("HOME") + "/src/chrome/src/third_party/llvm-build/Release+Asserts"

Diff Detail

Repository
rL LLVM

Event Timeline

thakis created this revision.Nov 9 2018, 12:31 PM

just one symlink

fix bad exec_script_whitelist default

thakis updated this revision to Diff 173428.Nov 9 2018, 1:03 PM

zero symlinks, in exchange for longer gen command

Shouldn't all the GN files have a copyright header?

llvm/utils/gn/.gn
9 ↗(On Diff #173428)

The convention is to use the name secondary, i.e. //llvm/utils/gn/secondary.

11 ↗(On Diff #173428)

I prefer llvm/contrib/gn as suggested on the list over utils.

llvm/utils/gn/README.rst
90 ↗(On Diff #173428)

nit: capitalize GN here and below

llvm/utils/gn/build/BUILD.gn
6 ↗(On Diff #173428)

I'd prefer to break these into smaller configs so they can be added/removed independently rather than using nested if/else branches, but it's something that can be probably done later as a cleanup.

llvm/utils/gn/build/buildflags.gni
9 ↗(On Diff #173428)

Shouldn't this be llvm_enable_assertions = is_debug?

llvm/utils/gn/build/mac_sdk.gni
4 ↗(On Diff #173428)

Make it an argument so it can be overriden if needed? Also maybe call it mac_sdk_path so it's obvious that it's a path.

llvm/utils/gn/build/toolchain/BUILD.gn
3 ↗(On Diff #173428)

This should be moved to a separate .gni file, see below.

12 ↗(On Diff #173428)

I'd prefer moving these into a template and then define the concrete instance using those templates like most GN builds do. I think it makes it easier to define different toolchains, e.g. for clang and gcc or clang-cl and cl.

Also "posix" is kind of a strange name, Fuchsia also uses the GCC style frontend but we're not POSIX :P I'd slightly prefer using the name Chromium convention to name these after the flag style, i.e. gcc_toolchain or cl_toolchain.

47 ↗(On Diff #173428)

We should use response files here to pass the list of inputs.

49 ↗(On Diff #173428)

Why not use ar on macOS as well?

51 ↗(On Diff #173428)

Can we make ar variable as well? I'd like to change this, e.g. for Clang toolchains to use llvm-ar.

53 ↗(On Diff #173428)

nit: I'd just use AR as the name.

55 ↗(On Diff #173428)

Can you also set default_output_extension = ".a" and use {{target_output_name}}{{output_extension}} here?

61 ↗(On Diff #173428)

Ditto here, use response files.

65 ↗(On Diff #173428)

The -Wl,-z,defs should be handled through compiler config, not here.

You also want {{solibs}} beside {{libs}} here.

73 ↗(On Diff #173428)

Why do you prefer this over specifying the library with a full path?

76 ↗(On Diff #173428)

nit: I'd combine this branch with the one above.

103 ↗(On Diff #173428)

Response file here as well.

llvm/utils/gn/build/toolchain/compiler.gni
3 ↗(On Diff #173428)

Can we split this into a separate file, e.g. goma.gni? This is a one we use in Fuchsia: https://fuchsia.googlesource.com/build/+/master/toolchain/goma.gni

llvm/utils/gn/tree/llvm/lib/Demangle/BUILD.gn
2 ↗(On Diff #173428)

Setting output_name is generally frowned upon as an anti-pattern that should be only used when absolutely necessary, I'd prefer to just set the name of the target to LLVMDemangle.

thakis marked 2 inline comments as done.

address comments

Thanks for the thorough review! I replied "not needed yet" to several items, I hope that's fine.

I added a few comments to the .gn file, and I realized that I can put the root BUILD.gn file in gn/secondary and then I don't need to set root, so I did that.

Shouldn't all the GN files have a copyright header?

Maybe! But all the cmake files don't have them either, so I'm not sure.

llvm/utils/gn/.gn
9 ↗(On Diff #173428)

Done. I think guessing the meaning of a directory called "secondary" is super hard if you don't know about this one line of gn code here, and "tree" is much more self-explanatory, but /shrug.

11 ↗(On Diff #173428)

I agree it'd be a good place if there was a contrib dir, but there isn't, so utils is a better place imo. (My reading of the thread is that someone said that some projects have a contrib dir and that would be a good place if it existed, and someone else saying that that might be a good place for a few other things too, but nobody really suggested putting this in a hypothetical contrib dir, since it doesn't exist yet.)

Anyhoo, this is a bit of a bikeshed and easy to change later if someone feels we need contrib in addition to tools and utils. For now, this location is as good as most others imho, and it's also one dir suggested in that thread :-)

llvm/utils/gn/build/BUILD.gn
6 ↗(On Diff #173428)

Ack. I'm happy with folks tweaking this to their heart's content later on.

llvm/utils/gn/build/buildflags.gni
9 ↗(On Diff #173428)

It's at least intentional; from README.rst: "By default, you get a release build with assertions enabled that targets the host arch". That's imho the best config for day-to-day development, which is what we're targeting here. I don't feel strongly about this, but it's imho the best default.

llvm/utils/gn/build/mac_sdk.gni
4 ↗(On Diff #173428)

We can make it an argument once someone needs to override it. Until then, YAGNI :-)

But I did rename it, that's a better name.

llvm/utils/gn/build/toolchain/BUILD.gn
3 ↗(On Diff #173428)

Replied below.

12 ↗(On Diff #173428)

I expect that this will be necessary once we do cross builds, bootstrap builds, etc. Until then, I'd like to keep this as-is if that's alright – then blame history will show where the template came from and why it's there.

Re "posix": We used to have LLVM_ON_WIN32 and LLVM_ON_POSIX (we still have the latter), so these names were consistent with that. I don't care much about the name, and it's easy to change (referenced in 2 places I think).

47 ↗(On Diff #173428)

LLVM is small enough that they aren't needed (at least for the parts I've hooked up), and if you can get away without them that's preferable: You can just copy failing link commands around without having to pass -d keeprsp to ninja, and it's easier to see the full link command.

49 ↗(On Diff #173428)

Last I checked (a few years ago), libtool -static was considerably faster than ar.

I just re-checked and it's still a tiny bit faster. It's _much_ faster if I don't rm the file first, which points out that the ar branch below doesn't rm first -- so thanks for the comment, the other branch had a bug :-) Fixed!

51 ↗(On Diff #173428)

Sure, but let's do that when we need it?

53 ↗(On Diff #173428)

Sure, done.

55 ↗(On Diff #173428)

I haven't found a place where I had to override this yet.

61 ↗(On Diff #173428)

Same answer, don't need it (yet?) and more convenient to not use them when not needed.

65 ↗(On Diff #173428)

I consider -Wl,-z,defs not being the default a bug :-) Unless something wants to disable that, it can stay here.

{{solibs}} isn't here because I haven't implemented the llvm-as-lots-of-shared-objects, and so nothing needs that atm.

73 ↗(On Diff #173428)

It's how I write my commands by hand (cc foo.cc -lbar), and it's what chromium uses. No strong preference though.

103 ↗(On Diff #173428)

(same answer)

llvm/utils/gn/build/toolchain/compiler.gni
3 ↗(On Diff #173428)

If you use goma, you need to use some compiler locally that's available remotely, i.e. you'll have to set clang_base_path. So it's not clear to me if that should be in goma.gni or here. If it's in goma.gni, then I'd have to include that here for the is_clang default value below. If it's here, then I'd have to put an awkward "see clang_base_path in that other file" comment in goma.gni. So I figured one file is simpler, especially since it's just a single arg.

llvm/utils/gn/tree/llvm/lib/Demangle/BUILD.gn
2 ↗(On Diff #173428)

I don't frown upon it :-)

The options here are:

  1. Set the target name to LLVMDemangle. This means we'd have to refer to everything as "llvm/lib/foo:LLVMFoo" instead of "llvm/lib/foo", which is annoying.
  1. Don't set output_name and have output files different from the cmake build. IIRC, llvm-config returns the names of these files and something tests for that, so that's kind of annoying too.
  1. As-is, which has no downside other than apparently someone frowning upon it.
phosek added inline comments.Nov 14 2018, 10:20 AM
llvm/utils/gn/build/toolchain/BUILD.gn
169 ↗(On Diff #173848)

Can we use lld-link if clang_base_path is set?

51 ↗(On Diff #173428)

Why not use llvm-ar if the clang_base_path is set? I don't think there's any reason not to?

llvm/utils/gn/secondary/llvm/lib/Demangle/BUILD.gn
2 ↗(On Diff #173848)

Setting output_name explicitly is generally considered an anti-pattern and should be used only when absolutely necessary. I think we should just "LLVMDemangle" as the target name.

thakis marked an inline comment as done.

address comments

llvm/utils/gn/build/toolchain/BUILD.gn
169 ↗(On Diff #173848)

Sure, why not :-)

51 ↗(On Diff #173428)

My use case for clang_base_path is using goma with chromium's clang package, which doesn't include llvm-ar.

llvm/utils/gn/secondary/llvm/lib/Demangle/BUILD.gn
2 ↗(On Diff #173848)
thakis added inline comments.Nov 15 2018, 11:00 AM
llvm/utils/gn/build/toolchain/BUILD.gn
12 ↗(On Diff #173428)

Turns out this is a lie, it's LLVM_ON_UNIX. I'll rename this to "unix".

rename toolchain to "unix"

phosek added inline comments.Nov 16 2018, 8:30 AM
llvm/utils/gn/secondary/llvm/lib/Demangle/BUILD.gn
2 ↗(On Diff #173848)

I missed that reply (Phabricator no longer shows that file because it was moved).

There are two downsides to manually specifying output_name:

  1. You have to manually type it in each target which is also kind of annoying.
  2. It's difficult to derive the output name from the target name which may be needed when dealing with packaging, copy rules, etc. (this is potentially YAGNI, but it's something we ran into Fuchsia on multiple occasions).
thakis added inline comments.Nov 16 2018, 8:39 AM
llvm/utils/gn/secondary/llvm/lib/Demangle/BUILD.gn
2 ↗(On Diff #173848)

I volunteer to change things around if we should need it for packaging or something at some point. For now, I'd prefer to keep this as-is since all the build files I've written currently use this pattern and there isn't really a reason for changing it at this point.

phosek accepted this revision.Nov 16 2018, 5:31 PM

LGTM

This revision is now accepted and ready to land.Nov 16 2018, 5:31 PM
This revision was automatically updated to reflect the committed changes.