This is an archive of the discontinued LLVM Phabricator instance.

Fix crash on overloaded postfix unary operators due to invalid SourceLocation
ClosedPublic

Authored by eduucaldas on Jul 1 2020, 5:36 AM.

Diff Detail

Event Timeline

eduucaldas created this revision.Jul 1 2020, 5:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2020, 5:36 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
eduucaldas marked an inline comment as done.

Code that reproduces the crash
Notice that when using a postfix unary operator ++ one argument is introduced to differ it from its prefix counterpart, but that argument is not used. This phantom argument shows up in the ClangAST in the form of an IntegerLiteral with invalid sloc. This invalid sloc in a valid AST node was causing the SyntaxTree generation to crash.
We can address this problems at two different levels:

  1. At the TraverseCXXOperatorCallExpr, by overriding it and replacing the children iteration to not iterate over this phantom argument.
  2. At the WalkUpFromIntegerLiteral, by skipping the visitation of the phantom node.

We preferred the latter.

  1. Cons: We would have to duplicate the implementation of RecursiveASTVisitor in BuildTree, to handle this subtle change in traversal. That would add code complexity.
  2. Cons: We are handling a problem of CXXOperatorCallExpr in IntegerLiteral.

We chose the latter as, anyways, if an AST node has an invalid sloc, it was either a problem with parsing or the node is supposed to be ignored

clang/include/clang/AST/ExprCXX.h
143–148

Should new additions to CXXOperatorCallExpr go on a different patch?
I'll also add unit tests for those. I'm waiting for review on https://reviews.llvm.org/D82937, to do that.

riccibruno added inline comments.
clang/lib/AST/ExprCXX.cpp
82

This will be true for operator->.

eduucaldas marked an inline comment as done.Jul 1 2020, 9:08 AM
eduucaldas added inline comments.
clang/lib/AST/ExprCXX.cpp
82

Thank you ! I had missed that! I'll propose a more robust solution. I'll probably split this patch. But I'll take care to subscribe you to anything related

  • Remove predicates in CXXOperatorCallExpr.
  • Implement getOperatorNodeKind to dispatch from CXXOperatorCallExpr to syntax constructs like BinaryOperatorExpression, PrefixUnaryOperatorExpression...
eduucaldas updated this revision to Diff 275318.Jul 3 2020, 1:00 AM

Revert mistake on last update, used arc diff --update with the wrong diff

eduucaldas added inline comments.Jul 3 2020, 1:50 AM
clang/lib/Tooling/Syntax/BuildTree.cpp
117

Where to put this logic?

The pro of having this function inside BuildTree.cpp is that it is closer to it's *only* usage. And also for now BuildTree.cpp agglomerates everything that takes a semantic AST as an input, so it would be coherent.

Another option is to put it in Nodes, as it might serve as documentation for *OperatorExpressions.
For example, it would document that the semantic node CXXOperatorCallExpr can also generate the syntax node BinaryOperatorExpression, which was previously only generated by a semantic BinaryOperator

Another option is to add put this function as a lambda inside WalkUpFromCXXOperatorCallExpr as probably it will only be used there.

gribozavr2 retitled this revision from Fix crash on overloaded postfix unary operators due to invalid sloc to Fix crash on overloaded postfix unary operators due to invalid SourceLocation.Jul 3 2020, 2:14 AM
eduucaldas added inline comments.Jul 5 2020, 11:57 PM
clang/lib/Tooling/Syntax/BuildTree.cpp
187

Actually arrow star is treated like a normal binary operator in the grammar

gribozavr2 added inline comments.Jul 6 2020, 10:06 AM
clang/lib/Tooling/Syntax/BuildTree.cpp
117

Placing this helper here makes sense to me. However, please mark it static.

177

"Not supported by syntax tree *yet*"

651

WDYT about overriding TraverseCXXOperatorCallExpr, so that we don't even visit the synthetic argument of the postfix unary ++? I would prefer to not introduce blanket "if invalid then ignore" checks in the code.

761

"getOperatorNodeKind() does not return this value"

clang/unittests/Tooling/Syntax/TreeTest.cpp
2278

Could you also add tests for a prefix unary operator?

2321

I'm not sure about this part where ++ is wrapped in IdExpression -- shouldn't the syntax tree look identical to a builtin postfix ++ (see another test in this file)?

->* and , are binary operators.
Unit tests covering most of the operator kinds

eduucaldas updated this revision to Diff 275940.Jul 7 2020, 1:33 AM
eduucaldas marked 9 inline comments as done.

Answering comments

clang/lib/Tooling/Syntax/BuildTree.cpp
651

Code that reproduces the crash
Notice that when using a postfix unary operator ++ one argument is introduced to differ it from its prefix counterpart, but that argument is not used. This phantom argument shows up in the ClangAST in the form of an IntegerLiteral with invalid sloc. This invalid sloc in a valid AST node was causing the SyntaxTree generation to crash.
We can address this problems at two different levels:

  1. At the TraverseCXXOperatorCallExpr, by overriding it and replacing the children iteration to not iterate over this phantom argument.
  2. At the WalkUpFromIntegerLiteral, by skipping the visitation of the phantom node.

We preferred the latter.

  1. Cons: We would have to duplicate the implementation of RecursiveASTVisitor in BuildTree, to handle this subtle change in traversal. That would add code complexity.
  2. Cons: We are handling a problem of CXXOperatorCallExpr in IntegerLiteral.

We chose the latter as, anyways, if an AST node has an invalid sloc, it was either a problem with parsing or the node is supposed to be ignored

I've explained my reasoning in my first comment for this patch. But as it was a long time ago, I guess it got lost, even by me.

I'll sketch how the Traverse solution would look like, to be able to give more concrete arguments.

clang/unittests/Tooling/Syntax/TreeTest.cpp
2321

This comes back to a discussion we had a month ago about operators ( +, !, etc)
Question: Should we represent operators (built-in or overloaded) in the syntax tree uniformly? If so in which way?
Context: The ClangAST stores built-in operators as mere tokens, whereas overloaded operators are represented as a DeclRefExpr. That makes a lot of sense for the ClangAST, as we might refer back to the declaration of the overloaded operator, but the declaration of built-in operator doesn't exist.
Conclusion: Have the same representation for both types of operators. I have implemented the "unboxed" representation of overloaded operators, i.e. just storing their token in the syntax tree. I have not committed that, but I can do it just after this patch.

eduucaldas updated this revision to Diff 275942.Jul 7 2020, 1:40 AM

Unifying user defined binary operator tests

gribozavr2 accepted this revision.Jul 7 2020, 4:13 AM
gribozavr2 added inline comments.
clang/lib/Tooling/Syntax/BuildTree.cpp
148

Please drop this comment, I don't think it helps understand the code.

clang/unittests/Tooling/Syntax/TreeTest.cpp
2137

I don't think we need a separate class to show the left shift operator. The declaration below can be:

friend X operator<<(X&, const X&);
2143

"TODO: Fix the crash on ->* and add a test for it."

2321

SGTM. Please just leave a FIXME or TODO in this patch that shows that certain parts of tests are not final.

This revision is now accepted and ready to land.Jul 7 2020, 4:13 AM
eduucaldas updated this revision to Diff 276344.Jul 8 2020, 2:08 AM
eduucaldas marked 4 inline comments as done.

answering minor comments

clang/unittests/Tooling/Syntax/TreeTest.cpp
2137

If we don't bother much about "realistic" operator declarations we could drop all the friend and declare every operator in their most concise form. WDYT

gribozavr2 added inline comments.Jul 8 2020, 3:14 AM
clang/unittests/Tooling/Syntax/TreeTest.cpp
2137

I think we shouldn't try to make tests realistic in terms of function names etc., but we should try to cover as many different AST shapes as possible. In the case of binary operators, we have three cases -- free function, friend function, member function, that all generate slightly different ASTs, so I believe we should try to cover them all.

eduucaldas marked 2 inline comments as done.Jul 8 2020, 4:56 AM
eduucaldas added inline comments.
clang/unittests/Tooling/Syntax/TreeTest.cpp
2137

But those all regard the operators declaration.
Here we test the operator call expression - CXXOperatorCallExpr.

I think we should test friend function declarations when we add support for them in the tree, and then we add tests for declaration of friend operators, friend member functions and whatnot.

gribozavr2 added inline comments.Jul 8 2020, 5:39 AM
clang/unittests/Tooling/Syntax/TreeTest.cpp
2137

The call expression AST node can be meaningfully different between calls to member and non-member functions.

eduucaldas updated this revision to Diff 276408.Jul 8 2020, 6:38 AM
eduucaldas marked an inline comment as done.

Switch to TraverseCXXOperatorCallExpr

eduucaldas marked an inline comment as done.Jul 8 2020, 6:39 AM
gribozavr2 accepted this revision.Jul 8 2020, 6:44 AM
gribozavr2 added inline comments.
clang/lib/Tooling/Syntax/BuildTree.cpp
728

"is used to distinguish"

731

"... because does not correspond to anything written in the source code"

eduucaldas updated this revision to Diff 276411.Jul 8 2020, 6:49 AM
eduucaldas marked 2 inline comments as done.

minor fix comments

This revision was automatically updated to reflect the committed changes.