Page MenuHomePhabricator

[lldb] Add support for using integral const static data members in the expression evaluator
AcceptedPublic

Authored by teemperor on Jun 9 2020, 8:10 AM.

Details

Summary

This adds support for using const static integral data members as described by C++11 [class.static.data]p3
to LLDB's expression evaluator.

So far LLDB treated these data members are normal static variables. They already work as intended when they are declared in the class definition and then defined in a namespace scope. However, if they are declared and initialised in the class definition but never defined in a namespace scope, all LLDB expressions that use them will fail to link when LLDB can't find the respective symbol for the variable.

The reason for this is that the data members which are only declared in the class are not emitted into any object file so LLDB can never resolve them. Expressions that use these variables are expected to directly use their constant value if possible. Clang can do this for us during codegen, but it requires that we add the constant value to the VarDecl we generate for these data members.

This patch implements this by:

  • parsing the constant values from the debug info and adding it to variable declarations we encounter.
  • ensuring that LLDB doesn't implicitly try to take the address of expressions that might be an lvalue that points to such a special data member.

The second change is caused by LLDB's way of storing lvalues in the expression parser. When LLDB parses an expression, it tries to keep the result around via two mechanisms:

  1. For lvalues, LLDB generates a static pointer variable and stores the address of the last expression in it: T *$__lldb_expr_result_ptr = &LastExpression
  2. For everything else, LLDB generates a static variable of the same type as the last expression and then direct initialises that variable: T $__lldb_expr_result(LastExpression)

If we try to print a special const static data member via something like expr Class::Member, then LLDB will try to take the address of this expression as it's an lvalue. This means LLDB will try to take the address of the variable which causes that Clang can't replace the use with the constant value. There isn't any good way to detect this case (as there a lot of different expressions that could yield an lvalue that points to such a data member), so this patch also changes that we only use the first way of capturing the result if the last expression does not have a type that could potentially indicate it's coming from such a special data member.

This change shouldn't break most workflows for users. The only observable side effect I could find is that the implicit persistent result variables for const int's now have their own memory address:

Before this change:

(lldb) p i
(const int) $0 = 123
(lldb) p &$0
(const int *) $1 = 0x00007ffeefbff8e8
(lldb) p &i
(const int *) $2 = 0x00007ffeefbff8e8

After this change we capture i by value so it has its own value.

(lldb) p i
(const int) $0 = 123
(lldb) p &$0
(const int *) $1 = 0x0000000100155320
(lldb) p &i
(const int *) $2 = 0x00007ffeefbff8e8

Diff Detail

Event Timeline

teemperor created this revision.Jun 9 2020, 8:10 AM
teemperor retitled this revision from [lldb] Add support for using const static data members in the expression evaluator [WIP to [lldb] Add support for using const static data members in the expression evaluator [WIP].Jun 9 2020, 8:10 AM

I will take a second look later on.

lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp
203

Should we assert that this is an lvalue?

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
7286 ↗(On Diff #269547)

So the name says AddInitToVarDecl but we are asserting only integral or enum type ... seems like the name should be more specific.

lldb/source/Utility/DataExtractor.cpp
882 ↗(On Diff #269547)

Nice refactoring!

I will take a second look later on.

Just saying that this is a WIP revision (see the title) just for the sake of linking my status report here, so this isn't actually up for review yet (and that's why that DataExtractor refactoring is in here, it's just to make my CI happy when testing this patch). You're added as reviewer because of your automatic Herald rule.

teemperor retitled this revision from [lldb] Add support for using const static data members in the expression evaluator [WIP] to [lldb] Add support for using integral const static data members in the expression evaluator.
teemperor edited the summary of this revision. (Show Details)
teemperor added reviewers: JDevlieghere, labath.
teemperor set the repository for this revision to rLLDB LLDB.
teemperor marked 2 inline comments as done.Jun 15 2020, 10:43 PM
abidh removed a subscriber: abidh.Aug 11 2020, 5:05 AM
jarin accepted this revision.Aug 11 2020, 7:01 AM

This looks great! Thank you for the extensive test suite.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2344

Out of curiosity, why can't you just use the APInt as 'result' here?

This revision is now accepted and ready to land.Aug 11 2020, 7:01 AM
teemperor added inline comments.Aug 11 2020, 8:33 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2344

You mean why use APSInt instead of APInt here? That's just because of the required_bits check below (getMinSignedBits is only in APSInt). It's not a complex function, but I rather make it an APSInt than document the MinSignedBits algorithm here.

And the required_bits check is only there because the DWARFFormValue's uint64_t value wasn't checked so far with the specific type in mind (like, having 0xabc for an unsigned char would otherwise just be silently truncated).

lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
20

Note to myself: This change and the one below were actually supposed to be its own NFC commit...

jarin added inline comments.Aug 11 2020, 9:50 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2344
teemperor planned changes to this revision.Aug 11 2020, 10:39 AM
teemperor added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2344

Ah that's a good catch, thanks! I assumed that was defined in APSInt. Let me make that just APInt then.

Very nice!

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2331

In this particular case = 64 might be more readable.

  • Revert unintended NFC cleanup in DWARFASTParserTest
  • APSInt -> APInt (Thanks Jaroslav!)
This revision is now accepted and ready to land.Aug 12 2020, 12:44 AM
teemperor updated this revision to Diff 284998.EditedAug 12 2020, 12:58 AM
  • Simplify max size to just '64' instead of sizeof(uint64_t) * 8 (thanks Adrian!)
  • Update comment to mention APInt instead of APSInt

I am curious about constexpr static as well here.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
2331

constexpr

2348

there is no APSInt anymore? right?

lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
172

Why not use int32_t although this should never affect us int can be 64 bit, so int32_t is a better reflection of your intent.

172

You can also use constexpr here since numeric_limits methods are all constexpr as well.

219

std::numeric_limits<uint64_t>::max(); or am I missing the intent here?

jarin added a comment.Aug 13 2020, 4:00 AM

I am curious about constexpr static as well here.

+1

It would be great to write tests for the static constexpr fields, too.