[PATCH] D142401: [Clang] Fix a crash when recursively callig a default member initializer.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D142401#4096498 <https://reviews.llvm.org/D142401#4096498>, @erichkeane wrote:

> In D142401#4096478 <https://reviews.llvm.org/D142401#4096478>, @aaron.ballman wrote:
>
>> In D142401#4082356 <https://reviews.llvm.org/D142401#4082356>, @cor3ntin wrote:
>>
>>> In D142401#4074959 <https://reviews.llvm.org/D142401#4074959>, @shafik wrote:
>>>
>>>> I could not find any tests that test for this warning.
>>>
>>> I'm unable to trigger the warning at all. I feel i should but just using that function resolves the crash, probably because it allocates more stack in a separate thread, and there is actually some code that avoids marking the same object used twice.
>>> But the warning might still trigger on some platform, so i suspect the test i added might, sometimes, trigger a warning. I'm not exactly sure how to handle that
>>
>> Tests for stack space are always really challenging because triggering the failure is so system specific. We only have one test for it (`test/SemaTemplate/stack-exhaustion.cpp`)
>>
>> But do we need to run these on separate threads in all of those places to address the issue?
>>
>> "and there is actually some code that avoids marking the same object used twice." makes me wonder if we're missing an early return somewhere that would reduce stack usage more and avoid needing to spin up threads for this. (The upside to this change is that the code compiles more often but the downside is that we're not addressing the memory pressure issue, just kicking the can down the road a bit.)
>
> I agree for the most part, but the 'runWithSufficientStackSpace' doesn't run in a separate thread, it just wraps it in a function that makes our diagnostic for running out of memory better.  But I think you're right, we probably should have SOMETHING that lets us exit early or defer constructing the initializers or something.

That's not entirely accurate: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Stack.h#L40 which calls into https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Stack.cpp#L66

It MIGHT get run in a new thread (if that's enabled) or it might not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142401/new/

https://reviews.llvm.org/D142401