This is an archive of the discontinued LLVM Phabricator instance.

[AST] Build recovery expressions by default for C++.
ClosedPublic

Authored by hokein on Mar 24 2020, 5:44 AM.

Diff Detail

Event Timeline

hokein created this revision.Mar 24 2020, 5:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2020, 5:44 AM
hokein updated this revision to Diff 252320.Mar 24 2020, 7:42 AM

fix the clangd test.

Do you also want to update LangOpts.td to make the default for the langopt equal to CPlusPlus?
(I saw other opts doing that, not sure exactly what it affects, may be voodoo cargo cult stuff)

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
1198 ↗(On Diff #252320)

Why did the original test start failing? Is this a regression?

(This certainly seems OK to regress and fix later, but there may be other consequences?)

sammccall accepted this revision.Mar 24 2020, 10:02 AM
This revision is now accepted and ready to land.Mar 24 2020, 10:02 AM
hokein updated this revision to Diff 252513.Mar 25 2020, 12:40 AM
hokein marked an inline comment as done.

address review comments.

Do you also want to update LangOpts.td to make the default for the langopt equal to CPlusPlus?
(I saw other opts doing that, not sure exactly what it affects, may be voodoo cargo cult stuff)

ah, I missed that.

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
1198 ↗(On Diff #252320)

yes, but I didn't dig into it, it is likely a regression, added a FIXME (looking at other cases in this test, they just use 10 as the parameter).

This revision was automatically updated to reflect the committed changes.

We found an odd case in our downstream fork after this patch landed. We have a diagnostic in CheckVariableDeclarationType that checks for automatic variables that are too large for the stack address space, and it chokes on the testcase Parser/objcxx11-invalid-lambda.cpp:

void foo() {  // expected-note {{to match this '{'}}
  int bar;
  auto baz = [
      bar(  // expected-note {{to match this '('}} expected-note {{to match this '('}}
        foo_undeclared() // expected-error{{use of undeclared identifier 'foo_undeclared'}}
      /* ) */
    ] () { };   // expected-error{{expected ')'}}
}               // expected-error{{expected ')'}} expected-error {{expected ',' or ']'}} expected-error{{expected ';' at end of declaration}} expected-error{{expected '}'}}

When the lambda is parsed, the parsing of the initializer expression of the 'bar' capture fails since the paren is unbalanced, so it will build:

ParenListExpr 0xc592ce8 'NULL TYPE' contains-errors
`-RecoveryExpr 0xc592cc0 '<dependent type>' contains-errors lvalue
  `-UnresolvedLookupExpr 0xc592c38 '<overloaded function type>' lvalue (ADL) = 'foo_undeclared' empty

Then, when building the lambda closure type, it will be given an auto member for bar with its type deduced to AutoType 0xc592d40 'auto' dependent:

CXXRecordDecl 0xdfeb798 <...> col:14 implicit class definition
|-DefinitionData lambda pass_in_registers standard_layout trivially_copyable can_const_default_init
| |-DefaultConstructor
| |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| |-MoveConstructor exists simple trivial needs_implicit
| |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
| |-MoveAssignment
| `-Destructor simple irrelevant trivial
|-CXXMethodDecl 0xdfeb8d0 <line:10:8, col:12> line:5:14 operator() 'void () const' inline
| `-CompoundStmt 0xdfeba20 <line:10:10, col:12>
|-FieldDecl 0xdfeba58 <line:6:7> col:7 implicit 'auto'
`-CXXDestructorDecl 0xdfebb08 <line:5:14> col:14 implicit referenced ~ 'void () noexcept' inline default trivial

However, this just means that baz has a very odd type; it's a variable of closure type that contains a dependent member that will never be resolved. Our diagnostic breaks, since the variable's type isn't dependent, but it has a member which is, and we can't take the size of that.

Is the lambda parser really supposed to build this kind of odd type when faced with RecoveryExpr?

However, this just means that baz has a very odd type; it's a variable of closure type that contains a dependent member that will never be resolved. Our diagnostic breaks, since the variable's type isn't dependent, but it has a member which is, and we can't take the size of that.

Interesting. Simply turning this off for lambdas isn't enough. The following crashes clang for me:

class X {
  decltype(unresolved()) foo;
};
constexpr int s = sizeof(X);

@hokein I think we need to revert until we find a solution for this. I suspect this is going to mean adding the error bit to type and... dropping members that have it set? Or marking them as invalid?

Thanks for the test case. Reverted in https://github.com/llvm/llvm-project/commit/62dea6e9be31b100962f9ad41c1b79467a53f6cd for now. Adding the error-bit to type looks like a right direction.

We have also encountered crashes in downstream testing caused by this using just the vanilla source from trunk. When there is a proposed fix, please let us know so we can test. Thanks.

@ebevhan Thanks again for the testcase. I've added a reduced version in 47e7bdb10732e6f140adce39a1bc34e3ee2a6ea6 to ensure this is fixed on re-land.
This particular case can be fixed by marking the decl as invalid I think, I'm curious how this generalizes.

We have also encountered crashes in downstream testing caused by this using just the vanilla source from trunk. When there is a proposed fix, please let us know so we can test. Thanks.

We can try but it would be very useful to have details, no idea whether it's going to be the same issue and the more we know about the easier it is to improve the design.
Can you describe the case at all? Any chance of a minimal example? Ideally we'd check it in.

We have also encountered crashes in downstream testing caused by this using just the vanilla source from trunk. When there is a proposed fix, please let us know so we can test. Thanks.

We can try but it would be very useful to have details, no idea whether it's going to be the same issue and the more we know about the easier it is to improve the design.
Can you describe the case at all? Any chance of a minimal example? Ideally we'd check it in.

All of them appear to have unresolved names in constexpr evaluation. It is likely to be the same issue.

All of them appear to have unresolved names in constexpr evaluation. It is likely to be the same issue.

The general scheme is probably common: unresolved expr -> ??? -> an expression is dependent but not marked as such -> constant evaluation crashes.

But the ??? matters, as that's where the fix is.
In the case above: expr is used in a member of X, and X is not a dependent type, so sizeof(X) is not considered dependent

The general scheme is probably common: unresolved expr -> ??? -> an expression is dependent but not marked as such -> constant evaluation crashes.

But the ??? matters, as that's where the fix is.
In the case above: expr is used in a member of X, and X is not a dependent type, so sizeof(X) is not considered dependent

The context, if I understand correctly for the cases I am seeing, boil down to:

  • Value of a member initializer for a constexpr constructor
  • Bitfield width

The general scheme is probably common: unresolved expr -> ??? -> an expression is dependent but not marked as such -> constant evaluation crashes.

But the ??? matters, as that's where the fix is.
In the case above: expr is used in a member of X, and X is not a dependent type, so sizeof(X) is not considered dependent

The context, if I understand correctly for the cases I am seeing, boil down to:

  • Value of a member initializer for a constexpr constructor
struct X {
  int Y;
  constexpr X() : Y(foo()) {]
};

This will need a different fix I think. Maybe just isPotentialConstantExpr needs to bail out if there are errors.

  • Bitfield width
struct X { int Y : foo(); };
constexpr int Z = sizeof(X);

I think this one is just another case of marking the fielddecl as invalid.