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.
Details
- Reviewers
echristo beanz dexonsmith
Diff Detail
Event Timeline
+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.
Agree with Mehdi, we should only be using LLVM_ENABLE_ABI_BREAKING_CHECKS when it's actually an ABI-breaking check.
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:
- Even with LLVM_NDEBUG defined, plain assert's will still be evaluated.
- To have complete asserts build, LLVM_NDEBUG and NDEBUG must be defined at the same time.
- NDEBUG and assert are linked by the C/C++ language. To unlink, we must also introduce llvm_assert (or ban assert in headers).
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.
Yes I agree the example case I provided is not an LLVM_ENABLE_ABI_BREAKING_CHECKS modification
I agree with dexonsmith on these points
- That LLVM_ENABLE_ABI_BREAKING_CHECKS addresses the issue
- 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
- 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?