CMAKE_BUILD_TYPE=Release fails

Forum rules
Please don't bump threads here if you have a problem - it will often be forgotten about if you do. Instead, make a new thread here.

Post a reply

Smilies
:D :) :( :o :shock: :? 8-) :lol: :x :P :oops: :cry: :evil: :twisted: :roll: :wink: :geek: :ugeek: :!: :?: :idea: :arrow: :| :mrgreen: :3: :wub: >:( :blergh:
View more smilies

BBCode is OFF
Smilies are ON

Topic review
   

Expand view Topic review: CMAKE_BUILD_TYPE=Release fails

Re: CMAKE_BUILD_TYPE=Release fails

by Chris » Fri Jun 23, 2017 4:32 am

Graf Zahl wrote: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>

Re: CMAKE_BUILD_TYPE=Release fails

by Graf Zahl » Thu Jun 22, 2017 8:24 am

kevans91 wrote: - 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.

Re: CMAKE_BUILD_TYPE=Release fails

by kevans91 » Thu Jun 22, 2017 7:52 am

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.

Re: CMAKE_BUILD_TYPE=Release fails

by _mental_ » Thu Jun 22, 2017 1:40 am

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.

Re: CMAKE_BUILD_TYPE=Release fails

by _mental_ » Thu Jun 22, 2017 12:52 am

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.

Re: CMAKE_BUILD_TYPE=Release fails

by Rachael » Thu Jun 22, 2017 12:24 am

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?

Re: CMAKE_BUILD_TYPE=Release fails

by Graf Zahl » Thu Jun 22, 2017 12:13 am

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.

Re: CMAKE_BUILD_TYPE=Release fails

by _mental_ » Wed Jun 21, 2017 11:01 pm

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

Re: CMAKE_BUILD_TYPE=Release fails

by kevans91 » Wed Jun 21, 2017 6:51 pm

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

Re: CMAKE_BUILD_TYPE=Release fails

by Rachael » Wed Jun 21, 2017 4:41 pm

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.

CMAKE_BUILD_TYPE=Release fails

by kevans91 » Wed Jun 21, 2017 3:44 pm

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

Top