[PATCH] D38306: Cleanup some problems with LLVM_ENABLE_DUMP in release builds.

[Don Hinton via Phabricator via llvm-commits]

hintonda added inline comments.


================
Comment at: llvm/trunk/CMakeLists.txt:400
+if( LLVM_ENABLE_ASSERTIONS )
+  set(LLVM_ENABLE_DUMP ON)
+endif()
----------------
MatzeB wrote:
> mehdi_amini wrote:
> > hintonda wrote:
> > > MatzeB wrote:
> > > > MatzeB wrote:
> > > > > mehdi_amini wrote:
> > > > > > MatzeB wrote:
> > > > > > > hintonda wrote:
> > > > > > > > hintonda wrote:
> > > > > > > > > MatzeB wrote:
> > > > > > > > > > MatzeB wrote:
> > > > > > > > > > > hintonda wrote:
> > > > > > > > > > > > Just noticed that this doesn't actually update the cache.  However, that doesn't matter in this case since the updated variable is what is actually used.  
> > > > > > > > > > > > 
> > > > > > > > > > > > I'll fix this shortly so it won't confuse anyone looking at the cache.
> > > > > > > > > > > Yep it does not update the cache and that is a good thing in this situation IMO... I'd rather not see cmake change setting B just because I switched setting A...
> > > > > > > > > > Or put another way: That's why I used the "Enable Dump function **in release builds**" description to indicate the flag has no effect in debug builds because the feature is enabled anyway.
> > > > > > > > > > Yep it does not update the cache and that is a good thing in this situation IMO... I'd rather not see cmake change setting B just because I switched setting A...
> > > > > > > > > 
> > > > > > > > > We can do this by adding an additional variable, i.e., cache the passed variable, but determine LLVM_ENABLE_DUMP on the fly -- using the same name for both can be confusing.
> > > > > > > > > Or put another way: That's why I used the "Enable Dump function in release builds" description to indicate the flag has no effect in debug builds because the feature is enabled anyway.
> > > > > > > > 
> > > > > > > > That's a good point.  Once we remove the test for !defined(NDEBUG), LLVM_ENABLE_DUMP will be used for both, so we'll need to change that.
> > > > > > > Sure, any good name suggestions? I'll throw LLVM_FORCE_ENABLE_DUMP into the bikeshed discussion.
> > > > > > Do we need this?
> > > > > > What about changing is in the `config.h`:
> > > > > > 
> > > > > > ```
> > > > > > /* Define if LLVM_ENABLE_DUMP is enabled or not DNDEBUG */
> > > > > > #cmakedefine LLVM_ENABLE_DUMP
> > > > > > #ifndef NDEBUG
> > > > > > #ifndef LLVM_ENABLE_DUMP
> > > > > > #define LLVM_ENABLE_DUMP
> > > > > > #endif
> > > > > > #endif
> > > > > > ```
> > > > > > 
> > > > > > Wouldn't this cover all the uses?
> > > > > > 
> > > > > This solution gets us back into the trouble of picking up the users NDEBUG setting in the header and not necessarily the one used when compiling llvm.
> > > > Then again this is hardly the first instance of this problem in LLVM and we could start fixing it by adding something like LLVM_ENABLE_ASSERTION and use that instead of NDEBUG in our headers.
> > > After doing a mass update of llvm, and discovering additional issues with NDEBUG, I think it would be better to replace all instances of NDEBUG with the appropriate ${PROJECT}_NDEBUG, and set ${PROJECT})_NDEBUG via cmake (in either config.h or llvm_config.h).
> > > 
> > > That will break the dependency on NDEBUG, which we don't really control, and allow us to set it on a project basis.
> > > 
> > > I'm working on a POC patch right now.
> > > This solution gets us back into the trouble of picking up the users NDEBUG setting in the header and not necessarily the one used when compiling llvm.
> > 
> > Ah good point!
> See also: https://reviews.llvm.org/D11833 and https://reviews.llvm.org/D11834
> 
> Would be good to push this topic again.
Thanks for the pointer.  Yes, I think that's the best solution I've seen.


Repository:
  rL LLVM

https://reviews.llvm.org/D38306