[PATCH] D149733: [clang][USR] Prevent crashes on incomplete FunctionDecls

Mon May 15 07:10:15 PDT 2023

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D149733#4342095 <https://reviews.llvm.org/D149733#4342095>, @kadircet wrote:

>> I think we probably should have a broader discussion before moving forward here. It's not that this isn't incremental progress fixing an issue, but it's more that this same justification works to add the workaround 200 more times without ever addressing the underlying architectural concerns.
>
> I can see the desire to fix such issues at a higher level, and not wanting to play whack-a-mole in various places. But that's the reason why this patch specifically increases robustness in a piece of code that already has some logic to deal with invalid code. Rather than core clang pieces, which has different assumptions about invariants of the AST (e.g. not adding the null-check to every getType call in FunctionDecl methods).
> If we're not willing to land such fixes that only add relevant checks to pieces of clang that's suppose to be more robust against invalid code, I don't see how we can have any stable tooling in the short term. Most of the feature work that introduces handling for new language constructs introduces regressions on invalid code as it's not really a concern and never verified. Hence we fix those afterwards as we notice them, fixing the implementation whenever it's feasible or increasing robustness in rest of the pieces when fixing the implementation is infeasible (or implementation is just right and the application was relying on some assurances that were incidentally there before). As we happen to hit some increasing resistance towards these robustness improvement patches, it'd be nice to understand what should be the way to fix them going forward. e.g. treat them as usual crashes, find the offending patch and just revert it with the reproducer?

On the one hand, I agree with you about the fact that this code is already trying to be robust against invalid code and thus it's reasonable to add more checks for that. No argument from me about that! But at the same time, it's become more obvious (at least to me) that clangd has features that don't work with all of the invariants in Clang and I don't know that we ever really stopped to figure out whether that's reasonable or not. That's why I think we need a broader discussion. The problem is not ideological, it's one of maintainability of the primary product. For example, the community could decide "it is not Clang's job to be resilient to this sort of thing". Or we could decide "we need to be resilient to this but only if it doesn't introduce more than X% overhead". And so on. Each time we land another one of these whack-a-mole changes, we potentially make it harder to get to a more principled approach.

(My personal feelings are that invariants about internal object state are going to be hard for us to change or introduce unwarranted overhead in at least some circumstances. In those circumstances, I think the onus is on clangd to determine how to work within those invariants and not on clang to change unless there isn't another reasonable option. Refactoring or adding smoke tests can introduce overhead that typical compilation scenarios should never have to pay the cost for, and we should avoid that as best we can. But I also realize this adds burden to the clangd folks to have compiler performance statistics for changes or refactoring that relate to invariants which may or may not be reasonable.)

>> That said, is this issue blocking further work so you need to land this in order to make forward progress elsewhere?
>
> This is resulting in crashes in clangd whenever the users trigger code completion in such code patterns (which is ~100s times a day in our production setup, so not rare in practice). 
> So it isn't linked to any bigger task, but rather general robustness/QoL improvements for invalid code handling in clang(d) which are quite important especially on big projects. As a crash could cost minutes of warm-up times because we need to rebuild all of the caches.

The fact that this is happening 100s of times a day for you suggests we should land the changes in this patch so there's less pressure when having the broader discussion about where the division of labor is between clangd and clang. So LGTM!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149733