Page MenuHomePhabricator

[CMake] Support generating Config.h
ClosedPublic

Authored by beanz on Apr 11 2017, 6:18 PM.

Details

Summary

This patch removes the hand maintained config files in favor of auto-generating the config file. We will still need to maintain the defines for the Xcode builds on Mac, but all CMake builds use the generated header instead.

This will enable finer grained platform support tests and enable supporting LLDB on more platforms with less manual maintenance.

I have only tested this patch on Darwin, and any help testing it out on other platforms would be greatly appreciated. I've probably messed something up somewhere.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz created this revision.Apr 11 2017, 6:18 PM
krytarowski added inline comments.
cmake/modules/LLDBConfig.cmake
433 ↗(On Diff #94923)

Can we just use #ifdef F_GETPATH in the source code? No need to define another symbol wrapping F_GETPATH.

While there there are candidates like pipe2(2) in source/Host/posix/PipePosix.cpp.

beanz updated this revision to Diff 94924.Apr 11 2017, 6:35 PM
  • Fixing installation to make sure CMake always installs the generated Config.h
  • Removing LLDB_CONFIG_FCNTL_GETPATH_SUPPORTED becasue we really don't need it
beanz updated this revision to Diff 94925.Apr 11 2017, 6:36 PM

Actually uploading the updates this time...

beanz updated this revision to Diff 94928.Apr 11 2017, 6:47 PM

Updating the hard coded Config.h

zturner edited edge metadata.Apr 11 2017, 7:01 PM

How much would it complicate things to move the hand maintained files out of tree? If the Xcode build isn't really a thing we're officially supporting, perhaps we can take that aggressive approach in-tree as well?

cmake/modules/LLDBConfig.cmake
287–293 ↗(On Diff #94928)

Is this necessary?

include/lldb/Host/Config.h.cmake
10–11 ↗(On Diff #94928)

Since we're in here anyway, can you use a fully qualified include guard here? Like #ifndef LLDB_HOST_CONFIG_H?

labath accepted this revision.Apr 12 2017, 3:38 AM

Thank you very much. I am soo happy that these files are going away.

How much would it complicate things to move the hand maintained files out of tree? If the Xcode build isn't really a thing we're officially supporting, perhaps we can take that aggressive approach in-tree as well?

AFAIK, the xcode project already passes some additional -D flags to the compiler when building lldb. It shouldn't be too hard (for someone who knows how to edit xcode projects :) ) to add these two flags there and delete the hand-written Config.h. I don't feel too strongly about that though.

This revision is now accepted and ready to land.Apr 12 2017, 3:38 AM
beanz added a comment.Apr 12 2017, 1:20 PM

My intention in this patch is not in any way to adversely impact the Xcode project, which is the supported and documented way to build LLDB on OS X (http://lldb.llvm.org/build.html#BuildingLldbOnMacOSX).

The goal of this patch is to support configuration time capabilities inspection to aid in porting LLDB to other platforms via the CMake build system. By leaving the Host/Config.h header in LLDB's source tree we continue supporting the Xcode project, and we don't require changing the LLDB sources which include lldb/Host/Config.h. This seems like the best solution to me.

cmake/modules/LLDBConfig.cmake
287–293 ↗(On Diff #94928)

CMake's documentation does not specify the order in which directories may be processed, and since the source and build directories will have an overlapping Config.h file we need to make sure the right one gets picked every time regardless of any changes to how CMake might handle the underlying implementation of the install command.

beanz updated this revision to Diff 95024.Apr 12 2017, 1:24 PM

Fixing up the include guard as per feedback from zturner, and fixing up an install logic error that I spoted.

This revision was automatically updated to reflect the committed changes.