This is an archive of the discontinued LLVM Phabricator instance.

[clang][ExprConstant] Fix crash on uninitialized base class subobject
ClosedPublic

Authored by hazohelet on Jun 28 2023, 6:33 AM.

Details

Summary

This patch fixes the reported regression caused by D146358 through adding notes about an uninitialized base class when we diagnose uninitialized constructor.

This also changes the wording from the old one in order to make it clear that the uninitialized subobject is a base class and its constructor is not called.
Wording changes:
BEFORE: subobject of type 'Base' is not initialized
AFTER: constructor of base class 'Base' is not called

Fixes https://github.com/llvm/llvm-project/issues/63496

Diff Detail

Event Timeline

hazohelet created this revision.Jun 28 2023, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2023, 6:33 AM
hazohelet requested review of this revision.Jun 28 2023, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2023, 6:33 AM
hazohelet added inline comments.Jun 28 2023, 6:37 AM
clang/lib/AST/ExprConstant.cpp
2379

The reason I put the assertion here in the original patch was that there were not enough test cases to see what could cause null here, and I thought we could rethink later about this assertion's validness through crash reports like this. FWIW, I thought this assertion could be beneficial at that time because it had caught two internal bugs.

If reviewers do not agree with the assertion here anymore, we can fallback to the old diagnostic(subobject of type 'A' is not initialized) when we have invalid subobject, or revert the original patch.
Currently this patch is a partial fallback to the old diagnostic.

tbaeder added inline comments.Jun 28 2023, 10:15 AM
clang/lib/AST/ExprConstant.cpp
2422

Can you pass << BS.getSourceRange() here? Does that improve things?

hazohelet added inline comments.Jun 29 2023, 8:18 AM
clang/lib/AST/ExprConstant.cpp
2422

Currently, DiagLoc points to the variable declaration and the BS.getSourceRange covers the line where the base class is inherited. This causes distant source range and thus unnecessarily many lines of snippet printing.
e.g.

struct Base {
  Base() = delete;
};
struct Derived : Base {
  constexpr Derived() {}
};
constexpr Derived dd;

Output:

source.cpp:7:19: error: constexpr variable 'dd' must be initialized by a constant expression
    7 | constexpr Derived dd;
      |                   ^~
source.cpp:7:19: note: constructor of base class 'Base' is not called
    7 | struct Derived : Base {
      |                  ~~~~
    8 |   constexpr Derived() {}
    9 | };
   10 | constexpr Derived dd;
      |                   ^
source.cpp:4:18: note: base class inherited here
    4 | struct Derived : Base {
      |                  ^

(The line numbers seem incorrect but is already reported in https://github.com/llvm/llvm-project/issues/63524)

I think we don't need two notes here because the error is already pointing to the variable declaration. Having something like the following would be succint.

source.cpp:7:19: error: constexpr variable 'dd' must be initialized by a constant expression
    7 | constexpr Derived dd;
      |                   ^~
source.cpp:4:18: note: constructor of base class 'Base' is not called
    4 | struct Derived : Base {
      |                  ^~~~

Providing source range would be beneficial because the inherited class often spans in a few lines (the code in the crashing report, for example)

hazohelet added inline comments.Jun 29 2023, 9:12 AM
clang/lib/AST/ExprConstant.cpp
2422

Sorry, I was looking at the line above. The distant source range problem doesn't occur.

I tested another input

struct Base {
  Base() = delete;
  constexpr Base(int){}
};

struct Derived : Base {
  constexpr Derived() {}
  constexpr Derived(int n): Base(n) {}
};

constexpr Derived darr[3] = {1, Derived(), 3};

expecting that the DiagLoc points to the second initializer Derived(), but it pointed to the same location as the error, so I'm still in favor of the idea of having a single note here.

hazohelet updated this revision to Diff 536771.Jul 3 2023, 7:56 AM
hazohelet edited the summary of this revision. (Show Details)
  • Removed the base class inherited here redundant note
  • Provided source range and added test for it

The provided source range is NOT directly calling CXXBaseSpecifier::getSourceRange so as not to cover the access specifiers like public, private or protected

aaron.ballman added inline comments.Jul 10 2023, 10:55 AM
clang/docs/ReleaseNotes.rst
378–379
clang/lib/AST/ExprConstant.cpp
2422

Erich's suggestion in https://github.com/llvm/llvm-project/issues/63496#issuecomment-1607415201 was to continue to evaluate the constructor because there may be further follow-on diagnostics that are relevant and not related to the base class subobject. I tend to agree -- is there a reason why you're not doing that here?

clang/test/SemaCXX/constexpr-subobj-initialization.cpp
31

I'd also like to see a test case with more interesting base class specifiers:

struct Foo {
  constexpr Foo();
};

struct Bar : protected Foo {
  int i;
  constexpr Bar() : i(12) {}
}

template <typename Ty>
struct Baz {
  constexpr Baz();
};

struct Quux : Baz<Foo>, private Bar {
  int i;
  constexpr Quux() : i(12) {}
}
hazohelet marked 3 inline comments as done.

Address comments from aaron.ballman

  • Added more tests
hazohelet added inline comments.Jul 10 2023, 11:52 AM
clang/lib/AST/ExprConstant.cpp
2422

My question (https://github.com/llvm/llvm-project/issues/63496#issuecomment-1607177233) was whether or not we should utilize constant evaluator even if the evaluated expression is a semantically-invalid constructor like the crashing case.
So in my understanding, Erich's suggestion was that we should continue utilizing the constant evaluator in these cases, and stopping the evaluator here at uninitialized base class subobject is something else.

aaron.ballman added inline comments.Jul 13 2023, 5:30 AM
clang/lib/AST/ExprConstant.cpp
2422

Our usual strategy is to continue compilation to try to find follow-on issues. For example: https://godbolt.org/z/qrMchvh1f -- even though the constructor declaration is not valid, we still go on to diagnose issues within the constructor body.

aaron.ballman added inline comments.Aug 1 2023, 6:30 AM
clang/lib/AST/ExprConstant.cpp
2422

This is the only outstanding discussion that I see left in the review, and it may be due to a misunderstanding. Am I correct that your changes cause us to not diagnose further issues in the constructor body because we're stopping here at the uninitialized base class subobject? e.g., do we still diagnose this https://godbolt.org/

aaron.ballman added inline comments.Aug 1 2023, 6:33 AM
clang/lib/AST/ExprConstant.cpp
2422

Oops, I munged that link. I meant this one: https://godbolt.org/z/qrMchvh1f

hazohelet added inline comments.Aug 1 2023, 7:42 AM
clang/lib/AST/ExprConstant.cpp
2422

I don't think it's correct. This patch does not change clang's behavior for the code in that link.
This patch does not introduce any change in diagnostic behavior from clang 16 (the previous patch is NOT included) other than the wording. Clang16 was also stopping the interpreter here. Links below are clang 16 codes.

https://github.com/llvm/llvm-project/blob/7cbf1a2591520c2491aa35339f227775f4d3adf6/clang/lib/AST/ExprConstant.cpp#L2395-L2399
Clang 16 calls CheckEvaluationResult here.
https://github.com/llvm/llvm-project/blob/7cbf1a2591520c2491aa35339f227775f4d3adf6/clang/lib/AST/ExprConstant.cpp#L2355-L2361
And the CheckEvaluationResult called there immediately returns false if Value.hasValue() evaluates to false.

This patch only adds this Value.hasValue() check here before calling CheckEvaluationResult to emit more precise diagnostics and fix the regression. It does nothing more than that.

aaron.ballman accepted this revision.Aug 3 2023, 9:45 AM

LGTM!

clang/lib/AST/ExprConstant.cpp
2422

Ah thank you for the explanation!

This revision is now accepted and ready to land.Aug 3 2023, 9:45 AM

Because this is fixing a regression, it may be worth backporting it to the 17.x branch: https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches

This revision was landed with ongoing or failed builds.Aug 7 2023, 11:56 PM
This revision was automatically updated to reflect the committed changes.