CMAKE_BUILD_TYPE=Release fails

[kevans91]

As of https://github.com/coelckers/gzdoom/com ... 618fda2551, a dependency on _DEBUG/AssertObject has grown that is not satisfied when CMAKE_BUILD_TYPE=Release, produced first from g3.1.0 then again with a6b7ce0. Relevant PR [1] submitted to add an AssertObject macro to effectively no-op calls to it if !_DEBUG.

[1] https://github.com/coelckers/gzdoom/pull/349

[Rachael]

What compiler are you using that this is actually needed?

I've used fairly updated versions of MSVC++, GCC, and Clang, and none of them have needed this.

Granted, this has no bearing on the acceptance of the PR, I am just wondering what kind of a setup actually needed this.

[kevans91]

LLVM/Clang 4.0.0 here; haven't tried anything else.

[_mental_]

Travis CI and macOS devbuilds disagree with you too. Are you sure that you don't have local changes?

[Graf Zahl]

This can only happen if for some reason 'assert' is not being defined to nothing. The AssertObject call is inside an assert and every sane compiler will weed that out so that the AssertObject will never be in the preprocessed code.

It seems that for some reason not all preprocessor macros are generated correctly on the affected system for "CMAKE_BUILD_TYPE=Release" and this needs to be investigated. The proposed fix only hides the real problem.

[Rachael]

In that case, _mental_'s question still applies (and is much more important now) - along, also, with how you've obtained Clang 4.0.0. Did you get it in a distro package, did you compile it yourself, etc?

What operating system (and variant, if Linux/Unix) are you using?

[_mental_]

Probably I know what's wrong with it. The assert() macro is disabled when NDEBUG is defined but not when _DEBUG is not defined.
So #ifdef _DEBUG should be replaced with #ifndef NDEBUG in a few related locations. I'm checking this now.

[_mental_]

Should be fixed in 279b1e2.

The only thing I can imagine that can cause such error (except broken Clang installation of course) is missing definition of NDEBUG in your Release build.

[kevans91]

Hello! I meant to follow-up and check it out more closely, but ran into time issues. To answer some questions:

- No local changes
- Running FreeBSD
- NDEBUG is successfully getting defined, as indicated by an #error yes in vmexec.cpp where this is checked
- Only NDEBUG is getting twiddled in vmexec.cpp. I am curious as to why MSVC/glibc's <assert.h> isn't failing similarly since it doesn't check _DEBUG either, but it's got more preproc soup than our libc's version, so I wish not to think about it too much. =)

FWIW, this is our version of assert.h: https://svnweb.freebsd.org/base/head/in ... iew=markup -- it's fairly straightforward. Thanks for the fix! Works beautifully.

[Graf Zahl]

- Only NDEBUG is getting twiddled in vmexec.cpp. I am curious as to why MSVC/glibc's <assert.h> isn't failing similarly since it doesn't check _DEBUG either, but it's got more preproc soup than our libc's version, so I wish not to think about it too much. =)

It's probably because for those platforms _DEBUG is defined automatically. To be honest, I think it's stupid to #define non-debugging state. It should have been the other way from the start.

[Chris]

To be honest, I think it's stupid to #define non-debugging state. It should have been the other way from the start.

From what I've heard, some system developers don't even like using NDEBUG. On Debian, for example, they explicitly remove NDEBUG from Release builds of packages specifically so asserts stay.

What bugs me most about the assert macro is that the preprocessor removes the code. If you want to disable the checks with NDEBUG, that's fine by me, but let the compiler see it and optimize it out. Like

Code: Select all

#ifndef NDEBUG
#define assert(x) do { if(!(x)) __assert_fail(#x); } while(0)
#else
#define assert(x) do { if(false && !(x)) __assert_fail(#x); } while(0)
#endif

Any sane compiler should optimize out the check in the latter case (C and C++ short-circuit conditionals; if the left side of && evaluates false, or the left side of || evaluates true, it will skip evaluating the right side since it can't affect the result, and consequently won't have any run-time side-effect). Not only would it handle issues like this, where references to classes or functions may or may not disappear in an assert, it would also handle cases where certain variables are only ever checked in an assert (assign values to a variable and check the value in an assert, but in Release builds the compiler complains that the variable is set but never used because the use was taken out before the compiler ever saw it).
</rant>