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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: v.g.vassilev, junaire, rsmith.
aaron.ballman added a comment.

Adding in some of the clang-repl folks because they are likely to run into this as well, and adding Richard in case he has more historical context.

What I understand of the historical context here is that we've assumed very early on that inspecting state of partially-constructed AST nodes only happens when debugging (through things like calls to `dump()`, etc). So it was assumed that having a partially constructed object which is updated as we continue parsing and semantic analysis is fine because the only way to break the invariant is from the debugger. However, over time, we've invalidated that assumption more and more (REPL, libclang, etc) and now we're paying the price.

I think that refactoring the way we construct AST nodes so that they're not in a partially-constructed state by the time we've called `Create()` is a nonstarter; I think we've got too much reliance built up on that pattern. For example, we do `CreateEmpty()` + building up state as a matter of course when deserializing the AST for PCH or modules.  However, I'm not keen on us playing whack-a-mole with the kinds of checks from this review. For starters, that's going to have a long-tail that makes it hard to know if we've ever gotten to the bottom of the issue. But also, each one of these checks is basically useless for the primary way in which Clang is consumed (as a compiler), so this increases compile times for limited benefit to "most" users. In this particular case, we may be fine as this is limited to libclang and so it shouldn't add overhead for the common path, but I suspect we're going to find cases that need fixing in more common areas of the compiler that could be more troublesome.

I wish I had a good answer for how to address this, but I don't have one off the top of my head. About the closest I can think of is to use a bit on the declaration to say "I am being worked on still" and add a `Finalize()` method to say "I am no longer being worked on" and perform sanity checks and a getter method to tell us if the AST node is still under construction or not. That gives us a generic way to quickly test "should we be inspecting this further" in circumstances where it matters and there is precedent with how we handle parsing declaration specifiers (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Sema/DeclSpec.h#L844), but this isn't all that much better than ad hoc tests like checking it the type is null or not (so I'm not suggesting this is the best way to solve the problem, just spitballing ideas).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149733