This is an archive of the discontinued LLVM Phabricator instance.

Fix the variable printing for stack-list-local and stack-list-arguments.
ClosedPublic

Authored by abidh on Feb 12 2015, 7:09 AM.

Details

Reviewers
ki.stfu
Summary

Fix for http://llvm.org/bugs/show_bug.cgi?id=21744

GetVariableInfo () collected the values of the variable in a list. But it also
tried to generate the name/value pairs for children. This caused generation of
a wrong value string for may items. This function has been fixed to put value in
the list only.

The handling of --print-value related option has been moved to caller.

GetVariableInfo2 and MIResponseFormVariableInfo3 have been removed. They were
almost the duplicate of functions of similar names. I dont see any difference in
the output of -stack-list-locals and -stack-list-arguments. So these functions
just seemed unnecessary.

Char variable was being printed as a string which caused garbage output. This has
been fixed.

Some misc. cleanup.

Test cases have been added that check -stack-list-locals for struct, array and
pointers. Modified other tests which depended on hard coded line numbers.

TODO: Those hardcoded line numbers should be removed.

Diff Detail

Event Timeline

abidh updated this revision to Diff 19829.Feb 12 2015, 7:09 AM
abidh retitled this revision from to Fix the variable printing for stack-list-local and stack-list-arguments..
abidh updated this object.
abidh edited the test plan for this revision. (Show Details)
abidh added a reviewer: ki.stfu.
abidh added a subscriber: Unknown Object (MLST).
ki.stfu edited edge metadata.Feb 12 2015, 7:57 AM

Looks good. I dislike these func/func2/func3/func4 (and so on) functions. One question for you: --simple-values works now?

FYI: I didn't check it on OS X yet. I'll do it later.

tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
729

Is it enough? Should we unescape this string?

733–734

valueStr = CMIUtilString::Format("{%s}", valueStr.c_str());

738

Maybe would better to name it like miValueResultName? I know that lldb-mi uses sequential naming standard for miValueConst/miValueResult values, but in your case it was created on 20 lines above.

762

vrwnDepth must be a reference? I think it can cause a problem on large objects. For example, on line #844 you increment it in cycle (which serves to obtain a value of complex object). But on every iteration we will call GetVariableInfo with vrwnDepth which grows, but actually these children have one depth on call stack.

RE: TODO: Those hardcoded line numbers should be removed.

I think we should leave them to check *stopped notification. We can move it to separate test file to avoid modification of it when we are improving the tests.

abidh added a comment.Feb 12 2015, 8:22 AM

Please see my comments inline. Also it will be good if you can check that test pass on OSX.

RE: TODO: Those hardcoded line numbers should be removed.

I think we should leave them to check *stopped notification. We can move it to separate test file to avoid modification of it when we are improving the tests.

Yes that will not be part of this revision. Just wanted to draw the attention. I was thinking that we can use the line_number () like function to get the line to check instead of hard coding them. But it can be done later.

tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
729

I will check.

733–734

I will change it. I dont like the unnecessary string copying that goes on in lldb-mi. Something to fix in future.

738

ok. I will change it. You may have already guesssed but it is done to have output like
[name="var"] when there is no value field.

If it is added in the tuple first then we will get
[{name="var"}]
which I dont think is wrong but I wanted to be as close to what gdb does.

762

You are right. I think this should be decremented after function return to have the right depth counter. Current implementation will fail if the child struct have more then 10 members.

abidh updated this revision to Diff 19839.Feb 12 2015, 9:43 AM
abidh edited edge metadata.

Handled the comments from review.

  1. Use correct constructor of the CMICmnMIValueConst. Removed Trim Call.
  2. Used Format function to format a string.
  3. Renamed miValueResul to miValueResultName.
  4. Decremented the vrwnDepth after function return.

I have one more issue (sorry, forgot to say immediately about it):

tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
760–762
bool
CMICmnLLDBDebugSessionInfo::GetVariableInfo(const MIuint vnMaxDepth, const lldb::SBValue &vrValue, const bool vbIsChildValue,
                         const MIuint vnDepth, CMICmnMIValueList &vwrMiValueList)
850–852
bOk = GetVariableInfo(vnMaxDepth, member, true, vnDepth + 1, miValueList2);
tools/lldb-mi/MICmnLLDBDebugSessionInfo.h
203

I'd prefer "const MIuint vnDepth". And it will be great if you make the vnDepth 4th argument (and move vwrMiValueList to the end).

bool GetVariableInfo(const MIuint vnMaxDepth, const lldb::SBValue &vrValue, const bool vbIsChildValue,
                         const MIuint vnDepth, CMICmnMIValueList &vwrMiValueList);
ki.stfu requested changes to this revision.Feb 13 2015, 2:55 AM
ki.stfu edited edge metadata.
This revision now requires changes to proceed.Feb 13 2015, 2:55 AM

MiExecTestCase.test_lldbmi_exec_step_instruction failed:

======================================================================
ERROR: test_lldbmi_exec_step_instruction (TestMiExec.MiExecTestCase)
    Test that 'lldb-mi --interpreter' works for instruction stepping into.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/IliaK/p/llvm/tools/lldb/test/lldbtest.py", line 422, in wrapper
    return func(self, *args, **kwargs)
  File "/Users/IliaK/p/llvm/tools/lldb/test/lldbtest.py", line 537, in wrapper
    func(*args, **kwargs)
  File "/Users/IliaK/p/llvm/tools/lldb/test/tools/lldb-mi/TestMiExec.py", line 333, in test_lldbmi_exec_step_instruction
    self.expect("\*stopped,reason=\"end-stepping-range\".*main.c\",line=\"22\"")
  File "/Users/IliaK/p/llvm/tools/lldb/test/tools/lldb-mi/lldbmi_testcase.py", line 43, in expect
    return self.child.expect(pattern, *args, **kwargs)
  File "/Users/IliaK/p/llvm/tools/lldb/test/pexpect-2.4/pexpect.py", line 1316, in expect
    return self.expect_list(compiled_pattern_list, timeout, searchwindowsize)
  File "/Users/IliaK/p/llvm/tools/lldb/test/pexpect-2.4/pexpect.py", line 1330, in expect_list
    return self.expect_loop(searcher_re(pattern_list), timeout, searchwindowsize)
  File "/Users/IliaK/p/llvm/tools/lldb/test/pexpect-2.4/pexpect.py", line 1414, in expect_loop
    raise TIMEOUT (str(e) + '\n' + str(self))
TIMEOUT: Timeout exceeded in read_nonblocking().
<pexpect.spawn object at 0x10f3868d0>
version: 2.4 ($Revision: 516 $)
command: /Users/IliaK/p/llvm/build_ninja/bin/lldb-mi
args: ['/Users/IliaK/p/llvm/build_ninja/bin/lldb-mi', '--interpreter']
searcher: searcher_re:
    0: re.compile("\*stopped,reason="end-stepping-range".*main.c",line="22"")
buffer (last 100 chars): p/llvm/tools/lldb/test/tools/lldb-mi/main.c",line="24"},thread-id="1",stopped-threads="all"
(gdb)
test/tools/lldb-mi/TestMiExec.py
333

on OS X it should be:

self.expect("\*stopped,reason=\"end-stepping-range\".*main.c\",line=\"24\"")
339

on OS X it should be:

self.expect("\*stopped,reason=\"end-stepping-range\".*main.c\",line=\"24\"")
abidh updated this revision to Diff 19879.Feb 13 2015, 3:27 AM
abidh edited edge metadata.

Fixed some cosmetics changes reported in reviews.
Adjusted line numbers to take care of failures on OSX.

ki.stfu accepted this revision.Feb 13 2015, 3:42 AM
ki.stfu edited edge metadata.

lgth. All tests passed on OS x.

This revision is now accepted and ready to land.Feb 13 2015, 3:42 AM
abidh closed this revision.Feb 13 2015, 3:52 AM