Conditional symbol exports in header files.

[llvmbot]

Bugzilla Link	12759
Version	unspecified
OS	Linux
Reporter	LLVM Bugzilla Contributor
CC	@lattner,@DougGregor

Extended Description

Hello, I'm trying to build the Mesa project and there could be problems building with or without NDEBUG. The Mesa project uses this also and I don't know if it's use needs to be independent of llvm.

However I'm sure that the header file is the wrong place to be making changes in the ABI based on preprocessor variables. If this switch were instead moved into the code, then there is only a slight performance penalty calling an empty/noop function.

Currently in certain situations the Mesa project is built with symbols that are not part of the llvm library and this breaks compilation.

[llvmbot]

Some Example Fixes for correct use of NDEBUG. I've attempted to do a minimal fix of issues, however I've concluded that just about every instance of NDEBUG has something that prevented the use of an innocuous Debug.h.

Thus after correcting several of these I've nearly given up after looking at the lib/CodeGen/SelectionDAG folder. I won't have time to review all of these and reach my goal of testing the llvm r660g compiler for the release of Diablo III.

On May 15th my time will be spent playing, not writing drivers so I can play.

Thank you.

[llvmbot]

The part about assert() being a no-op when NDEBUG is set is not contagious and does not spread to other macros, like DEBUG. Using DEBUG_WITH_TYPE to indirectly give this effect, didn't always work.

[llvmbot]

Another truly awful aspect of this code.

Take this code when NDEBUG is set: if(::llvm::isCurrentDebugType("")) {};

// Perfectly valid! Until it's preprocessed... if(::llvm::(false)) {};

llvm/Support/Debug.h: 00072 #define isCurrentDebugType(X) (false) 00073 #define SetCurrentDebugType(X) 00074 #define DEBUG_WITH_TYPE(TYPE, X) do { } while (0)

[llvmbot]

There are other examples of this, this might be the last one. However this isn't the last place where NDEBUG is used in an include file and it's also not the only place where assert() is used in an include file.

LLVM needs to be hardened against multiple projects with there own independent value for NDEBUG.

Line 51: include/llvm/CodeGen/ScheduleDAGInstrs.h /// NewSUnit - Creates a new SUnit and return a ptr to it. /// SUnit NewSUnit(MachineInstr MI) {

ifndef NDEBUG

  const SUnit *Addr = SUnits.empty() ? 0 : &SUnits[0];

endif

  SUnits.push_back(SUnit(MI, (unsigned)SUnits.size()));
  assert((Addr == 0 || Addr == &SUnits[0]) &&
         "SUnits std::vector reallocated on the fly!");
  SUnits.back().OrigNode = &SUnits.back();
  return &SUnits.back();
}

[llvmbot]

At first I though a fix for the current issue was something like...

As a point of discussion: diff --git a/lib/VMCore/Constants.cpp b/lib/VMCore/Constants.cpp index aa66969..bee2ab1 100644 --- a/lib/VMCore/Constants.cpp +++ b/lib/VMCore/Constants.cpp @@ -11,6 +11,9 @@ // //===----------------------------------------------------------------------===//

+#ifdef LLVM_NDEBUG +#define NDEBUG LLVM_NDEBUG +#endif

include "llvm/Constants.h"

include "LLVMContextImpl.h"

include "ConstantFold.h"

-- 1.7.9.5

However now I relies that, although this is simple and it works, this is perhaps the worst solution ever.

The issue was created when some thing like this was typed:

ifndef NDEBUG

int ctr=0;

endif

assert(ctr);

Now ordinarily you'd swear up and down that this will never compile, but instead of screaming bloody murder it happily compiles.

A coincidence I say! This code is broken, through and through. The part about assert() being a no-op when NDEBUG is set should be used as a conveyance, not a coding construct to be made use of.

I'd like to ask that these are fixed properly, even if just a few every day or so.

[llvmbot]

opps, I might have moved on of the #endif up above #ifdef LLVM_NDEBUG / Let this overide REDEBUG /

It's a minor implementation detail, not a bug or anything.

[llvmbot]

Pass LLVM_NDEBUG to assert.h, but only for our project. This safely makes sure to honor the LLVM_NDEBUG and disable asserts. It also fixes a bug in one part of the build where under certain conditions assert.h must be passed NDEBUG.

[llvmbot]

A few issues with these patches, from d0k.

1. NDEBUG is needed to nullify assert and perhaps other things.
2. I shouldn't have changed lib/Support/reg*, they depend on NDEBUG being set.

[llvmbot]

This is odd. I took a source I've already had building and now it's getting what I see as unrelated errors.

https://launchpadlibrarian.net/104543561/buildlog_ubuntu-precise-i386.llvm-3.1_3.1~%2Brc2-1~cheako1~precise0_FAILEDTOBUILD.txt.gz

[llvmbot]

This changes things around a bit, I feel it's an improvement... If it works. It's kinda hard to describe what is going on in this patch. Basically the Debug.h header file was gaining extra symbols when NDEBUG wasn't set and this causes build failures in other projects when attempting to link with llvm that was compiled with this set.

This patch turns these API elements into macros, the idea is that every one of the four(two bools) combinations will now produce a working setup.

There is one thing I wish I could figure out, a -debug parameter should alter the value of DebugFlag. The preprocessor should ensure that DebugFlag is always (false) when NDEBUG is set, I.E. any writing to DebugFlag should be protected by #ifndef NDEBUG.

[llvmbot]

This replaces NDEBUG with LLVM_NDEBUG. This won't conflict with NDEBUG from any other project, hopefully. It also exposes this for other projects to use.

[llvmbot]

Perhaps, you can do both.

Put something like this in the header file, making sure these objects are always built:

/// DebugFlag - This boolean is set to true if the '-debug' command line optio /// is specified. This should probably not be referenced directly, instead, u /// the DEBUG macro below. /// extern bool _DebugFlag;

/// isCurrentDebugType - Return true if the specified string is the debug type /// specified on the command line, or if none was specified on the command lin /// with the -debug-only=X option. /// bool _isCurrentDebugType(const char *Type);

/// SetCurrentDebugType - Set the current debug type, as if the -debug-only=X /// option were specified. Note that DebugFlag also needs to be set to true f /// debug output to be produced. /// void _SetCurrentDebugType(const char *Type);

/// DEBUG_WITH_TYPE macro - This macro should be used by passes to emit debug /// information. In the '-debug' option is specified on the commandline, and /// this is a debug build, then the code specified as the option to the macro /// will be executed. Otherwise it will not be. Example: /// /// DEBUG_WITH_TYPE("bitset", dbgs() << "Bitset contains: " << Bitset << "\n") /// /// This will emit the debug information if -debug is present, and -debug-only /// is not specified, or is specified as "bitset".

define _DEBUG_WITH_TYPE(TYPE, X) \

do { if (::llvm::_DebugFlag && ::llvm::_isCurrentDebugType(TYPE)) { X; } \ } while (0)

ifndef LLVM_NDEBUG

define isCurrentDebugType(X) _isCurrentDebugType(X)

define SetCurrentDebugType(X) _SetCurrentDebugType(X)

define DEBUG_WITH_TYPE(TYPE, X) _DEBUG_WITH_TYPE(TYPE, X)

else

define isCurrentDebugType(X) (false)

define SetCurrentDebugType(X)

define DEBUG_WITH_TYPE(TYPE, X) do { } while (0)

endif

[llvmbot]

If you remove it from the header file, it's no longer an issue what you call it. If you just re-name it that would solve the current problems with Mesa, but I'd still wonder if it's a good coding practice.

I'm willing to just wait and see, as I'm not sold on any solution currently.

[DougGregor]

So you're asking us to replace our usage of NDEBUG with an LLVM-specific macro. That's reasonable; over to the appropriate Clang component.

[llvmbot]

I believe I was vary clear with my request.

Please remove the "#ifndef NDEBUG" from header files in an effort to not dynamically alter your ABI. This functionality can be implemented as part of the code where these functions are defined.

[DougGregor]

Please direct questions to the cfe-dev mailing list. The bug reporter is for specific bugs or feature requests only.