Differential D82160
[lldb][PDB] Constexpr static member values as AST literals Authored by jackoalan on Jun 19 2020, 1:50 AM.
Details When evaluating an expression referencing a constexpr static member variable, an error is issued because the PDB does not specify a symbol with an address that can be relocated against. Rather than attempt to resolve the variable's value within the IR execution, the values of all constants can be looked up and incorporated into the AST of the record type as a literal, mirroring the original compiler AST. This change applies to DIA and native PDB loaders.
Diff Detail
Event TimelineComment Actions You might also be interested in D81471 which is a very similar patch but for DWARF and only focused on static const integer/enum data members. One thing I'm curious about is how this is solving (or if it's even supposed to solve) the issue that the expression parser is by default taking the address of lvalues (which will bring us back to the issue that LLDB will try to resolve the symbol)? From what I understand, this patch allows me to to things like expr Class::Member + 1 but not expr Class::Member (because the expression evaluator will rewrite that lvalue to int *$__lldb_expr_result_ptr = &Class::Member).
Comment Actions Review changes from @teemperor. Base on TypeSystemClang::SetIntegerInitializerForVariable from D81471, and add associated TypeSystemClang::SetFloatingInitializerForVariable. Comment Actions Thank you @teemperor, your comments and changes in D81471 are very informative. I didn't realise DWARF had similar issues with static const members. Curiously, I am able to resolve direct variable expressions without your ASTResultSynthesizer changes. I'm guessing this is due to differences in the MS linking convention for the IR module. If you're interested, here are lldb.expr logs comparing member evaluation with and without the synthesizer change: Your changes are definitely preferred. The IR execution takes a much more roundabout approach originally, and the change noticeably speeds up evaluation.
Comment Actions Hello! Thanks for the patch, generally it looks good to me, the only thing I worry about is scoped enums (please, see the comment below).
Comment Actions The only remaining issue with the LLDB expression part is that now that SetFloatingInitializerForVariable is based on the D81471 version it no longer makes the member variable a constexpr. Int/Enum members with initialisers can just be const, but for any floating point types this ends ups creating an AST that we usually can't get from the Clang parser (as floats can't be initialised this way). Clang will still codegen that for us, but I think making the floating point VarDecls constexpr could save us from bad surprises in the future (e.g., someone adds an assert to Clang/Sema that const static data members are only initialised when they have integer/enum type). I think marking the variable as constexpr in the two places where you call SetFloatingInitializerForVariable seems like the right place for this (SetFloatingInitializerForVariable probably shouldn't set that flag on its own, that seems like unexpected behaviour from this function). Otherwise I think auto is used quite frequently and doesn't really fit to LLVM's "auto only if it makes code more readable" policy. I think all the integer types for the type widths should just be their own type (uint32_t makes the code easier to understand than auto). And I don't think LLDB usually does auto in LLDB for types/decls as LLDB has its own type/decl abstraction (CompilerType and CompilerDecl), so that's also confusing. Beside that this patch looks good to me. Maybe someone that knows more about PDB should also take a look about the PDB-specific parts of the patch.
Comment Actions
| ||||||||||||||||||||||||||||||||||||||||||||||||
What about just doing the switch directly on member_ct.GetBasicTypeEnumeration() and check for eBasicTypeFloat and so on. That's less code to do the same thing without all the casting.