[libc++] Disable _LIBCPP_NODEBUG temporarily

[philnik777]

This should be reverted once the crash reported in #118710 has been
analyzed.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This should be reverted once the crash reported in #118710 has been
analyzed.

---

Full diff: https://github.com/llvm/llvm-project/pull/122393.diff

2 Files Affected:

• (modified) libcxx/include/__config (+3-1)
• (modified) libcxx/test/tools/clang_tidy_checks/libcpp_module.cpp (+1-1)

diff --git a/libcxx/include/__config b/libcxx/include/__config
index ace6e1cd73e3e0..2de4c009b5afde 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -1170,7 +1170,9 @@ typedef __char32_t char32_t;
 #    define _LIBCPP_NOESCAPE
 #  endif
 
-#  define _LIBCPP_NODEBUG [[__gnu__::__nodebug__]]
+// FIXME: Expand this to [[__gnu__::__nodebug__]] again once the testcase reported in
+// https://github.com/llvm/llvm-project/pull/118710 has been analyzed
+#  define _LIBCPP_NODEBUG
 
 #  if __has_attribute(__standalone_debug__)
 #    define _LIBCPP_STANDALONE_DEBUG __attribute__((__standalone_debug__))
diff --git a/libcxx/test/tools/clang_tidy_checks/libcpp_module.cpp b/libcxx/test/tools/clang_tidy_checks/libcpp_module.cpp
index bc7c8ce7ec443a..f49f3e3c615ca9 100644
--- a/libcxx/test/tools/clang_tidy_checks/libcpp_module.cpp
+++ b/libcxx/test/tools/clang_tidy_checks/libcpp_module.cpp
@@ -27,7 +27,7 @@ class LibcxxTestModule : public clang::tidy::ClangTidyModule {
     check_factories.registerCheck<libcpp::header_exportable_declarations>("libcpp-header-exportable-declarations");
     check_factories.registerCheck<libcpp::hide_from_abi>("libcpp-hide-from-abi");
     check_factories.registerCheck<libcpp::internal_ftm_use>("libcpp-internal-ftms");
-    check_factories.registerCheck<libcpp::nodebug_on_aliases>("libcpp-nodebug-on-aliases");
+    // check_factories.registerCheck<libcpp::nodebug_on_aliases>("libcpp-nodebug-on-aliases");
     check_factories.registerCheck<libcpp::proper_version_checks>("libcpp-cpp-version-check");
     check_factories.registerCheck<libcpp::robust_against_adl_check>("libcpp-robust-against-adl");
     check_factories.registerCheck<libcpp::uglify_attributes>("libcpp-uglify-attributes");

[Michael137]

FYI, I'm able to repro the Clang frontend crash by running the TestNamespaceLocalVarSameNameObjC.py LLDB test on my Intel machine (it curiously doesn't reproduce on my M1).

Might not be exactly the same issue as the LLVM crash but we'll have to address it either way. Will keep you updated

[Michael137]

Reproducer (on Intel macOS):

xcrun ${BUILD_DIR}/bin/clang++ \
    -fmodules \
    -gmodules \
    -fcxx-modules \
    -g -O0 \
    -fno-limit-debug-info \
    -nostdlib++ \
    -nostdinc++ \
    -cxx-isystem ${BUILD_DIR}/include/c++/v1 \
    -c \
    -o main.o \
    main.mm

Where main.mm is just:

#import <objc/NSObject.h>

I.e., we're crashing building the ObjectiveC module

[Michael137]

Perhaps a naive patch such as this suffices (it fixed this particular crash for me locally):

diff --git a/clang/lib/CodeGen/ObjectFilePCHContainerWriter.cpp b/clang/lib/CodeGen/ObjectFilePCHContainerWriter.cpp
index 5447b98d7105..02635ce235a1 100644
--- a/clang/lib/CodeGen/ObjectFilePCHContainerWriter.cpp
+++ b/clang/lib/CodeGen/ObjectFilePCHContainerWriter.cpp
@@ -81,6 +81,9 @@ class PCHContainerGenerator : public ASTConsumer {
         if (!TD->isCompleteDefinition())
           return true;
 
+      if (D->hasAttr<NoDebugAttr>())
+        return true;
+
       QualType QualTy = Ctx.getTypeDeclType(D);
       if (!QualTy.isNull() && CanRepresent(QualTy.getTypePtr()))
         DI.getOrCreateStandaloneType(QualTy, D->getLocation());

The PCH generator is asking to emit full standalone debug-info for a type marked NoDebug, which makes no sense. There's other places in CGDebugInfo where we guard against similar asks.

[ldionne]

CCing @dwblaikie @adrian-prantl @echristo as maintainers for Debug info (according to clang/Maintainers.rst).

To summarize the situation, we need to find a way to fix the Clang frontend crash above so we can revert the patch landed in this PR. Until we've done that, libc++ is going to produce lots of debug information for things that users don't care about, leading to an explosion of the debug info size for no user benefit. Since the LLVM 20 release is approaching, I believe it would be important to get this fixed soon to avoid shipping a release (or even release candidate) that contains the workaround in this PR. I hope someone more knowledgeable about debug info and the moving parts around it can take ownership of this issue, since I am not the best person to do that.

[dwblaikie]

(might've been best to revert the previous patch rather than create this urgency - and that'd still be an option to avoid shipping this regression)

But it seems like it should be a pretty easy fix: #118710 (comment)

[dwblaikie]

Ah, hmm, the PCH case might not be addressed by the above fix - might be multiple issues/fixes required.

[ldionne]

(might've been best to revert the previous patch rather than create this urgency - and that'd still be an option to avoid shipping this regression)

As I explained in the other thread, this change touches a lot of things and reverting the original patch would have caused a bunch of conflicts. If it comes to that we could probably shuffle around and do it, but in my experience it's better to fix these things fast when they happen, otherwise we'll just carry this debt for the foreseeable future. I don't know how many times we've been in the same situation and whenever we don't have a direct incentive to fix something, it just doesn't happen. So as of right now, I still feel like turning this off temporarily and trying to get the underlying issues fixed is the right approach, but we can reevaluate later if there are deep reasons why this is difficult to fix.

[dwblaikie]

(might've been best to revert the previous patch rather than create this urgency - and that'd still be an option to avoid shipping this regression)

As I explained in the other thread, this change touches a lot of things and reverting the original patch would have caused a bunch of conflicts.

I appreciate that it's a big patch & causes a lot of churn to commit/revert - though the number of failures, and the impact of disabling LIBCPP_NODEBUG entirely (substantial regressions in debug info size) seems enough to warrant a revert-to-green to me.

If it comes to that we could probably shuffle around and do it, but in my experience it's better to fix these things fast when they happen, otherwise we'll just carry this debt for the foreseeable future.

That doesn't seem like the best approach - sort of using this to force prioritizing a fix with urgency that is artificial.

I don't know how many times we've been in the same situation and whenever we don't have a direct incentive to fix something, it just doesn't happen. So as of right now, I still feel like turning this off temporarily and trying to get the underlying issues fixed is the right approach, but we can reevaluate later if there are deep reasons why this is difficult to fix.

The direct incentive seems to be that we both/all want debug info size reductions. And the folks who care about that most are the folks who work on debug info emission, the ones most suited to fix the bugs... so it feels like incentives are aligned, without the need for this extra time pressure/standing regression/leverage in the project.

[ldionne]

I appreciate that it's a big patch & causes a lot of churn to commit/revert - though the number of failures, and the impact of disabling LIBCPP_NODEBUG entirely (substantial regressions in debug info size) seems enough to warrant a revert-to-green to me.

There's a tradeoff. At the moment when we discovered the issues and decided we needed to disable debug info (or revert the patch), it would have taken me (or @philnik777) some time to work through the undoing of the patch and the subsequent ones that had landed, and then additional time to go through our CI to land that revert. If the Clang bugs were pretty straightforward to fix, it could easily not be worth it. I still feel like that was the right call, but that doesn't mean we don't want to go back and partially revert the patch now based on our understanding of the bugs that need fixing. Do you have a clear view of where we stand with respect to fixing those bugs?

The direct incentive seems to be that we both/all want debug info size reductions. And the folks who care about that most are the folks who work on debug info emission, the ones most suited to fix the bugs... so it feels like incentives are aligned, without the need for this extra time pressure/standing regression/leverage in the project.

I agree that interests are aligned in this case. But everyone's really busy and even important fixes or improvements get delayed (usually indefinitely) in these cases where there's no clear responsibility on someone to fix things, or when a fix is seen only as "nice to have". In my experience, that's an extremely common and recurring theme in LLVM, and it's actually frequent enough to be problematic. So while I would never let a release ship this way, reverting in a way that's too comfortable too quickly has proven to have a stalling effect over and over again. Since I would have to do non-trivial work to make that happen, I made the call not to invest time in doing a detailed revert unless necessary while being willing to revisit that based on our understanding of what's needed to fix Clang.

I think there's a fine line between "ensuring there are incentives to fix something" and "strong-arming into fixing something". I think the former is acceptable and healthy when kept within reason and justified (e.g. in this case the revert was non-trivial), and the later is clearly not. My goal here was to do the former, based on the rationale above.

[Michael137]

The PCH crash is merged now (#123253). Feel free to try and reland this. Will keep an eye out on the LLDB bots.