Page MenuHomePhabricator

Ignore trailing NullStmts in StmtExprs for GCC compatibility
ClosedPublic

Authored by domdom on Jan 22 2019, 10:50 PM.

Details

Summary

Ignore trailing NullStmts in compound expressions when determining the result type and value. This is to match the GCC behavior which ignores semicolons at the end of compound expressions.

Diff Detail

Event Timeline

domdom created this revision.Jan 22 2019, 10:50 PM
domdom updated this revision to Diff 183043.Jan 22 2019, 11:32 PM

Formatting

aaron.ballman edited reviewers, added: aaron.ballman; removed: lattner.Jan 23 2019, 6:17 AM
aaron.ballman added a subscriber: aaron.ballman.

Can you also add a test case to TestMiscStmts() in clang\test\AST\ast-dump-stmt.c that demonstrates we calculate the correct resulting type and don't lose the extra null statements from the AST?

clang/lib/Parse/ParseStmt.cpp
975–984

gcc -> GCC

Also, add a full stop to the end of the comment.

976

This should be unsigned rather than int.

978

Prefer ++LookHead; since the result isn't needed.

984

Remove spurious newline.

clang/lib/Sema/SemaExpr.cpp
13372

Add full stop to the end of the sentence.

13374–13377

Add full stop to the end of the sentence.

13377

Add full stop to the end of the sentence.

domdom updated this revision to Diff 184001.Jan 28 2019, 5:44 PM

Thanks for your comments, @aaron.ballman. I've addressed the comments and added a test case as suggested.

This revealed an issue with the code-gen side of things, so I fixed that and added another test case for that as well.

domdom marked 7 inline comments as done.Jan 28 2019, 5:46 PM
aaron.ballman added inline comments.Jan 30 2019, 5:51 AM
clang/include/clang/AST/Stmt.h
1303

How about getIndexOfLastNonNullStmt() since it doesn't return the Stmt* itself?

clang/lib/CodeGen/CGStmt.cpp
387

const Stmt *

412

What happens if ExprResult is nullptr?

clang/test/CodeGen/exprs.c
202

Does this test need the extra null statement between the 3; and 4;?

domdom updated this revision to Diff 184413.Jan 30 2019, 5:01 PM
domdom updated this revision to Diff 184414.
domdom marked 6 inline comments as done.
domdom marked an inline comment as not done.Jan 30 2019, 5:05 PM
domdom added inline comments.
clang/include/clang/AST/Stmt.h
1303

Agreed.

1328

getLastNonNullStmt asserts anyway, should I remove this?

clang/lib/CodeGen/CGStmt.cpp
412

Then ExprResult == *I should not be evaluated. (Since GetLast would be false)

clang/test/CodeGen/exprs.c
202

Not strictly speaking, no. Just added it to ensure it has no effect.

aaron.ballman added inline comments.Jan 31 2019, 10:47 AM
clang/include/clang/AST/Stmt.h
1303

What do you think about having this function return an optional<unsigned> rather than asserting? This will keep the API safe when called on an empty CompoundStmt.

1328

If you did switch to using an optional, then you can assert the optional has a value here. The effect should be the same as your current approach, but with slightly more tolerance.

1337

And if getLastIndexOfNonNullStmt() returns an empty value here, you can return nullptr to signify that there is no statement expression result possible.

clang/lib/CodeGen/CGStmt.cpp
412

I think I was worried about a different situation that isn't present in this patch. However, if you switch to using optional, then getStmtExprResult() could potentially return null as well (when GetLast is true), so we'd have to handle that (probably with an assertion).

clang/test/CodeGen/exprs.c
202

That sounds good to me!

domdom updated this revision to Diff 185196.Feb 4 2019, 5:13 PM

Switched getIndexOfLastNonNullStmt to return Optional<unsigned> instead of Stmt* as per comments.

domdom marked 6 inline comments as done.Feb 4 2019, 5:14 PM
aaron.ballman added inline comments.Feb 5 2019, 7:48 AM
clang/include/clang/AST/Stmt.h
1301–1302

Given the name of the function, why return the index of the last null statement if it only contains null statements?

1312

The only way you can get here is if all statements are null statements, so this should return None as well, shouldn't it?

1338–1339

return ExprResult ? body_begin()[*ExprResult] : nullptr;

riccibruno added inline comments.
clang/include/clang/AST/Stmt.h
1302

Additionally I am not sure that the comment is optimal. This is inside CompoundStmt and it is therefore strange to use "if this compound expression", since the compound expression is represented with the StmtExpr node.

1326

I think it needs at the very least to mention that this is about the GNU extension. Perhaps it would be useful to mention the relation between the CompoundStmt node and the StmtExpr node ? Also more generally is it not possible to avoid mutating the compound statement node after it has been created ?

1335

Same, I would find it clearer if you mentioned the extension.

You'll also need to update TreeTransform::TransformCompoundStmt. (And please add some tests for template instantiation.)

domdom updated this revision to Diff 203775.Jun 9 2019, 11:10 PM

Sorry I it's taken me a while to get back to this work. I've rebased the changes and taken advantage of the refactoring to stop modifying the CompoundStmt after creating it. This definitely simplified the changes required in Stmt.h, which is nice.

I've addressed the the need to update the TreeTransformer for the template case, and added a test case for that.

Something I should ask, it seems like GCC only ignores the NullStmts at the end if it's in C mode. Should clang match this behaviour exactly?

aaron.ballman accepted this revision.Mon, Jun 17, 9:30 AM

Sorry I it's taken me a while to get back to this work. I've rebased the changes and taken advantage of the refactoring to stop modifying the CompoundStmt after creating it. This definitely simplified the changes required in Stmt.h, which is nice.

I've addressed the the need to update the TreeTransformer for the template case, and added a test case for that.

Thanks! I think this is looking good to me, but you should wait for the other reviewers before committing in case they have further concerns.

Something I should ask, it seems like GCC only ignores the NullStmts at the end if it's in C mode. Should clang match this behaviour exactly?

I can't think of a reason that this should only happen in C mode, can you @rsmith?

clang/test/SemaCXX/statements.cpp
41–52

Be sure to run the patch through clang-format.

This revision is now accepted and ready to land.Mon, Jun 17, 9:30 AM

Something I should ask, it seems like GCC only ignores the NullStmts at the end if it's in C mode. Should clang match this behaviour exactly?

I can't think of a reason that this should only happen in C mode, can you @rsmith?

No, I can't think of such a reason either. When we've seen such weirdnesses before (eg, break or continue in a statement expression in a for loop increment expression) we generally try to pick a behavior that's consistent across languages, and warn on the cases where we give a different behavior from GCC as a result.

domdom updated this revision to Diff 205501.Tue, Jun 18, 8:30 PM

clang-format the patch

domdom marked an inline comment as done.Tue, Jun 18, 8:31 PM

clang-format the patch

Thanks! Do you need someone to commit on your behalf?

clang-format the patch

Thanks! Do you need someone to commit on your behalf?

You are very welcome; thank you both for your comments!

I do need someone to commit on my behalf :)

clang-format the patch

Thanks! Do you need someone to commit on your behalf?

You are very welcome; thank you both for your comments!

I do need someone to commit on my behalf :)

I'm sorry for the incredibly long delay in committing this for you -- I managed to lose track of this thread. I went to apply the changes today and get the following test failures when trying on Windows 10 x64:

FAIL: Clang :: AST/ast-dump-stmt.c (152 of 11055)
******************** TEST 'Clang :: AST/ast-dump-stmt.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   c:\cmakebuilds\build\x64-debug\bin\clang.exe -cc1 -internal-isystem c:\cmakebuilds\build\x64-debug\lib\clang\9.0.0\include -nostdsysteminc -std=gnu11 -ast-dump C:\llvm\tools\clang\test\AST\ast-dump-stmt.c | c:\cmakebuilds\build\x64-debug\bin\filecheck.exe -strict-whitespace C:\llvm\tools\clang\test\AST\ast-dump-stmt.c
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "c:\cmakebuilds\build\x64-debug\bin\clang.exe" "-cc1" "-internal-isystem" "c:\cmakebuilds\build\x64-debug\lib\clang\9.0.0\include" "-nostdsysteminc" "-std=gnu11" "-ast-dump" "C:\llvm\tools\clang\test\AST\ast-dump-stmt.c"
# command stderr:
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:48:3: warning: expression result unused
  -T1;
  ^~~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:49:3: warning: expression result unused
  -T2;
  ^~~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:58:3: warning: expression result unused
  ~T1;
  ^~~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:59:3: warning: expression result unused
  ~T2;
  ^~~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:70:21: warning: expression result unused
  _Generic(i, int : 12);
                    ^~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:79:21: warning: expression result unused
  _Generic(i, int : 12, default : 0);
                    ^~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:90:34: warning: expression result unused
  _Generic(i, default : 0, int : 12);
                                 ^~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:101:21: warning: expression result unused
  _Generic(i, int : 12, float : 10, default : 100);
                    ^~
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:142:3: warning: expression result unused
  0;
  ^
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:288:8: warning: expression result unused
  for (b; b; b)
       ^
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:288:14: warning: expression result unused
  for (b; b; b)
             ^
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:367:17: warning: expression result unused
  ({int a = 10; a;});
                ^
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:375:3: warning: expression result unused
  ({int a = 10; a;;; });
  ^~~~~~~~~~~~~~~~~~~~~
13 warnings generated.

$ "c:\cmakebuilds\build\x64-debug\bin\filecheck.exe" "-strict-whitespace" "C:\llvm\tools\clang\test\AST\ast-dump-stmt.c"
# command stderr:
C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:376:18: error: CHECK-NEXT: expected string not found in input
  // CHECK-NEXT: StmtExpr 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:22> 'int'
                 ^
<stdin>:282:5: note: scanning from here
    `-StmtExpr 0x19f2d493d18 <line:375:3, col:23> 'int'
    ^
<stdin>:282:5: note: with "@LINE-1" equal to "375"
    `-StmtExpr 0x19f2d493d18 <line:375:3, col:23> 'int'
    ^
<stdin>:282:14: note: possible intended match here
    `-StmtExpr 0x19f2d493d18 <line:375:3, col:23> 'int'
             ^

error: command failed with exit status: 1

--

It looks like the column number is off by one, and I wasn't certain why. Can you look into that before I commit?

domdom updated this revision to Diff 208550.Mon, Jul 8, 5:28 PM

Thank you for catching that @aaron.ballman!

I forgot to update the test after the running clang-format. I thought I ran the tests again before submitting, but I guess I didn't :P

Thanks again!

aaron.ballman closed this revision.Tue, Jul 9, 8:02 AM

I've commit on your behalf in r365498. Thank you for the patch!