[clang][Sema] Add checks for validity of default ctor's class #78898

Endilll · 2024-01-21T13:28:04Z

Fixes #10518
Fixes #67914
Fixes #78388
Also addresses the second example in #49103

This patch is based on suggestion from @cor3ntin in #67914 (comment)

@cor3ntin

Fixes llvm#10518 Fixes llvm#67914 Fixes llvm#78388 Also addresses the second example in llvm#49103 This patch is based on suggestion from @cor3ntin in llvm#67914 (comment)

llvmbot · 2024-01-21T13:28:37Z

@llvm/pr-subscribers-libcxx

@llvm/pr-subscribers-clang

Author: Vlad Serebrennikov (Endilll)

Changes

Fixes #10518
Fixes #67914
Fixes #78388
Also addresses the second example in #49103

This patch is based on suggestion from @cor3ntin in #67914 (comment)

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

2 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+5)
(modified) clang/lib/Sema/SemaDeclCXX.cpp (+7)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8bb26fcae18d6b..5971bda21a5e25 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1013,6 +1013,11 @@ Bug Fixes to C++ Support
 - Fix a false-positive ODR violation for different definitions for `std::align_val_t`.
   Fixes (`#76638 <https://github.com/llvm/llvm-project/issues/76638>`_)
 
+- Fix crash when calling the constructor of an invalid class.
+  Fixes (`#10518 <https://github.com/llvm/llvm-project/issues/10518>`_),
+  (`#67914 <https://github.com/llvm/llvm-project/issues/10518>`_),
+  and (`#78388 <https://github.com/llvm/llvm-project/issues/78388>`_)
+
 - Remove recorded `#pragma once` state for headers included in named modules.
   Fixes (`#77995 <https://github.com/llvm/llvm-project/issues/77995>`_)
 
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index df5bd55e7c2836..634af573480b45 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -5990,6 +5990,10 @@ void Sema::ActOnDefaultCtorInitializers(Decl *CDtorDecl) {
 
   if (CXXConstructorDecl *Constructor
       = dyn_cast<CXXConstructorDecl>(CDtorDecl)) {
+    if (CXXRecordDecl *ClassDecl = Constructor->getParent();
+        !ClassDecl || ClassDecl->isInvalidDecl()) {
+      return;
+    }
     SetCtorInitializers(Constructor, /*AnyErrors=*/false);
     DiagnoseUninitializedFields(*this, Constructor);
   }
@@ -14030,6 +14034,9 @@ void Sema::DefineImplicitDefaultConstructor(SourceLocation CurrentLocation,
 
   CXXRecordDecl *ClassDecl = Constructor->getParent();
   assert(ClassDecl && "DefineImplicitDefaultConstructor - invalid constructor");
+  if (ClassDecl->isInvalidDecl()) {
+    return;
+  }
 
   SynthesizedFunctionScope Scope(*this, Constructor);

Endilll · 2024-01-21T13:29:23Z

I decided to not include tests, because our testing infrastructure is not ready to test that Clang doesn't crash without overspecifying the tests using -verify mode.

Fznamznon · 2024-01-22T10:37:51Z

I decided to not include tests, because our testing infrastructure is not ready to test that Clang doesn't crash without overspecifying the tests using -verify mode.

Could you elaborate a bit more on that? What is the exact problem with the testing infrastructure? Can we just add a separate test with the cases from the issues, perhaps without -verify at all?

Endilll · 2024-01-22T11:16:27Z

Could you elaborate a bit more on that? What is the exact problem with the testing infrastructure? Can we just add a separate test with the cases from the issues, perhaps without -verify at all?

I think that the most reliable way to detect a crash would be to leverage our existing machinery for that inside Clang and its driver. Result this machinery produces is exit code.
We should keep in mind that on top of assertion failures, we also have segmentation faults.
Stack traces are also not a given, as can be seen in https://godbolt.org/z/cf7ovjrq3 (Crashing clang-4.0 on ubuntu #31936)
Unfortunately, our exit code in case of crash is not consistent. On Linux, both driver and frontend return 143. On Windows, frontend returns -1073741819 (which is 0xc0000005), which is converted to 1 by driver. The same 1 is used for regular failures due to errors.
From what I was able to find, lit can only check whether exit code is zero or non-zero (via not). not has not --crash mode that can also be leveraged.
This is not helped by the fact that by default piping (to e.g. FileCheck) ignores crashes. lit is following shell behavior here, but thankfully has pipefail option in lit.cfg that can help.

While I think it's possible to craft a general solution out of what we have today, it seems too brittle and intricate. It's also possible to use a simpler solution that works only for e.g. assertion failures. I don't think either of the above are good options, especially when considered at scale. While it's a discussion for another day, we have hundreds (if not thousands) of regression test cases in our bug tracker labeled crash, crash-on-valid, and crash-on-invalid. Some of them have been long gone, but resurfaced during the last 2 years. So I'm not interested in a limited one-off solution just for this PR.

Ideal solution seems to be along the following lines:

Make frontend consistently return e.g. 143 on all platforms in case of crash.
Make lit capable of checking specific exit code.

Fznamznon · 2024-01-22T12:37:57Z

So, there is no way to consistently check on all platforms that we didn't crash when an error diagnostic was issued (does clang return non-zero when there is error diagnostic?), is that a right understanding?
Still, if the tests won't be "pretty" that is not an excuse to not add tests, IMO.

Endilll · 2024-01-22T13:00:09Z

So, there is no way to consistently check on all platforms that we didn't crash when an error diagnostic was issued (does clang return non-zero when there is error diagnostic?), is that a right understanding?

Yes. On Linux and Windows 1 is returned if error diagnostic was issued.

Still, if the tests won't be "pretty" that is not an excuse to not add tests, IMO.

I tried my best to explain that it's not an issues of prettiness. Complicated solution that seems to be possible to craft today feels too brittle, requiring reader to understand how various parts are fit together, not to say I'm not sure myself if something won't slip between piping behavior, not, not --crash, FileCheck, and varied platforms.

If you can point me to existing corpus of crash tests that are done correctly, I'll be happy to follow. So far I saw handful of crash tests here and there across out test suite of various quality: from tests that do not account for segmentation faults (https://github.com/llvm/llvm-project/blob/88d1de5ec64210686d93a90529583505635d257d/clang/test/Parser/objc-diag-width.mm is going to fire in this case even though it's not intended to) to borderline useless tests (https://github.com/llvm/llvm-project/blob/60963272c5c83890910fd97f8d941180b6006fca/clang/test/Sema/crash-deduction-guide-access.cpp is going to pass if clang crashes or error diagnostic is produced, because in both cases exit code is not zero).

Fznamznon · 2024-01-22T13:51:56Z

I tried my best to explain that it's not an issues of prettiness.

Well, ok, perhaps checking that there is actually no crash in clang can be tricky. But what I meant is, since there is no good way to check exit code, why can't we add -verify test? Yes, it will be checking errors that the patch didn't touch, but it is what mostly people do when adding clang tests and it will be +N test cases in a regular test base which not only ensure your change is correct, but the future ones too.

Endilll · 2024-01-22T14:39:24Z

why can't we add -verify test? Yes, it will be checking errors that the patch didn't touch, but it is what mostly people do when adding clang tests and it will be +N test cases in a regular test base which not only ensure your change is correct, but the future ones too.

Because we have more than a couple of tests that should ensure we don't crash. Many of them don't make much sense, e.g. automatically reduced ones (even if identifiers are kept intact; #67914 is not a bad example). Which makes it harder for people altering diagnostic behavior to reason about such tests when they fail to pass CI. It also makes contributors to have more "fun" like @AaronBallman had no so long ago, updating tons of tests because of his VLA-related change.

Overspecifying a significant corpus of hard-to-reason-about tests leads us to a slippery slope of a applying changes blindly and developing scripts akin to update_cc_test_checks.py. I see a significant amount of harm in this direction.

Fznamznon · 2024-01-22T14:51:17Z

Because we have more than a couple of tests that should ensure we don't crash.

Now imagine all these come in without tests because there is no good way to add a test.

I see your point, but again, not having a good way to make a test shouldn't free people from adding tests. Instead we should add tests and work on improving test system.

It also makes contributors to have more "fun" like @AaronBallman had no so long ago, updating tons of tests because of his VLA-related change.

I would rather say it is a specific example. VLAs are an extension in C++ for which we choose how to deal with it. Most of the incoming changes are supported by the standards, so we shouldn't ideally expect many drastic changes in diagnostics like this one.

AaronBallman

Yeah, these changes really need to come with test coverage. We allow landing changes with no test coverage when it's an NFC change or when the testing would require heroic efforts. Otherwise, we expect tests that are as good as you can reasonably make them (sometimes that means going with a unit test instead of a lit test, sometimes it requires creativity in lit, etc). You don't have to fix the testing system first, it's okay to land tests in an unideal state with a FIXME comment, for example.

In terms of the code changes, they look mostly reasonable (some minor nits for coding style conformance) and should be something we can test. We used to crash, so the test is demonstrating that we no longer crash (and that means verifying some particular set of diagnostics are now generated instead or that some IR is now generated, etc). Such a test would be good to have a comment explaining that the code used to crash with a mention or link to the github issue(s).

AaronBallman · 2024-01-22T15:38:10Z

clang/lib/Sema/SemaDeclCXX.cpp

+    if (CXXRecordDecl *ClassDecl = Constructor->getParent();
+        !ClassDecl || ClassDecl->isInvalidDecl()) {
+      return;
+    }


Suggested change

if (CXXRecordDecl *ClassDecl = Constructor->getParent();

!ClassDecl || ClassDecl->isInvalidDecl()) {

return;

}

if (CXXRecordDecl *ClassDecl = Constructor->getParent();

!ClassDecl || ClassDecl->isInvalidDecl())

return;

AaronBallman · 2024-01-22T15:38:22Z

clang/lib/Sema/SemaDeclCXX.cpp

+  if (ClassDecl->isInvalidDecl()) {
+    return;
+  }


Suggested change

if (ClassDecl->isInvalidDecl()) {

return;

}

if (ClassDecl->isInvalidDecl())

return;

Endilll · 2024-01-22T15:47:25Z

So my takeaways here are:

Tests that ensure we don't crash anymore are important for us.
-verify is an acceptable way to write such tests.

Is this correct?

AaronBallman · 2024-01-22T15:55:46Z

So my takeaways here are:

1. Tests that ensure we don't crash anymore are important for us.

2. `-verify` is an acceptable way to write such tests.

Is this correct?

Yup, that's it exactly!

…n-sequence-perform

cor3ntin · 2024-01-26T07:39:09Z

clang/test/SemaCXX/crash-GH10518.cpp

+// RUN: %clang_cc1 -verify -std=c++98 %s
+// RUN: %clang_cc1 -verify -std=c++11 %s
+// RUN: %clang_cc1 -verify -std=c++14 %s
+// RUN: %clang_cc1 -verify -std=c++17 %s
+// RUN: %clang_cc1 -verify -std=c++20 %s
+// RUN: %clang_cc1 -verify -std=c++23 %s
+// RUN: %clang_cc1 -verify -std=c++2c %s


We could do 11 and 2c if you want, i don't think it's useful to test all language modes for these tests (ditto the other ones)

Since newer language modes do not necessarily subsume older ones (think dynamic exception specification), and have disabled code paths, I'm not sure we want to be smart about which language modes to test. If we're going to be smart, then yes, 98, 11, and 2c sounds like a good compromise.

…n-sequence-perform

@philnik

@philnik said that static asserts are the only interesting part of the test

philnik777

The libc++ changes LGTM.

[clang][Sema] Add checks for validity of default ctor's class

b99a75a

Fixes llvm#10518 Fixes llvm#67914 Fixes llvm#78388 Also addresses the second example in llvm#49103 This patch is based on suggestion from @cor3ntin in llvm#67914 (comment)

Endilll requested a review from cor3ntin January 21, 2024 13:28

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 21, 2024

Endilll requested a review from AaronBallman January 22, 2024 11:17

AaronBallman reviewed Jan 22, 2024

View reviewed changes

Endilll added 2 commits January 26, 2024 08:50

Add regression tests for fixed crashes

826dc7a

Merge remote-tracking branch 'upstream/main' into crash-initializatio…

6b1b5fd

…n-sequence-perform

cor3ntin reviewed Jan 26, 2024

View reviewed changes

cor3ntin approved these changes Jan 26, 2024

View reviewed changes

Endilll added 3 commits January 26, 2024 10:51

Merge remote-tracking branch 'upstream/main' into crash-initializatio…

3088cc2

…n-sequence-perform

Merge remote-tracking branch 'upstream/main' into crash-initializatio…

dc22e1f

…n-sequence-perform

Silence libc++ transform_error.mandates.verify.cpp test

60624f7

@philnik said that static asserts are the only interesting part of the test

Endilll requested a review from a team as a code owner February 9, 2024 15:22

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Feb 9, 2024

philnik777 approved these changes Feb 9, 2024

View reviewed changes

Endilll merged commit c58c6aa into llvm:main Feb 9, 2024

Endilll deleted the crash-initialization-sequence-perform branch February 9, 2024 16:59

Endilll mentioned this pull request Oct 15, 2024

[clang] Check null TypeSourceInfo in CreateUnaryExprOrTypeTraitExpr #112111

Open

[clang][Sema] Add checks for validity of default ctor's class #78898

[clang][Sema] Add checks for validity of default ctor's class #78898

Uh oh!

Conversation

Endilll commented Jan 21, 2024

Uh oh!

llvmbot commented Jan 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Endilll commented Jan 21, 2024

Uh oh!

Fznamznon commented Jan 22, 2024

Uh oh!

Endilll commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fznamznon commented Jan 22, 2024

Uh oh!

Endilll commented Jan 22, 2024

Uh oh!

Fznamznon commented Jan 22, 2024

Uh oh!

Endilll commented Jan 22, 2024

Uh oh!

Fznamznon commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

AaronBallman Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

Endilll commented Jan 22, 2024

Uh oh!

AaronBallman commented Jan 22, 2024

Uh oh!

cor3ntin Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

Endilll Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

philnik777 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Jan 21, 2024 •

edited

Loading

Endilll commented Jan 22, 2024 •

edited

Loading

Fznamznon commented Jan 22, 2024 •

edited

Loading