This is an archive of the discontinued LLVM Phabricator instance.

Cleanup some problems with LLVM_ENABLE_DUMP in release builds.
ClosedPublic

Authored by hintonda on Sep 26 2017, 11:33 PM.

Details

Summary

Cleanup some problems with LLVM_ENABLE_DUMP in release builds

Removed a few unneeded tests for NDEBUG and replace others with
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP).

One issue involved ifdef'ing away a char* field from a struct, and changing
tablegen to handle it correctly, which might not be the best way to handle it.

Event Timeline

hintonda created this revision.Sep 26 2017, 11:33 PM
hintonda updated this revision to Diff 116831.Sep 27 2017, 9:22 AM
  • Set LLVM_ENABLE_DUMP in config.h instead of passing it on the command line.
MatzeB edited edge metadata.

Thanks for working on it. Looks fine to me, but I think the cmake logic needs adjustment.

CMakeLists.txt
399–402

This is fragile, as for any subsequent cmake runs the value for LLVM_ENABLE_DUMP is already set in the cmake cache and won't change even if the user modifies LLVM_ENABLE_ASSERTIONS.

We rather want cmake code that sets the define if either this option or LLVM_ENABLE_ASSERTIONS is enabled.

lib/CodeGen/ScoreboardHazardRecognizer.cpp
35

Is this to silence a warning about that member being unused? Maybe we better put an #ifdef around the member then?

hintonda added inline comments.Sep 27 2017, 11:06 AM
CMakeLists.txt
399–402

So, are you saying it should always be set for +Asserts builds?

The email thread seemed to indicate they should vary independently.

lib/CodeGen/ScoreboardHazardRecognizer.cpp
35

Yes, and I considered that, but you'd also have to #ifdef away the ctor initializer which seems really ugly to me. Please let me know which you prefer.

MatzeB added inline comments.Sep 27 2017, 11:11 AM
CMakeLists.txt
399–402

Yes.
I cannot think of any reason why you would not want the dump functions in an asserts build. Also currently we use a !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) in the cpp files so we have to enable the dumpers in assert builds of llvm. Admittedly after this whole thread I wonder if we shouldn't simplify this to just #ifdef LLVM_ENABLE_DUMP but we better do that another time in a separate patch.

lib/CodeGen/ScoreboardHazardRecognizer.cpp
35

It's fine like this.

hintonda updated this revision to Diff 116860.Sep 27 2017, 12:22 PM
  • Always set LLVM_ENABLE_DUMP=ON for +Asserts builds.
MatzeB accepted this revision.Sep 27 2017, 12:53 PM

Thanks, LGTM

This revision is now accepted and ready to land.Sep 27 2017, 12:53 PM
This revision was automatically updated to reflect the committed changes.

Thanks for fixing this :)

CMakeLists.txt
399–402

+1:

The code relate to dump should only be guarded by #ifdef LLVM_ENABLE_DUMP , and this should be on by default in Debug build, and then it is flexible enough and everyone is happy :)

Ideally, I think all the headers would hide the method behind #ifdef LLVM_ENABLE_DUMP , so that it wouldn't be a linker failure but a compile time failure!

Ideally, I think all the headers would hide the method behind #ifdef LLVM_ENABLE_DUMP , so that it wouldn't be a linker failure but a compile time failure!

Yes. I think this wasn't done in the past because we did not want to have defined(NDEBUG) checks in the public headers because users could obviously set this flag different from what we had when LLVM was building. But as soon as we push that logic into a LLVM_ENABLE_DUMP flag and make it not depend on NDEBUG anymore we can go back to hiding the definitions in the headers as users should get the correct setting via LLVMs config.h.

hintonda added inline comments.Sep 27 2017, 5:18 PM
llvm/trunk/CMakeLists.txt
400 ↗(On Diff #116884)

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.

MatzeB added inline comments.Sep 27 2017, 5:57 PM
llvm/trunk/CMakeLists.txt
400 ↗(On Diff #116884)

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...

MatzeB added inline comments.Sep 27 2017, 5:59 PM
llvm/trunk/CMakeLists.txt
400 ↗(On Diff #116884)

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.

hintonda added inline comments.Sep 27 2017, 7:23 PM
llvm/trunk/CMakeLists.txt
400 ↗(On Diff #116884)

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.

400 ↗(On Diff #116884)

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.

MatzeB added inline comments.Sep 28 2017, 10:13 AM
llvm/trunk/CMakeLists.txt
400 ↗(On Diff #116884)

Sure, any good name suggestions? I'll throw LLVM_FORCE_ENABLE_DUMP into the bikeshed discussion.

mehdi_amini added inline comments.Sep 28 2017, 10:23 AM
llvm/trunk/CMakeLists.txt
400 ↗(On Diff #116884)

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?

MatzeB added inline comments.Sep 28 2017, 10:27 AM
llvm/trunk/CMakeLists.txt
400 ↗(On Diff #116884)

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.

MatzeB added inline comments.Sep 28 2017, 10:29 AM
llvm/trunk/CMakeLists.txt
400 ↗(On Diff #116884)

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.

hintonda added inline comments.Sep 28 2017, 10:36 AM
llvm/trunk/CMakeLists.txt
400 ↗(On Diff #116884)

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.

mehdi_amini added inline comments.Sep 28 2017, 10:54 AM
llvm/trunk/CMakeLists.txt
400 ↗(On Diff #116884)

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!

MatzeB added inline comments.Sep 28 2017, 10:57 AM
llvm/trunk/CMakeLists.txt
400 ↗(On Diff #116884)

See also: https://reviews.llvm.org/D11833 and https://reviews.llvm.org/D11834

Would be good to push this topic again.

hintonda added inline comments.Sep 28 2017, 11:11 AM
llvm/trunk/CMakeLists.txt
400 ↗(On Diff #116884)

Thanks for the pointer. Yes, I think that's the best solution I've seen.