Page MenuHomePhabricator

Improve behavior in the case of stack exhaustion.
ClosedPublic

Authored by rsmith on Aug 16 2019, 12:56 PM.

Details

Summary

Clang performs various recursive operations (such as template instantiation),
and may use non-trivial amounts of stack space in each recursive step (for
instance, due to recursive AST walks). While we try to keep the stack space
used by such steps to a minimum and we have explicit limits on the number of
such steps we perform, it's impractical to guarantee that we won't blow out the
stack on deeply recursive template instantiations on complex ASTs, even with
only a moderately high instantiation depth limit.

The user experience in these cases is generally terrible: we crash with
no hint of what went wrong. Under this patch, we attempt to do better:

  • Detect when the stack is nearly exhausted, and produce a warning with a nice template instantiation backtrace, telling the user that we might run slowly or crash.
  • For cases where we're forced to trigger recursive template instantiation in arbitrarily-deeply-nested contexts, check whether we're nearly out of stack space and allocate a new stack (by spawning a new thread) after producing the warning.

Diff Detail

Event Timeline

rsmith created this revision.Aug 16 2019, 12:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2019, 12:56 PM
rnk added inline comments.Aug 16 2019, 2:16 PM
include/clang/Basic/Stack.h
42

For LLVM_THREADS_DISABLED builds used by Chromium today, maybe we should skip this completely?

lib/Basic/Stack.cpp
23

I think _AddressOfReturnAddress() ifdef _MSC_VER would be the proper way to ask MSVC for a reasonable stack pointer, if we're worried about returning the address of a local.

53–54

So, for example, ASan with UAR, where frames are heap allocated. I suppose in that case we will go down the __builtin_frame_address path, though.

test/SemaTemplate/stack-exhaustion.cpp
10

This test seems like it could be fragile. If threads are disabled, it will probably crash.

rsmith updated this revision to Diff 215678.Aug 16 2019, 2:33 PM
rsmith marked 2 inline comments as done.
  • Review feedback: use _AddressOfReturnAddress with MSVC, and simplify
rsmith added inline comments.Aug 16 2019, 2:42 PM
lib/Basic/Stack.cpp
53–54

Can we detect that? It seems better to turn this all off if we think there's anything funny going on with the stack layout.

test/SemaTemplate/stack-exhaustion.cpp
10

I've handled the "threads are disabled" case. Is there anything else that can meaningfully be detected here? (ASan maybe? Are there any other cases that lead to a non-linear stack layout that we can detect?)

rsmith updated this revision to Diff 215680.Aug 16 2019, 2:42 PM
  • Disable stack exhaustion test if threads are disabled.
aaron.ballman added inline comments.Aug 19 2019, 7:37 AM
include/clang/Basic/DiagnosticSemaKinds.td
14–17

Should this be a Sema warning as opposed to a Basic warning? It seems to me that we may want to guard against similar stack exhaustion from the parser as well, wouldn't we?

include/clang/Basic/Stack.h
43–47

Elide braces.

lib/Sema/SemaExpr.cpp
15070–15079

This seems unrelated to the patch?

rsmith updated this revision to Diff 216520.Aug 21 2019, 4:33 PM
rsmith marked 5 inline comments as done.
  • Address review comments from Aaron.
include/clang/Basic/DiagnosticSemaKinds.td
14–17

Sure, moved to DiagnosticCommonKinds.td. We have no uses of it outside Sema yet, but it's definitely plausible that some would be added.

lib/Sema/SemaExpr.cpp
15070–15079

I agree it seems that way, but it is related:

The block of code below that got turned into a lambda contains early exits via return for the cases that get downgraded to FormallyOdrUsed here. We used to bail out of the whole function and not mark these trivial special member functions as "used" (in the code after the NeedDefinition && !Func->getBody() condition), which seems somewhat reasonable since we don't actually need definitions of them; other parts of Clang (specifically the static analyzer) have developed a reliance on that behavior.

So this is preserving the existing behavior and (hopefully) making it more obvious what that behavior is, rather than getting that behavior by an early exit from the "need a definition?" portion of this function leaking into the semantics of the "mark used" portion.

I can split this out and make this change first if you like.

test/SemaTemplate/stack-exhaustion.cpp
10

I've read through the ASan UAR design and played with some testcases, and I think this is fine. ASan will allocate the locals for some frames on the heap instead of on the stack, but that's OK, it just might mean that the stack usage grows more slowly. The stack pointer still points to the real stack, __builtin_frame_address(0) still returns a stack pointer (and critically, not a pointer to the ASan-heap-allocated frame), and we can still detect when we're getting close to stack exhaustion -- it just happens more slowly in this mode because we have another pool of places to allocate frames that's tried before we fall back to the stack.

aaron.ballman accepted this revision.Aug 22 2019, 1:04 PM

LGTM aside from an accidental newline change.

include/clang/Basic/DiagnosticSemaKinds.td
15

Spurious removal?

lib/Sema/SemaExpr.cpp
15070–15079

I can split this out and make this change first if you like.

No need, I appreciate the explanation!

This revision is now accepted and ready to land.Aug 22 2019, 1:04 PM
Closed by commit rG26a92d5852b2: Improve behavior in the case of stack exhaustion. (authored by Richard Smith <richard-llvm@metafoo.co.uk>). · Explain WhyAug 26 2019, 11:21 AM
This revision was automatically updated to reflect the committed changes.

This change broke on NetBSD.

http://lab.llvm.org:8011/builders/netbsd-amd64/builds/22003/steps/run%20unit%20tests/logs/FAIL%3A%20Clang%3A%3Astack-exhaustion.cpp

 (view as text)

******************** TEST 'Clang :: SemaTemplate/stack-exhaustion.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/motus/netbsd8/netbsd8/build/bin/clang -cc1 -internal-isystem /data/motus/netbsd8/netbsd8/build/lib/clang/10.0.0/include -nostdsysteminc -verify /data/motus/netbsd8/netbsd8/llvm/tools/clang/test/SemaTemplate/stack-exhaustion.cpp
--
Exit Code: 139

Command Output (stderr):
--
+ : 'RUN: at line 1'
+ /home/motus/netbsd8/netbsd8/build/bin/clang -cc1 -internal-isystem /data/motus/netbsd8/netbsd8/build/lib/clang/10.0.0/include -nostdsysteminc -verify /data/motus/netbsd8/netbsd8/llvm/tools/clang/test/SemaTemplate/stack-exhaustion.cpp
Stack dump:
0.	Program arguments: /home/motus/netbsd8/netbsd8/build/bin/clang -cc1 -internal-isystem /data/motus/netbsd8/netbsd8/build/lib/clang/10.0.0/include -nostdsysteminc -verify /data/motus/netbsd8/netbsd8/llvm/tools/clang/test/SemaTemplate/stack-exhaustion.cpp 
1.	/data/motus/netbsd8/netbsd8/llvm/tools/clang/test/SemaTemplate/stack-exhaustion.cpp:9:10: current parser token ';'
#0 0x000000000242038c llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/home/motus/netbsd8/netbsd8/build/bin/clang+0x242038c)
#1 0x000000000241e5ee llvm::sys::RunSignalHandlers() (/home/motus/netbsd8/netbsd8/build/bin/clang+0x241e5ee)
#2 0x000000000241e6ee SignalHandler(int) (/home/motus/netbsd8/netbsd8/build/bin/clang+0x241e6ee)
/data/motus/netbsd8/netbsd8/build/tools/clang/test/SemaTemplate/Output/stack-exhaustion.cpp.script: line 1:  7058 Segmentation fault      (core dumped) /home/motus/netbsd8/netbsd8/build/bin/clang -cc1 -internal-isystem /data/motus/netbsd8/netbsd8/build/lib/clang/10.0.0/include -nostdsysteminc -verify /data/motus/netbsd8/netbsd8/llvm/tools/clang/test/SemaTemplate/stack-exhaustion.cpp

--

********************

Please fix.

Test disabled for NetBSD in r370801. If you're interested in investigating why this isn't working there, feel free, but this is only a best-effort mitigation for the case where things have already gone wrong, so there are limits to how much effort it makes sense to resolve this.

Does NetBSD set a hard stack rlimit of less than 8MB by any chance?

Test disabled for NetBSD in r370801. If you're interested in investigating why this isn't working there, feel free, but this is only a best-effort mitigation for the case where things have already gone wrong, so there are limits to how much effort it makes sense to resolve this.

Does NetBSD set a hard stack rlimit of less than 8MB by any chance?

The stack rlimit is restricted by default to 4MB. The maximum at least on amd64 is 128MB... but if someone really needs it, it could by bypassed with more stacks.

$ ulimit -a
time(cpu-seconds)    unlimited
file(blocks)         unlimited
coredump(blocks)     unlimited
data(kbytes)         262144
stack(kbytes)        4096
lockedmem(kbytes)    10847213
memory(kbytes)       32541640
nofiles(descriptors) 1024
processes            1024
threads              1024
vmemory(kbytes)      unlimited
sbsize(bytes)        unlimited

Should the limit by raised by default in the system?