[Lldb-commits] [lldb] [lldb] Fix expressions that involve nested structs/classes/unions. (PR #77029)

[Greg Clayton via lldb-commits]

clayborg wrote:

> How'd this work before your recent changes, then - when each repeated query would get one level further down in the nesting? How'd that work given the clang limitations you're describing?

It was because we were actually parsing and converting many extra types. Anytime we did a type query for a given type, we would end up converting all of those types into the clang AST. So for `A::B::C`, the first time we would run this we would actually parse and convert `A` into the AST, but this would cause a bogus type query to then get sent looking for `B` at the translation unit level. But since our old type parsing code always created the types whose basename matched without looking at wether the decl context matches, it would end up parsing and converted all types whose basename matched `B` into the AST. When we incorrectly parsed the `B` type, we would and up actually putting it into `A` as the decl context, so the next time we ran an expression, `B` would already be in the definition for `A` and the expression would then work. So it was due to us parsing too many types, even if they were incorrectly scoped, but we would always create them correctly inside of the correct decl context...

> In any case, the extra clang requirements here seem like they might be of uncertain cost/maintainability (if it's only updating one place that everyone calls through - great, but if it's updating multiple callers, and the risk that new callers miss this handling - that seems like a maintainability problem) which is worrying. So, I'd be uncertain about that direction without more info - and with that info, if it is just one codepath, yeah, maybe it's quick enough to address the regression.

Yeah, it would be great if anyone with compiler experience could check and see if this is controllable and easy to fix. I was thinking we could add new APIs to the External AST that are marked as "only override these if you are a debugger"
> 
> But if it'd require substantial reengineering to clang to get this working - this seems an unfortunate regression to carry in the interim, and it might be worth reverting to the previous (differently buggy, but at least work-around-able) behavior.

I would rather have the current problem over the previous one. Are we mostly worried about template classes here? Or do all classes, even non templated ones, have this same issue?

> And if this would require updating many callers in clang (unless such updates include an API change that would make it difficult to accidentally introduce a new caller that didn't handle things correctly for lldb), I'd worry about the future stability of such a change & might be inclined towards the less efficient "search all the DWARF" thing.

I mean, in the expression parser we clearly say with `A::B`: "I am looking for something named `B` within the context of `A`" and only in the classes, structs unions or enums. Without this fix, we will end up making a global lookup for `B`, which might be ok for some things, but imagine typing `A::iterator`. If we go back to the previous bug, we will do a lookup for `A` at the translation unit level and when the auto lookup for `iterator` fails to be found within `A`, a type search for `iterator` at the translation unit level will take place.



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