Page MenuHomePhabricator

[lldb][PDB] Constexpr static member values as AST literals
ClosedPublic

Authored by jackoalan on Jun 19 2020, 1:50 AM.

Details

Summary

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 Timeline

jackoalan created this revision.Jun 19 2020, 1:50 AM

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).

lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
155

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.

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
7321

Passing a too large APSInt into this function will just silently truncate it. I think this should refuse to add the initializer in this case (and we probably should at least log that error somewhere).

lldb/test/Shell/SymbolFile/PDB/Inputs/AstRestoreTest.cpp
27

You might want to also add some static const (not constexpr) integer and enum members to this test.

Also I'm curious what happens if you put a long double as a member? From what I can see the expected behavior is that PDB will not emit that as a constant (as the current code asserts if it encounters long double anywhere).

Review changes from @teemperor.

Base on TypeSystemClang::SetIntegerInitializerForVariable from D81471, and add associated TypeSystemClang::SetFloatingInitializerForVariable.

jackoalan marked 4 inline comments as done.Jun 20 2020, 11:48 PM

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:
without: https://gist.github.com/jackoalan/f70f223c5e9ec25d6c8e3a5e09940a71
with: https://gist.github.com/jackoalan/e33c878c545c5b270e6f3da44ef57320

Your changes are definitely preferred. The IR execution takes a much more roundabout approach originally, and the change noticeably speeds up evaluation.

lldb/test/Shell/SymbolFile/PDB/Inputs/AstRestoreTest.cpp
27

long double is aliased to double according to MS docs. The constant can be extracted in the same manner.

jackoalan marked an inline comment as done.

Fix ternary to use corresponding float/double overloaded constructors of APFloat

jackoalan updated this revision to Diff 272297.Jun 21 2020, 1:17 AM

Apply formatting fixes

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).

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
7324–7331

I'm not sure, can we initialize a member this way if it has a scoped enum type?

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.

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
7324–7331

(That code is from D81471 so I think I should answer that)

Well, it works and but it relies on CodeGen/Sema not double-checking that we get the enum type (and not the underlying int type). I can inject a CXXStaticCastExpr too and will update D81471. Thanks!

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
7324–7331

Ok, thank you!

jackoalan updated this revision to Diff 272597.EditedJun 22 2020, 8:57 PM
  • Added a test for scoped enums (works as-is but still worth testing).
  • Less frivolous use of auto
  • Made the floating point vars constexpr to maintain validity in clang's internals.
  • AstRestoreTest CLASS tests run for both DIA and Native. A pure NativePDB test that does not depend on MSVC would require extending s_constant.cpp (S_CONSTANT not generated by clang-cl) but I'm not certain how to regenerate the listing file with comments (unless that's done by hand). For now, this change should only be considered relevant for PDBs generated by MSVC.
jackoalan marked 4 inline comments as done.Jun 22 2020, 9:00 PM
jackoalan added inline comments.
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
7324–7331

Added a test just in case

aleksandr.urakov accepted this revision.Jun 23 2020, 7:25 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jun 23 2020, 7:25 AM

Thank you! I do not have commit access, so I will need some help with that.

This revision was automatically updated to reflect the committed changes.