Make LLVM_ENABLE_DUMP independent LLVM_ENABLE_ASSERTIONS, move it to llvm-config.h, and update description.
Details
Details
- Reviewers
MatzeB mehdi_amini thakis chandlerc ahatanak whitequark bogner - Commits
- rG25e64a1b1541: [dump] Make LLVM_ENABLE_DUMP independent, and move to llvm-config.h
rG3e0199f7ebab: [dump] Remove NDEBUG from test to enable dump methods [NFC]
rL320111: [dump] Make LLVM_ENABLE_DUMP independent, and move to llvm-config.h
rL315590: [dump] Remove NDEBUG from test to enable dump methods [NFC]
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
While #ifdef LLVM_ENABLE_DUMP is arguably better than #ifndef NDEBUG, it's still problematic if we extend this to declarations in headers. Some headers that declare void dump() const, don't include any headers, so they won't #ifdef away the declaration which would have helped catch the problem at compile-time.
To fix this, we could test for a value instead of just definition, i.e., #if LLVM_ENABLE_DUMP, or even #if LLVM_ENABLE_DUMP > [some value].
That way, we are guaranteed to get a compile-time error.
Comment Actions
I like this change.
To put it differently: Your concern is that someone uses #ifdef LLVM_ENABLE_DUMP but forgets the #include "llvm/Config/llvm-config.h"?
To fix this, we could test for a value instead of just definition, i.e., #if LLVM_ENABLE_DUMP, or even #if LLVM_ENABLE_DUMP > [some value].
I'm not convinced that helps all that much (not that it is a problem either though).
You would expect to see compile problems when implementing the dump() function if llvm-config.h wasn't present in the header. It's also customary to include "foo.h" as first thing in "foo.cpp" so we have at least 1 file where we only have a no other headers included before "foo.h".
So I'd lean towards staying with #ifdef LLVM_ENABLE_DUMP as that is a more common idiom which avoids the other failure case where someone does #ifdef XXX working anyway even though XXX was defined to 0.
(No strong opinion from my side though so if others disagree I can be convinced to use this style).
Comment Actions
Yes, but as you mentioned below, it would be pretty rare. The only reason I brought it up would be to reduce churn if we did decide to go that route.
To fix this, we could test for a value instead of just definition, i.e., #if LLVM_ENABLE_DUMP, or even #if LLVM_ENABLE_DUMP > [some value].
I'm not convinced that helps all that much (not that it is a problem either though).
You would expect to see compile problems when implementing the dump() function if llvm-config.h wasn't present in the header. It's also customary to include "foo.h" as first thing in "foo.cpp" so we have at least 1 file where we only have a no other headers included before "foo.h".
So I'd lean towards staying with #ifdef LLVM_ENABLE_DUMP as that is a more common idiom which avoids the other failure case where someone does #ifdef XXX working anyway even though XXX was defined to 0.(No strong opinion from my side though so if others disagree I can be convinced to use this style).
Comment Actions
Re-opening:
This was reverted in rL315854 due to bot failures using multi-configuration generators, e.g., XCode and MSVC.
There were 2 issues:
- LLVM_ENABLE_DEBUG was missing on some dump declartions,
- LLVM_ENABLE_DEBUG was set based on CMAKE_BUILD_TYPE which isn't set when using multi-configuration generators.
We can fix 1 as needed, and 2 by adding LLVM_ENABLE_DUMP to CMAKE_CXX_FLAGS_(RELEASE|MINSIZEREL) as needed.
Comment Actions
Just some brainstorming for the generator problems (didn't really check if cmake is flexible enough to pull this off): Would it be possible to do cmake hackery to create a directory for every CMAKE_BUILD_TYPE and then generate the llvm-config.h apropriate for each BUILT_TYPE into each directory and add an -I flag to the flags of each build type.
Comment Actions
Pass LLVM_ENABLE_DUMP to appropriate CMAKE_CXX_FLAGS_<config>
variables instead of hard coding in config.h or llvm-config.h.
Comment Actions
Config directories are already created, e.g.: Release, Debug, etc., with bin and lib subdirs in each. We could easily add an include subdir to each, but then it gets complicated with adding -I as you mentioned.
But since the CMAKE_CXX_FLAGS_<config> variable already handles this, and also works for the case where headers don't include any other headers, I think it's a more general solution -- this was brought up by Justin Bogner earlier this evening, e.g.: MC/MCSchedule.h.
| CMakeLists.txt | ||
|---|---|---|
| 390–393 ↗ | (On Diff #125462) | Dynamically switching defaults in cmake is usually a bad idea. As that will only have an effect the first time cmake runs, AFAIK in subsequent cmake runs changing the default has no effect anymore as there is already a value in the cmake cache which will be unexpected by the user. (i.e. cmake -DCMAKE_BUILD_TYPE=DEBUG ; cmake -DCMAKE_BUILD_TYPE=RELEASE will give you a different value for LLVM_ENABLE_DUMP than cmake -DCMAKE_BUILD_TYPE=RELEASE assuming an empty build dir) |
| CMakeLists.txt | ||
|---|---|---|
| 390–393 ↗ | (On Diff #125462) | (Yes we already screwed this up with ENABLE_ASSERTIONS IMO, but no reason to screw it up with ENABLE_DUMP too IMO) |
Comment Actions
- Allow rerunning cmake with different values for build type or LLVM_ENABLE_ASSERTIONS.
| CMakeLists.txt | ||
|---|---|---|
| 390–393 ↗ | (On Diff #125462) | This now works for your example. If we want to be able to pass LLVM_ENABLE_DUMP independently, we can do that with the additional LLVM_FORCE_ENABLE_DUMP, but it should still work that same as ASSERTIONS. |
Comment Actions
Change LLVM_ENABLE_ASSERTIONS and LLVM_ENABLE_DUMP to non-cached
variables so we can determine if they were passed on the command line
and set defaults if not defined.
Only add/clear -DLLVM_ENABLE_DUMP in config specific
CMAKE_CXX_FLAGS_<config> variable corresponding to the current
CMAKE_BUILD_TYPE.
This configuration allows users to re-run cmake with different
CMAKE_BUILD_TYPE, LLVM_ENABLE_ASSERTIONS, or LLVM_ENABLE_DUMP and
update the appropriate CMAKE_CXX_FLAGS_<config> variable.
Comment Actions
When variables are passed on the command line, they are automatically cached
so that subsequent cmake runs will give identical results, i.e., as if the variables
were passed again. However, this makes it somewhat difficult to toggle options
since they look exactly the same and you don't know if the variable came from
the command line or the cache.
The current version of this patch doesn't handle this cleanly, but below is a simple
example of how to toggle variables (FOO in this case) on a per config basis.
Once set, the variable stays set until explicitly cleared.
I believe this technique could be used for both LLVM_ENABLE_DUMP
and LLVM_ENABLE_ASSERTIONS. Please let me know what you think.
local:/Users/dhinton/cmt/test $ cat ../CMakeLists.txt
cmake_minimum_required(VERSION 3.5)
string(TOUPPER ${CMAKE_BUILD_TYPE} uppercase_CMAKE_BUILD_TYPE)
# Do not cache FOO -- only allow it to be passed from the command line
# -- but set and cache the appropriate build type version instead.
if (DEFINED FOO)
foreach(lang C CXX)
if (FOO AND NOT FOO_${uppercase_CMAKE_BUILD_TYPE})
set(CMAKE_${lang}_FLAGS_${uppercase_CMAKE_BUILD_TYPE}
"${CMAKE_${lang}_FLAGS_${uppercase_CMAKE_BUILD_TYPE}} -DFOO")
elseif (NOT ${FOO})
string(REGEX REPLACE " *-DFOO *" " "
CMAKE_${lang}_FLAGS_${uppercase_CMAKE_BUILD_TYPE}
"${CMAKE_${lang}_FLAGS_${uppercase_CMAKE_BUILD_TYPE}}")
endif()
string(STRIP "${CMAKE_${lang}_FLAGS_${uppercase_CMAKE_BUILD_TYPE}}"
CMAKE_${lang}_FLAGS_${uppercase_CMAKE_BUILD_TYPE})
set(CMAKE_${lang}_FLAGS_${uppercase_CMAKE_BUILD_TYPE}
"${CMAKE_${lang}_FLAGS_${uppercase_CMAKE_BUILD_TYPE}}" CACHE STRING "" FORCE)
endforeach()
set(FOO_${uppercase_CMAKE_BUILD_TYPE} ${FOO} CACHE BOOL "" FORCE)
unset(FOO CACHE)
endif()
local:/Users/dhinton/cmt/test $ rm -rf *
local:/Users/dhinton/cmt/test $ cmake ../ -DCMAKE_BUILD_TYPE=Debug > /dev/null 2>&1 && grep -E "_CXX_FLAGS_(DEBUG|RELEASE):" CMakeCache.txt
CMAKE_CXX_FLAGS_DEBUG:STRING=-g
CMAKE_CXX_FLAGS_RELEASE:STRING=-O3 -DNDEBUG
local:/Users/dhinton/cmt/test $ cmake ../ -DCMAKE_BUILD_TYPE=Release > /dev/null 2>&1 && grep -E "_CXX_FLAGS_(DEBUG|RELEASE):" CMakeCache.txt
CMAKE_CXX_FLAGS_DEBUG:STRING=-g
CMAKE_CXX_FLAGS_RELEASE:STRING=-O3 -DNDEBUG
local:/Users/dhinton/cmt/test $ cmake ../ -DCMAKE_BUILD_TYPE=Release -DFOO=On > /dev/null 2>&1 && grep -E "_CXX_FLAGS_(DEBUG|RELEASE):" CMakeCache.txt
CMAKE_CXX_FLAGS_DEBUG:STRING=-g
CMAKE_CXX_FLAGS_RELEASE:STRING=-O3 -DNDEBUG -DFOO
local:/Users/dhinton/cmt/test $ cmake ../ -DCMAKE_BUILD_TYPE=Debug > /dev/null 2>&1 && grep -E "_CXX_FLAGS_(DEBUG|RELEASE):" CMakeCache.txt
CMAKE_CXX_FLAGS_DEBUG:STRING=-g
CMAKE_CXX_FLAGS_RELEASE:STRING=-O3 -DNDEBUG -DFOO
local:/Users/dhinton/cmt/test $ cmake ../ -DCMAKE_BUILD_TYPE=Debug -DFOO=Off > /dev/null 2>&1 && grep -E "_CXX_FLAGS_(DEBUG|RELEASE):" CMakeCache.txt
CMAKE_CXX_FLAGS_DEBUG:STRING=-g
CMAKE_CXX_FLAGS_RELEASE:STRING=-O3 -DNDEBUG -DFOO
local:/Users/dhinton/cmt/test $ cmake ../ -DCMAKE_BUILD_TYPE=Release > /dev/null 2>&1 && grep -E "_CXX_FLAGS_(DEBUG|RELEASE):" CMakeCache.txt
CMAKE_CXX_FLAGS_DEBUG:STRING=-g
CMAKE_CXX_FLAGS_RELEASE:STRING=-O3 -DNDEBUG -DFOO
local:/Users/dhinton/cmt/test $ cmake ../ -DCMAKE_BUILD_TYPE=Release -DFOO=Off > /dev/null 2>&1 && grep -E "_CXX_FLAGS_(DEBUG|RELEASE):" CMakeCache.txt
CMAKE_CXX_FLAGS_DEBUG:STRING=-g
CMAKE_CXX_FLAGS_RELEASE:STRING=-O3 -DNDEBUGComment Actions
I don't really think we want to be messing with the CMAKE_CXX_FLAGS_{BUILD_TYPE} variables for this. All of this regex-replace stuff is very complicated and seems fragile.
A couple of things need to happen:
- LLVM_ENABLE_DUMP definitely needs to be defined in llvm-config.h, or we won't be able to safely use it in headers. Headers that use it should include llvm-config.h
- While we probably don't want to set a cache variable's value based on the value of assertions, if the user explicitly specifies it we probably do want to trust them. I think having LLVM_ENABLE_DUMP be computed / non-cached and basing it off of LLVM_ENABLE_ASSERTIONS and LLVM_FORCE_ENABLE_DUMP is probably the clearest thing to do here, though that does hit the problem that people already having LLVM_ENABLE_DUMP in their caches. Maybe it would work to set the non-cached version based off of asserts iff the cached version is unset?
Comment Actions
If this would work for XCode and MSVC generators -- the MSVC generator breakage Aaron reported was the main reason this was originally rolled back -- then I'd be all for it.
My solution would support buildbots, developer builds, and installed versions via llvm-config. What you've proposed would only support ninja/make and installed versions. Btw, I only use ninja, so I'd love to do what you propose -- why would anyone use anything else, right? ;-)
However, I don't see a way to solve the multi-config issue without either using config specific variables, e.g., CMAKE_CXX_FLAGS_RELEASE (the solution I"m proposing), or config specific lvm-config.h files, e.g., builddir/Release/include/llvm-config.h and passing -I in dev builds -- installed versions would just install the correct llvm-config.h (which is what Mathias was alluding to).
Of course, I might be missing something, so if you've got a solution that will solve all of these problems, I'm all in. I just don't see how hard coding it in a single llvm-config.h could work.
Comment Actions
I don't understand how this version supports installed versions, the define doesn't appear in llvm-config at all here. Can you explain?
Comment Actions
While I could see an llvm-config solution working, I'd really rather see us try the llvm-config.h solution first. In my experience people who do not come from a unix/linux background usually setup their own projects in various IDEs and do not profit from an llvm-config tool feeding them the right flags, they would have to discover the right flags themselfes if they aren't in llvm-config.h.
Comment Actions
I'm also confused about how this works before this change, if multi-config generators only create one set of headers. Currently, we're already defining LLVM_ENABLE_DUMP in config.h, so how does MSVC handle the fact that this already has different values for different configs?
Comment Actions
I was planning on using llvm-config, e.g.:
local:/Users/dhinton/projects/llvm_project/build/Debug $ bin/llvm-config --cxxflags -I/Users/dhinton/projects/llvm_project/llvm/include -I/Users/dhinton/projects/llvm_project/build/Debug/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fcolor-diagnostics -O3 -DLLVM_ENABLE_DUMP -fno-exceptions -fno-rtti -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
Comment Actions
Btw, this was configured with cmake -GNinja ../../llvm -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=On against the current patch.
Comment Actions
That's the problem, as far as I can tell, it never worked. It always depended on NDEBUG.
Comment Actions
Ah, that llvm-config. Sorry, I got confused about that and llvm-config.h.
This sort of works, but I don't think it's optimal. Most users of llvm as a library I'm aware of don't use llvm-config --cxxflags (though they often do use other llvm-config flags). This command lists a number of flags that you don't necessarily want when building against llvm (like the specific warning settings).
Comment Actions
I can do this, but I got a lot of push back from multiple people at the dev meeting when I floated this idea, so I didn't pursue it. The complaint seemed to be multiple files and -I directives would be too confusing. Btw, we'll have to munge the command line variables based on the config. I'll give it a try and get back to you.
Comment Actions
While it's complicated/annoying that we have to do this, so far every other solution seems worse to me. So I'd definitely support the multiple llvm-config.h in different directories approach.
Comment Actions
Make LLVM_ENABLE_DUMP independent of LLVM_ENABLE_ASSERTIONS, and move
the LLVM_ENABLE_DUMP define from config.h to llvm-config.h.
This patch was originally motivated by a desire to better control
LLVM_ENABLE_DUMP defaults based on the current build type -- much like
LLVM_ENABLE_ASSERTIONS handles it -- as well as clean up the locations
in the code. Unfortunately, many locations only tested for NDEBUG, so
the cleanup caused a problem.
Also, the complexities of handling multiple build types, especially
for multi-config generators like Xcode and MSVC, made it really
difficult to effectively control LLVM_ENABLE_DUMP at that granularity.
The only way to do this would involve adding a command line option
(via either a -D or -I options), but that defeats the purpose of
moving LLVM_ENABLE_DUMP to a header file in the first place -- it was
originally set via a cmake add_definitions directive.
Comment Actions
(via either a -D or -I options), but that defeats the purpose of moving LLVM_ENABLE_DUMP to a header file in the first place -- it was originally set via a cmake add_definitions directive.
It doesn't really defeat that purpose: From the point of view of an external llvm user no flags need to be set at all! (Assuming someone prepared a nice package where llvm-config.h just lives next to all the other headers). The complexity of creating multiple llvm-config.h files in different directories and adding different -I to have each config include their specific directory is contained within llvms build system.
Comment Actions
Though admittedly I like the solution here for its simplicity. However we should change the default to ON. The only downside is obviously external packagers need to be aware of the flag now to take advantage in their releases but I personally can live with that given that this is only about a few kilobytes saved in release libraries I believe.
Comment Actions
Let's wait a bit to see if this is fine with people. It would probably be also nice to add the set(LLVM_ENABLE_DUMP OFF CACHE BOOL "") line to the varous caches in clang/cmake/caches that build llvm in release build types.
| CMakeLists.txt | ||
|---|---|---|
| 388 ↗ | (On Diff #125843) | Should also change the description to "Enable dump functions" (without the "in release builds") now. |
Comment Actions
What I meant was that by putting it in a header, means it could be used without additional user input.
Comment Actions
I don't mind changing the default, but the original default was OFF, and only flipped to ON if LLVM_ENABLE_ASSERTIONS was ON. But that had no affect, since enabling assertions removed -DNDEBUG and added -UNDEBUG, so all the dump definitions were already enabled. Setting LLVM_ENABLE_DUMP=ON only makes sense for non-debug builds where LLVM_ENABLE_ASSERTIONS=OFF.
So, I think maintaining the default of OFF is the correct choice, but will set it to whatever you want.
Comment Actions
Wait, I'm getting confused with all those discussions going in circles. I assumed we would also change the code inside llvm to simply test for LLVM_ENABLE_DUMP and not for !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP). But apparently we do not do that, in that case yes we can leave the default to "OFF" and the description to be "Enable dump functions even when assertions are disabled".
Comment Actions
At this point this is strictly better. Let's make this change and figure out where to go from here in tree.
| CMakeLists.txt | ||
|---|---|---|
| 396–398 ↗ | (On Diff #125854) | Since everything checks both currently it doesn't actually have an effect, but I think we should leave this in place for now. It more accurately reflects our intent. |
| include/llvm/Config/llvm-config.h.cmake | ||
|---|---|---|
| 17 ↗ | (On Diff #125854) | Maybe this should say "provide dump methods, even in release builds". Most of the flags in this file are written in the "what does it do" style rather than the "how does it do it" style. |
| CMakeLists.txt | ||
|---|---|---|
| 396–398 ↗ | (On Diff #125854) | I added this in D38306 based on the assumption that we could remove the NDEBUG test and just use the LLVM_ENABLE_DUMP test for dump definitions -- and other related functions. However, that turned out to be a bad idea. So I think this should removed. Especially since it leaves the reader with the impression that you need to do it, which is false. The only good thing about it is that it doesn't update the cache, so it's wrong but innocuous. |
| CMakeLists.txt | ||
|---|---|---|
| 396–398 ↗ | (On Diff #125854) | Okay, that makes sense. |