Page MenuHomePhabricator

Remove premature caching of the global variables list in CompileUnit.

Authored by aprantl on Apr 27 2018, 4:18 PM.



Remove premature caching of the global variables list in CompileUnit.
This is fixing a bug where

(lldb) target var g_ptr

would propulate the global variables list with exactly one entry because SymbolFileDWARF::ParseVariables() was invoked with a list of DIEs pre-filtered by name, such that a subsequent call to

(lldb) fr var --show-globals

would only list that one variable, because CompileUnit::m_variables was already initialized, fooling CompileUnit::GetVariableList().

Diff Detail

Event Timeline

aprantl created this revision.Apr 27 2018, 4:18 PM

This also adds test coverage for r224559, which is how I discovered this bug in the first place.

clayborg added inline comments.Apr 30 2018, 2:06 PM

So do we cache now somewhere else now that "sc.comp_unit->SetVariableList(variable_list_sp);" is removed?

aprantl added inline comments.Apr 30 2018, 2:21 PM

Yes, CompileUnit::GetVariableList() grabs the *complete* list of variables via (SymbolFileDWARF, ...)::ParseVariablesForContext and that still calls sc.comp_unit->SetVariableList(variables); which acts as the caching mechanism.

clayborg accepted this revision.Apr 30 2018, 2:29 PM

Sounds good. Looks fine.

This revision is now accepted and ready to land.Apr 30 2018, 2:29 PM
This revision was automatically updated to reflect the committed changes.
labath added a subscriber: labath.May 1 2018, 3:15 AM
labath added inline comments.
42 ↗(On Diff #144642)

this part wasn't correctly working for us on arm64, so I've split it off into a separate test, so we can still have coverage from the rest of the checks here. I think I managed to preserve the intent of your test, but you may want to have a quick look at the result (r331250).

63–65 ↗(On Diff #144642)

Why do you need to run this command twice?

aprantl added inline comments.May 1 2018, 8:39 AM
63–65 ↗(On Diff #144642)

Sorry, that's leftover debugging code. I'll remove it!