This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Convert the NATIVE llvm build process to be project agnostic
ClosedPublic

Authored by lanza on Jul 16 2019, 3:50 PM.

Details

Summary

lldb recently added a tablegen tool. In order to properly cross compile
lldb standalone there needs to be a mechanism to generate the native
lldb build, analgous to what's done for the NATIVE llvm build. Thus,
we can simply modify this setup to allow for any project to be used.

Diff Detail

Event Timeline

lanza created this revision.Jul 16 2019, 3:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 3:50 PM

Still testing the builds, just figured I'd put it up early.

smeenai added inline comments.Jul 16 2019, 4:03 PM
cmake/modules/CrossCompile.cmake
86–92

Does it hurt anything if these flags are included for other projects?

I'm pretty sure that this doesn't work for Visual Studio (where you do not have a cross-compiler).

lanza retitled this revision from [wip] Convert the NATIVE llvm build process to be project agnostic to [cmake] Convert the NATIVE llvm build process to be project agnostic.Jul 16 2019, 7:52 PM
lanza added reviewers: beanz, phosek.
lanza marked an inline comment as done.Jul 16 2019, 7:54 PM

I'm pretty sure that this doesn't work for Visual Studio (where you do not have a cross-compiler).

It already doesn't work for Visual Studio. This change doesn't change that.

This just seems like it's replacing all instances of LLVM with ${CMAKE_PROJECT_NAME}?

I think it'd be cleaner and more explicit to pass an additional project name parameter to llvm_create_cross_target instead of implicitly using ${CMAKE_PROJECT_NAME}. There's some prior art for that, e.g. in https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/cmake/modules/AddLLVM.cmake;366290$1009?as=source&blame=off

smeenai added inline comments.Jul 16 2019, 10:25 PM
cmake/modules/CrossCompile.cmake
58

You'll wanna change this one to read from a project-specific cache variable too. You'll also wanna preserve backwards compatibility for the people using the old spelling.

This just seems like it's replacing all instances of LLVM with ${CMAKE_PROJECT_NAME}?

I think it'd be cleaner and more explicit to pass an additional project name parameter to llvm_create_cross_target instead of implicitly using ${CMAKE_PROJECT_NAME}. There's some prior art for that, e.g. in https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/cmake/modules/AddLLVM.cmake;366290$1009?as=source&blame=off

That's what I did originally. However, the users of build_natve_tool wouldn't have the proper context to know what to use. It's only non-llvm specific usage is within add_tablegen which would then have to learn how to decide what project_name variable to pass in. e.g. if lldb is part of ENABLE_PROJECTS then add_tablegen should call llvm_native_build(LLVM NATIVE ...) else llvm_native_build(LLDB NATIVE ...). So we'd have to propagate that to all users of add_tablegen where we'd have to figure out whether the build is standalone or not via checking CMAKE_PROJECT_NAME anyways. e.g.

 if (${CMAKE_PROJECT_NAME} MATCHES lldb)
    add_tablegen(lldb lldb lldb-tblgen)
else()
    add_tablegen(llvm lldb lldb-tblgen)
endif()

I'm not convinced that adding the granularity to choose your llvm_create_cross_target project is worth propagating the configuration outwards given that there are no users that don't satisfy CMAKE_PROJECT_NAME == project_name.

An alternative would be to have some global variable llvm_cross_targets list that build_native_tool iterates over. But this is making an already clumsy solution more clumsy.

@smeenai I think if this change works then for now we can just land it because we have several bots to turn green in the short term. Would you be fine with allowing for this fix in the near term?

beanz added inline comments.Jul 17 2019, 1:43 PM
cmake/modules/CrossCompile.cmake
58

I think you probably want to make CROSS_TOOLCHAIN_FLAGS_${target_name} generic, not project specific, and only add a project-specific variant if it is needed.

It is entirely possible to use the cross-compile code to generate multiple different cross-projects for the same target where you do want the same target flags, because those flags relate to the target not the project you're building.

lanza updated this revision to Diff 210439.Jul 17 2019, 3:26 PM

adjust for beanz comments

lanza marked 2 inline comments as done.Jul 17 2019, 3:27 PM

This just seems like it's replacing all instances of LLVM with ${CMAKE_PROJECT_NAME}?

I think it'd be cleaner and more explicit to pass an additional project name parameter to llvm_create_cross_target instead of implicitly using ${CMAKE_PROJECT_NAME}. There's some prior art for that, e.g. in https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/cmake/modules/AddLLVM.cmake;366290$1009?as=source&blame=off

That's what I did originally. However, the users of build_natve_tool wouldn't have the proper context to know what to use. It's only non-llvm specific usage is within add_tablegen which would then have to learn how to decide what project_name variable to pass in. e.g. if lldb is part of ENABLE_PROJECTS then add_tablegen should call llvm_native_build(LLVM NATIVE ...) else llvm_native_build(LLDB NATIVE ...). So we'd have to propagate that to all users of add_tablegen where we'd have to figure out whether the build is standalone or not via checking CMAKE_PROJECT_NAME anyways. e.g.

 if (${CMAKE_PROJECT_NAME} MATCHES lldb)
    add_tablegen(lldb lldb lldb-tblgen)
else()
    add_tablegen(llvm lldb lldb-tblgen)
endif()

I'm not convinced that adding the granularity to choose your llvm_create_cross_target project is worth propagating the configuration outwards given that there are no users that don't satisfy CMAKE_PROJECT_NAME == project_name.

An alternative would be to have some global variable llvm_cross_targets list that build_native_tool iterates over. But this is making an already clumsy solution more clumsy.

I missed the build_native_tool thing. In that case, I would have been fine with sticking to ${CMAKE_PROJECT_NAME} inside llvm_create_cross_target as well, but it looks like you've already changed everything to use a project name variable :)

cmake/modules/CrossCompile.cmake
26

Are you missing an underscore here? Should the variable be CROSS_TOOLCHAIN_FLAGS_${project_name}_${target_name}?

58

Makes sense. I can see there being a need for project-specific variables as well.

@lanza you should definitely keep the original ${CROSS_TOOLCHAIN_FLAGS_${target_name}} spelling here. You may also want to add the ${CROSS_TOOLCHAIN_FLAGS_${project_name}_${target_name}} spelling if e.g. there's some LLDB-specific configurations you'll want to pass? (You could just use the generic ${CROSS_TOOLCHAIN_FLAGS_${target_name}} for project-specific configs as well; it's slightly ugly, but as long as the configs don't cause issues in other projects it should be fine.)

lanza marked 2 inline comments as done.Jul 17 2019, 4:35 PM
smeenai accepted this revision.Jul 18 2019, 3:18 PM

LGTM

This revision is now accepted and ready to land.Jul 18 2019, 3:18 PM
lanza closed this revision.Jul 18 2019, 5:11 PM