This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Add frame depth checking
ClosedPublic

Authored by tbaeder on Apr 18 2023, 1:36 AM.

Details

Summary

There was already some form of this, but it wasn't working. I moved the depth tracking from InterpState to InterpFrame so remove some of the magic from the Ret instructions.

Diff Detail

Event Timeline

tbaeder created this revision.Apr 18 2023, 1:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2023, 1:36 AM
tbaeder requested review of this revision.Apr 18 2023, 1:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2023, 1:36 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 514826.Apr 18 2023, 11:12 PM

Move things around a bit and add some + 1s to get matching diagnostic output with the current interpreter.

The changes make sense, I am sure about any trade-offs of doing the checking during the call Vs doing at return.

aaron.ballman added inline comments.May 16 2023, 10:32 AM
clang/lib/AST/Interp/Interp.cpp
345–352

-fconstexpr-depth sets the number of *recursive* constexpr calls, but this looks like it's measuring the depth of the call stack regardless of whether there's recursion or not. Can you add a test where you set this value to something low and make nested calls that exceed that depth?

(Also, there's -fconstexpr-steps https://clang.llvm.org/docs/UsersManual.html#cmdoption-fconstexpr-steps we'll need to support at some point, in case you weren't aware of the option.)

aaron.ballman accepted this revision.May 16 2023, 10:37 AM

LGTM, though I would appreciate adding the other test case from my comments since it's interesting behavior.

clang/lib/AST/Interp/Interp.cpp
345–352

Whelp, TIL that our docs are wrong and should be updated (I'll take care of that): https://godbolt.org/z/ahPjPnhGr

It has nothing to do with recursion, that's just the way in which you'd typically run into it.

This revision is now accepted and ready to land.May 16 2023, 10:37 AM
tbaeder added inline comments.May 30 2023, 3:04 AM
clang/lib/AST/Interp/Interp.cpp
345–352

I know about -fconstexpr-steps, but the notion of "steps" is implementation dependent, isn't it? I.e. I could just cal a "step" the execution of one opcode?

tbaeder updated this revision to Diff 526565.May 30 2023, 3:18 AM
tbaeder marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.