Page MenuHomePhabricator

[lldb] Lookup static const members in FindGlobalVariables
Needs ReviewPublic

Authored by werat on Dec 4 2020, 2:25 AM.

Details

Summary

Static const members initialized inside a class definition might not have a corresponding DW_TAG_variable, so they're not indexed by ManualDWARFIndex.

Add an additional lookup in FindGlobalVariables. Try looking up the enclosing type (e.g. foo::bar for foo::bar::A) and then searching for a static const member (A) within this type.

Diff Detail

Event Timeline

werat requested review of this revision.Dec 4 2020, 2:25 AM
werat created this revision.
Herald added a project: Restricted Project. · View Herald Transcript
werat edited reviewers, added: labath, jarin, teemperor, jankratochvil; removed: jdoerfert.Dec 4 2020, 2:28 AM
werat added a subscriber: labath.

Hi, please, take a look at this patch!

This is an approach suggested by @labath in https://reviews.llvm.org/D92223#2422184

This approach feels a little bit "adhoc" (i.e. manually going through the DIE entries for const members), but it works :) Please, let me know if there's a better way to do this!

labath added a comment.Dec 4 2020, 2:48 AM

This approach feels a little bit "adhoc" (i.e. manually going through the DIE entries for const members), but it works :) Please, let me know if there's a better way to do this!

Are the static members included in the (SB)Type object that gets created when parsing the enclosing type? If yes, we might be able to retrieve them that way. Whether that would be cleaner -- I'm not sure...

(I would expect they are included, as otherwise they would be unavailable to the expression evaluator.)

werat added a comment.Dec 4 2020, 3:00 AM

Are the static members included in the (SB)Type object that gets created when parsing the enclosing type? If yes, we might be able to retrieve them that way. Whether that would be cleaner -- I'm not sure...

(I would expect they are included, as otherwise they would be unavailable to the expression evaluator.)

I think they're not as of now, but there's a patch from @teemperor to add them there -- D81471

Also, I was looking at Type/TypeSystem and it seems there's no API right now to get static members from a Type.

jingham added a subscriber: jingham.Dec 4 2020, 9:53 AM

Are the static members included in the (SB)Type object that gets created when parsing the enclosing type? If yes, we might be able to retrieve them that way. Whether that would be cleaner -- I'm not sure...

(I would expect they are included, as otherwise they would be unavailable to the expression evaluator.)

I think they're not as of now, but there's a patch from @teemperor to add them there -- D81471

Also, I was looking at Type/TypeSystem and it seems there's no API right now to get static members from a Type.

I couldn't tell what you meant by this... I would expect that a Type would tell you that static members exist, and their type, etc. But I wouldn't expect a Type to be able to get a value for a static member. After all, the type comes from some Module, which could be shared amongst a bunch of processes in lldb. The actual member value could be different in each of these processes, but since the Type has no process, it has no way to get the correct value.

werat added a comment.Dec 4 2020, 10:30 AM

I couldn't tell what you meant by this... I would expect that a Type would tell you that static members exist, and their type, etc. But I wouldn't expect a Type to be able to get a value for a static member. After all, the type comes from some Module, which could be shared amongst a bunch of processes in lldb. The actual member value could be different in each of these processes, but since the Type has no process, it has no way to get the correct value.

Yeah, of course you can't get a value for a static member just from the type (although, if it's an inline static then it could be possible?). But something like VariableList EnumerateStaticMembers(Target, Type) would be nice.

werat added a comment.Dec 8 2020, 6:51 AM

@labath @teemperor gentle ping :) What do you think about this approach?

It looks like the Type interface does not expose the static members (I guess it was never meant to do that), although, the underlying clang type obviously does contain them. After more consideration, I think parsing these straight from DWARF is fine, and consistent with how we parse other variables. I am wondering though if this should share the implementation with the existing parsing code (like your first patch did).

On a higher level, I am also worried about what this (the fact that we have to do this kind of a search) says about the quality/usefulness of the debug_names index. Overall, I seems to me that _not_ including these member variables in the index is the right call -- they would bloat the index (you'd have to put one of these next to every type declaration), and there are (as we've seen) other ways to find them.

I am not sure what to do about namespace-level constexpr variables, though... Currently, clang seems to put them into the index, even though that's not specified by standard. My guess is that this is by accident (it just inserts all global vars, without checking the formal inclusion rules). However, the question is what that means. If we were to consider this a bug and "fix" clang to stop emitting them, then we'd need to find a way to locate them in lldb. And doing this for namespaces won't be as easy/cheap as it is for classes. Maybe it doesn't have to block this patch (as I said, I think this part is ok), but I think it would be useful to discuss this with the people responsible for the generation of the index in clang and the wider dwarf community. (Unfortunately, I don't have the bandwidth to drive that dicussion :( ).

werat added a comment.Dec 10 2020, 4:07 AM

Thanks for your review! If there are no objections to this patch, can you accept and land it for me? :)

On a higher level, I am also worried about what this (the fact that we have to do this kind of a search) says about the quality/usefulness of the debug_names index.

FWIW we're considering disabling debug_names completely for now, as it degrades performance significantly with no extra benefit. We can discuss that in more details if you're interested.

I think it would be useful to discuss this with the people responsible for the generation of the index in clang and the wider dwarf community. (Unfortunately, I don't have the bandwidth to drive that dicussion :( ).

Can't say I have a lot of expertise in DWARF, but I will try to research this more and reach out to clang/dwarf people.

@werat I see you are aware of D81471. Do you or someone else have anything against it? I find it more integrated using the clang type for it but I admit I do not understand the clang integration much. At least it handles more cases, I stopped after finding:

$ echo 'struct Vars { static const int i = 2; } v; int main() {}'|./bin/clang -Wall -g -x c++ -;./bin/lldb -b ./a.out -o 'p Vars::i'
error: Can't evaluate the expression without a running target due to: Interpreter doesn't handle one of the expression's operands
vs. D81471's:
(const int) $0 = 2
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3159

Here should be according to my D91014:

decl.SetFile(attributes.CompileUnitAtIndex(i)->GetFile(
werat added a comment.EditedFeb 6 2021, 1:56 AM

Thanks for reviewing!

@werat I see you are aware of D81471. Do you or someone else have anything against it? I find it more integrated using the clang type for it but I admit I do not understand the clang integration much.

I don't have anything against it, on the contrary I'm all for it :) As a matter of fact, we've already integrated that patch in our internal LLDB fork a while ago.

But my patch is not a replacement for that one, it complements it by enhancing the SBTarget::FindGlobalVariables API.

werat updated this revision to Diff 322309.Feb 9 2021, 12:46 AM

Change DW_AT_decl_file handling as per @jankratochvil comment.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3159

Thanks, didn't know about that

jankratochvil added inline comments.Mar 5 2021, 1:46 PM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2162

ConstString here is needlessly expensive to construct and it is then used only once. Use plain const char * or std::string is also much cheaper.

2162

This function also needs to be patched (with a testcase) as this command works:

(lldb) target variable Vars::inline_static
(double) Vars::inline_static = 1.5

But this one does not (and it should work):

(lldb) target variable -r Vars::inline_static
error: can't find global variable 'Vars::inline_static'
2166

Could this case have a testcase so that it cannot find such false positives?

3128

Could you try to merge this functionality into SymbolFileDWARF::ParseVariableDIE? There is now some code duplication. I hope the merge will not become too ugly. (@labath was also suggesting this.)

jankratochvil requested changes to this revision.Mar 5 2021, 1:46 PM
This revision now requires changes to proceed.Mar 5 2021, 1:46 PM
werat updated this revision to Diff 330445.Mar 13 2021, 5:14 AM

Address review comments:

  • Don't create expensive ConstString objects
  • Merge ParseStaticConstMemberDIE into ParseVariableDIE
  • Add more test cases
werat marked 4 inline comments as done.Mar 13 2021, 5:26 AM
werat added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2162

With the current approach implementing a search by regexp would mean enumerating ALL types and looking at ALL their static members for a potential match. This sounds way to expensive (and a significant degradation compared to the current logic). On the other hand I agree that's very confusing that target variable works, but target variable -r doesn't. What do you think?