This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Fix evaluation of qualified global variables
AbandonedPublic

Authored by evgeny777 on Oct 1 2015, 8:48 AM.

Details

Reviewers
spyffe
jingham
Summary

Current revision contains bug, related to evaluation of global variables. Imagine you have the following code

int _g = 1;
int func(void) {
    int _g = 2;
    return _g; // BP here and evaluate ::_g
}

evaluation of ::_g will return 2, while the correct value is 1

Another example:

namespace test {
    int test_var = 1;
}

int test_var = 2;
int func(void) {
     using namespace test;
     return ::test_var;  // BP here and try to evaluate ::test_var
}

Evaluation will return error (multiple candidates), while correct behaviour is to return '2'

Diff Detail

Event Timeline

evgeny777 updated this revision to Diff 36249.Oct 1 2015, 8:48 AM
evgeny777 retitled this revision from to [lldb] Fix evaluation of global variables.
evgeny777 updated this object.
evgeny777 added reviewers: clayborg, paulherman.
evgeny777 added subscribers: lldb-commits, KLapshin.
labath added a subscriber: labath.Oct 1 2015, 8:57 AM

Could you please put the examples you cite above into the test suite?

evgeny777 retitled this revision from [lldb] Fix evaluation of global variables to [lldb] Fix evaluation of qualified global variables.Oct 1 2015, 9:00 AM
zturner added a subscriber: zturner.Oct 1 2015, 9:25 AM

Could you please put the examples you cite above into the test suite?

Agreed, please make a test for this.

zturner requested changes to this revision.Oct 1 2015, 9:25 AM
zturner added a reviewer: zturner.
This revision now requires changes to proceed.Oct 1 2015, 9:25 AM

Also, this patch has changes in clang. There should be a clang reviewer I think [unless I am unaware something here.]

Good point. The two patches should actually be separate, and you will need a separate set of tests for the clang patch.

Thanks for your comments. I will certainly add the test cases to lldb. I was just curious if this is correct way of fixing the issue. Besides test case what else has to be done? Separate review for changes in clang linked to this review?

I suppose you should first post a separate self contained clang patch to the clang list. After landing it in clang, you should post an LLDB specific patch here.

About its correctness, I cannot comment on the clang changes. Commenting on the LLDB changes are probably meaningful after the clang change lands.

dawn added a subscriber: dawn.Oct 1 2015, 3:19 PM

This patch fixes bugs:

See inline comments. Mostly had concerns about indentation - please always run patches through clang-format (both clang and lldb have their own config file in the root dir).

clang/include/clang/AST/DeclBase.h
1769 ↗(On Diff #36249)

Indenting in clang is 2 spaces.

lldb/include/lldb/Symbol/CompilerDeclContext.h
17 ↗(On Diff #36249)

clang includes should precede lldb ones.

lldb/include/lldb/Symbol/TypeSystem.h
121 ↗(On Diff #36249)

I don't understand this comment - please clarify? Also, please start comments with capital letters.

123 ↗(On Diff #36249)

It would be cleaner to have a default argument instead of this second overload. What are the guidelines on using default arguments in lldb? I've seen them both used and not used.

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
1370 ↗(On Diff #36249)

Looks like indentation was accidentally changed here?

lldb/source/Symbol/ClangASTContext.cpp
8984 ↗(On Diff #36249)

Please run patch through clang-format to fix indentation issues.

dawn added a comment.Oct 1 2015, 3:21 PM

Was this patch run on the entire set of lldb tests? Were there any regressions?

clayborg resigned from this revision.Oct 5 2015, 1:03 PM
clayborg removed a reviewer: clayborg.

DeclBase.h will need to be submitted against clang, not LLDB.

Then add Sean Callanan and Jim Ingham as the reviewers as they are the masters of the expression parser. I helped to get the CompilerDecl and CompilerDeclContext stuff in, but Sean and Jim should review expression parser changes.

Please add Jim Ingham and Sean Callanan as reviewers.

evgeny777 updated this revision to Diff 40497.Nov 18 2015, 5:25 AM
evgeny777 edited edge metadata.

Hi folks!

The clang patch has been landed. I've added test cases, so please look at the new patch. Thanks

evgeny777 updated this revision to Diff 40508.Nov 18 2015, 7:23 AM
evgeny777 edited edge metadata.

Minor code cleanups

evgeny777 edited reviewers, added: clayborg; removed: paulherman.Nov 19 2015, 2:47 AM
clayborg resigned from this revision.Nov 19 2015, 9:50 AM
clayborg edited reviewers, added: jingham, spyffe; removed: clayborg.

Resigning as I will let Jim Ingham and Sean Callanan review.

zturner resigned from this revision.Jun 2 2016, 8:34 PM
zturner removed a reviewer: zturner.
spyffe accepted this revision.Jul 11 2016, 11:22 AM
spyffe edited edge metadata.

Yes, we should follow the same rules as in regular lookup for these contexts. If the test suite is happy here, I'm happy.

This revision is now accepted and ready to land.Jul 11 2016, 11:22 AM

Looks like patch was not committed.

evgeny777 abandoned this revision.Sep 27 2016, 8:52 AM

This patch is longer valid after more than 9 months on review due to changes in namespace handing in ClangExpressionDeclMap. It probably should be done in a different way, but I don't have time for this now.