[clang] [Serialization] Check for stack exhaustion when reading declarations (PR #79875)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> with stack exhaustion warning, the compiler can continue (albeit being slow or unstable). The depth limit here will be a hard restriction and so there will be no workaround if the code reaches it.

It is a surprise to me that this is only a warning instead of a hard error. I never thought it can continue when the stack runs out. Sorry for confusing.

> I am curious about performance concerns. I feel that the amount of work in ASTReaderDecl is large enough that this extra call should not add any noticeable overhead on a happy path (when we have enough stack). Or do you think the overhead might be problematic?

In my imagination, with `ASTReader::NumCurrentElementsDeserializing? will only require us to insert a compare between intergers (the value of `NumCurrentElementsDeserializing` and the threshold) on the hot path. So it will be simpler than the current method. But given you said above, maybe it doesn't matter so much. I believe the overhead is under control.

> Except the error should ideally be happening when we write PCM, but that seems hard

What's the meaning? Isn't this patch about reading?

> Just to make sure I got everything right this time. So you mean testing the clang::runWithSufficientStackSpace itself? That seems very reasonable. If we're talking about unit-testing the ASTReaderDecl, this seems like it's quite a bit of work (too much setup code that needs to be put around it).

I mean something like:

First, refactor the signature of `clang::runWithSufficientStackSpace` to:


```
namespace clang {
    clang::runWithSufficientStackSpace(function_call_back_for_diagnostic, function_to_run, desired_stack_size = default_value, sufficient_stack_size = default_value);
}
```

then add two setters to ASTReader

```
class ASTReader {
    std::optional<size_t> desired_stack_size;
    std::optional<size_t> sufficient_stack_size;
public:
     void set_desired_stack_size(size_t) {...}
     void set_sufficient_stack_size(size_t) {...}
}
```

then in the unittest under `clang/unittests/Serialization`, we can generate the BMI for the codes in your current patch (but with smaller scale) like other unit test did.

And we can try to parse the `use.cpp` in the current patch, we need to construct the CompilerInstance and get the ASTReader and set the corresponding value for desired_stack_size and sufficient_stack_size. Since we're calling `clang::runWithSufficientStackSpace` with a call back to the diagnostic function, it is easy to test the diagnostic function is called or it shouldn't be hard to test the actual diagnostic message.

This is the code in my mind. I don't feel it would be pretty hard. How do you think about it?



https://github.com/llvm/llvm-project/pull/79875