Page MenuHomePhabricator

s/NDEBUG/LLVM_NDEBUG/ in most places
Needs RevisionPublic

Authored by tstellarAMD on Aug 7 2015, 8:27 AM.

Details

Summary

The goal of this change is to remove the dependency on NDEBUG in
headers, so that library users can freely choose whether or not to
define this value when building their applications.

Diff Detail

Event Timeline

tstellarAMD retitled this revision from to s/NDEBUG/LLVM_NDEBUG/ in most places.
tstellarAMD updated this object.
tstellarAMD added a subscriber: llvm-commits.

+1 on Goal In particular the use of NDEBUG in external interface header files causes significant pain when using the header files in a build environment that uses NDEBUG

What steps are needed to move this change forward?

+1 on Goal In particular the use of NDEBUG in external interface header files causes significant pain when using the header files in a build environment that uses NDEBUG

What steps are needed to move this change forward?

Is this still a problem? Aren't all the header checks guarded by LLVM_ENABLE_ABI_BREAKING_CHECKS now?

Issue appears to still be present
For example: In llvm/include/llvm/Transforms/Scalar/LoopPassManager.h

https://llvm.org/doxygen/LoopPassManager_8h_source.html

line 207
...
#ifndef NDEBUG

for (Loop *NewL : NewChildLoops)
  assert(NewL->getParentLoop() == CurrentL && "All of the new loops must "
                                              "be immediate children of "
                                              "the current loop!");

#endif
...

Running the command: find . -iname '*.h' | xargs grep '^#' -- | grep -vE '^.*:.*(LLVM_|cplusplus|endif|else|include|define )' | grep NDEBUG | wc -l

returns
150 instances of NDEBUG in .h files the 9.0.0 code base.

Best,
--Roger

OK but these NDEBUG aren't necessarily harmful, are they?

IIRC we added LLVM_ENABLE_ABI_BREAKING_CHECKS to cover the cases because previous using NDEBUG in the client was *required* to match the settings at the time LLVM was built.

OK but these NDEBUG aren't necessarily harmful, are they?

IIRC we added LLVM_ENABLE_ABI_BREAKING_CHECKS to cover the cases because previous using NDEBUG in the client was *required* to match the settings at the time LLVM was built.

Agree with Mehdi, we should only be using LLVM_ENABLE_ABI_BREAKING_CHECKS when it's actually an ABI-breaking check.

arsenm removed a subscriber: arsenm.Nov 13 2019, 6:29 PM

s/NDEBUG/LLVM_NDEBUG/ will not apply to the use of NDEBUG in assert.h. Most uses of NDEBUG are hidden in the assert macro.

In the aforementioned use of NDEBUG:

#ifndef NDEBUG
for (Loop *NewL : NewChildLoops)
  assert(NewL->getParentLoop() == CurrentL && "All of the new loops must "
                                              "be immediate children of "
                                              "the current loop!");
#endif

it is used only to avoid the overhead of iteration when the assert macro expands to nothing.

Consequences:

  1. Even with LLVM_NDEBUG defined, plain assert's will still be evaluated.
  2. To have complete asserts build, LLVM_NDEBUG and NDEBUG must be defined at the same time.
  3. NDEBUG and assert are linked by the C/C++ language. To unlink, we must also introduce llvm_assert (or ban assert in headers).
dexonsmith requested changes to this revision.Nov 14 2019, 11:44 AM

Stepping back to look at the goals:

The goal of this change is to remove the dependency on NDEBUG in
headers, so that library users can freely choose whether or not to
define this value when building their applications.

This seems like the same goal that introducing LLVM_ENABLE_ABI_BREAKING_CHECKS had. Is this patch a change in direction for how to achieve the goal? Or is there a subtlety here, and it's achieving something additional that LLVM_ENABLE_ABI_BREAKING_CHECKS does not? I think it at least deserves an llvm-dev discussion before proceeding (I just took a look through the archives and don't see one, apologies if I missed it).

Note that there are some nice benefits to the existing LLVM_ENABLE_ABI_BREAKING_CHECKS model:

  • A developer that needs a stable ABI can configure LLVM_ENABLE_ABI_BREAKING_CHECKS (either on or off) to achieve that.
  • If not specified explicitly, LLVM_ENABLE_ABI_BREAKING_CHECKS is controlled by whether it's an "asserts" build. This gives the right behaviour for most developers most of the time.
  • The common case of assertions that are not ABI-breaking don't need special treatment.
    • Always controlled by whether it's an "asserts" build.
    • Uses the syntax that people expect.

I think the llvm-dev discussion would need to establish (a) why this isn't working (but the new thing would work) and/or (b) what additional benefits splitting off LLVM_NDEBUG and llvm_assert would solve. It's also not clear what's supposed to happen for other LLVM sub-projects, like Clang. Should they grow clang_assert or use llvm_assert? (BTW, it would probably be nice to also introduce an llvm_abi_breaking_assert or something to simplify the existing model, whether or not this change moves forward.)

I also think changes like this should be documented in the coding standards; I'm requesting changes on that basis.

This revision now requires changes to proceed.Nov 14 2019, 11:44 AM

Yes I agree the example case I provided is not an LLVM_ENABLE_ABI_BREAKING_CHECKS modification

I agree with dexonsmith on these points

  1. That LLVM_ENABLE_ABI_BREAKING_CHECKS addresses the issue
  1. The use LLVM_ENABLE_ABI_BREAKING_CHECKS should be better documented in the llvm-project coding standards

2a. And the use or non-use llvm_assert(),assert(), NDEBUG,.. should also be documented

  1. That we need to think through how this model should apply to subprojects: clang, lld, mlir,...

How can I help enable this discussion of 1 and 2?

Rhetorical question: What is the definition of when LLVM_ENABLE_ABI_BREAKING_CHECKS should be used ?

Candidate answer: Any change to the class/template/... that changes the size of class or template

Correct usage example from: llvm/include/llvm/Analysis/LoopInfo.h
...
template <class BlockT, class LoopT> class LoopBase {

LoopT *ParentLoop;
// Loops contained entirely within this one.
std::vector<LoopT *> SubLoops;

// The list of blocks in this loop. First entry is the header node.
std::vector<BlockT *> Blocks;

SmallPtrSet<const BlockT *, 8> DenseBlockSet;

#if LLVM_ENABLE_ABI_BREAKING_CHECKS

/// Indicator that this loop is no longer a valid loop.
bool IsInvalid = false;

#endif
...

header example that appears to need LLVM_ENABLE_ABI_BREAKING_CHECKS

File: llvm/include/llvm/CodeGen/LiveIntervalUnion.h

Line 101 in llvm-project 9.0.0
...
// Print union, using TRI to translate register names

void print(raw_ostream &OS, const TargetRegisterInfo *TRI) const;

#ifndef NDEBUG

// Verify the live intervals in this union and add them to the visited set.
void verify(LiveVirtRegBitSet& VisitedVRegs);

#endif
...

How do I confirm that the use of NDEBUG in a header file is correct?