This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Implement array initializers and subscript expressions
ClosedPublic

Authored by tbaeder on Aug 26 2022, 2:57 AM.

Details

Summary

It took me a pretty long time to get the patch this clean (and working), but here it is.

Diff Detail

Event Timeline

tbaeder created this revision.Aug 26 2022, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2022, 2:57 AM
tbaeder requested review of this revision.Aug 26 2022, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2022, 2:57 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder added inline comments.Aug 26 2022, 3:00 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
562

I've been wondering about a macro to simplify code like this, e.g. define DOIT(x) if(!x) return false; that I could instead define to assert(x) during development.

clang/test/AST/Interp/arrays.cpp
56

This is quite unfortunate, but it only fails when evaluating data[0] by itself, when printing the extra info for the failed static_assert. I roughly know what the problem is though.

tbaeder added inline comments.Aug 26 2022, 3:02 AM
clang/lib/AST/Interp/ByteCodeExprGen.h
53

Yes, a few of these cleanups, renames and added doc comments aren't necessary for this patch. I will remove them if you want to, I just don't want to rebase against main all the time to push NFC patches.

tbaeder updated this revision to Diff 455849.Aug 26 2022, 3:32 AM
tbaeder updated this revision to Diff 455873.Aug 26 2022, 4:51 AM
tbaeder added inline comments.
clang/test/AST/Interp/arrays.cpp
56

Ignore this, I fixed it.

Nothing suspicious as far as I can tell, other than just punting on ArrayFillers. Don't understand enough of this code to just do a straight-up approval though, so hopefully one of the other reviewers can take a look and confirm with another uninformed opinion :)

clang/lib/AST/Interp/ByteCodeExprGen.cpp
544

Heh, THIS is a huge "TODO" here. The ArrayFillers go through a ton of twists/turns in the current interpreter, as array-filler initializers can be massive. Do we have a way to avoid allocating space for filled-but-not-referenced values in this interpreter?

tbaeder added inline comments.Aug 26 2022, 6:44 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
544

Heh, believe me, I know ;) I'd have to look into that, but I'm pretty sure the code as it is right now doesn't handle them at all.

Precommit CI found a relevant failure:

FAIL: Clang :: AST/Interp/arrays.cpp (67 of 15858)
******************** TEST 'Clang :: AST/Interp/arrays.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   c:\ws\w5\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w5\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -fexperimental-new-constant-interpreter -verify C:\ws\w5\llvm-project\premerge-checks\clang\test\AST\Interp\arrays.cpp
: 'RUN: at line 2';   c:\ws\w5\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w5\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -verify=ref C:\ws\w5\llvm-project\premerge-checks\clang\test\AST\Interp\arrays.cpp
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "c:\ws\w5\llvm-project\premerge-checks\build\bin\clang.exe" "-cc1" "-internal-isystem" "c:\ws\w5\llvm-project\premerge-checks\build\lib\clang\16.0.0\include" "-nostdsysteminc" "-fexperimental-new-constant-interpreter" "-verify" "C:\ws\w5\llvm-project\premerge-checks\clang\test\AST\Interp\arrays.cpp"
# command stderr:
error: 'note' diagnostics expected but not seen: 
  File C:\ws\w5\llvm-project\premerge-checks\clang\test\AST\Interp\arrays.cpp Line 49: 5 == 4
1 error generated.

error: command failed with exit status: 1

--

********************
clang/lib/AST/Interp/ByteCodeExprGen.h
53

Please remove them; we want to keep changes logically separate per our developer policy. The biggest reason is: we sometimes need to revert changes and there's no reason to lose the good (unrelated) work at the same time, but also, it helps code reviewers to focus on the important parts without getting lost tracking which changes are for what reason.

clang/test/AST/Interp/arrays.cpp
10

I'd like to see a test for designated initialization of arrays, including reinitialization: https://godbolt.org/z/xfjW8xPfE

tbaeder updated this revision to Diff 456376.Aug 29 2022, 9:27 AM
tbaeder marked an inline comment as done.

Precommit CI found a relevant failure:

That needs the lvalue-to-rvalue conversion patch first.

Precommit CI found a relevant failure:

That needs the lvalue-to-rvalue conversion patch first.

Ah, good to know!

FWIW, one concern I've been pondering is building the new constant expression interpreter in such a way that we're working on higher-level constructs before having the lower-level constructs supported. I worry we're going to miss test coverage because we won't be able to add it until some later changes come in and then we'll forget to go back and add it. There's nothing specific to this review (nor actionable here), but just something to keep in mind as you're working on stuff. Implementing things like the type system (lvalue references, rvalue references, etc) first and then building other layers on top of that (like function calls, variables, etc) when plausible would ease those concerns.

clang/test/AST/Interp/arrays.cpp
41–44

A similar test we might want to add (whenever we get around to references):

template <typename T, int N>
constexpr T& getElementOf(T (&array)[N], int I) {
  return array[I];
}
static_assert(getElementOf(foo[2], 3) == &m, "");
tbaeder marked an inline comment as done.Aug 31 2022, 6:13 AM
tbaeder added inline comments.
clang/test/AST/Interp/arrays.cpp
41–44
tbaeder updated this revision to Diff 456953.Aug 31 2022, 7:13 AM
tbaeder marked an inline comment as done.
tbaeder marked an inline comment as done.Aug 31 2022, 7:14 AM
aaron.ballman added inline comments.Sep 2 2022, 11:50 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
277
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
246
256

You can drop this else because of the preceding unconditional return.

258
260

You should drop this else as well.

clang/lib/AST/Interp/Pointer.cpp
158

isArray() already covers isPrimitiveArray() so this change looks unnecessary.

tbaeder updated this revision to Diff 457767.Sep 3 2022, 12:13 AM
tbaeder marked 6 inline comments as done.
This revision is now accepted and ready to land.Sep 7 2022, 12:11 PM
This revision was landed with ongoing or failed builds.Sep 7 2022, 10:32 PM
This revision was automatically updated to reflect the committed changes.

Heads-up: after D132832 and D132727, clang/test/AST/Interp/arrays.cpp.test has a msan failure. It can be pre-existing or can be newly introduced.
There seems another issue about an integer overflow in InitMap::isInitialized.

Still investigating.

Thanks @MaskRay, I thought this was fixed by https://github.com/llvm/llvm-project/commit/86271798e51a7866dd2af44e0ee183d1331089e6, at least all the emails I got about failing msan builds were missing that change.

Thanks @MaskRay, I thought this was fixed by https://github.com/llvm/llvm-project/commit/86271798e51a7866dd2af44e0ee183d1331089e6, at least all the emails I got about failing msan builds were missing that change.

For a msan failure of llvm/clang/test/AST/Interp/arrays.cpp, I have verified that only after 49c289879c7aa9737429f7342c7149a8ba958667 it is fixed:)