[lldb] - Fix crash when listing the history with the key up.

[grimar]

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=40112,

Currently, lldb crashes after pressing the up arrow key when listing the history for expressions.

The patch fixes the mistype that was a reason.

[davide]

testcase?

[grimar]

In D56014#1339419, @davide wrote:

testcase?

I investigated how to add it and I do not see a good way write a test :(

Seems the proper place to test it would be unittest/Editline:
https://github.com/llvm-mirror/lldb/blob/master/unittests/Editline/EditlineTest.cpp

It allows to send the text using the virtual terminal:
https://github.com/llvm-mirror/lldb/blob/master/unittests/Editline/EditlineTest.cpp#L294

And I was able to simulate the key up press ("lldb-previous-line" command) sending the "\x1b[A" line
and to trigger the call of the private RecallHistory method of Editline, where this patch did the fix.
(Since we do not want to make methods public just for the test I believe)

But this is not enough. Case fixed by this patch assumes that some expression history exists.
Class responsible for work with the history is EditlineHistory:
https://github.com/llvm-mirror/lldb/blob/master/source/Host/common/Editline.cpp#L162
It contains logic to find the path for history file and logic to load/save it with the editline library API.
This class is a privately implemented in Editline.cpp and used internally by Editline.

I am not sure it is a good idea to export it for the unit test. I do not think it is correct
to duplicate its logic either or to create a history file manually, given that it seems
to contain a mini header, probably specific to the editline library version.
And also (perhaps that would not be needed, but FTR) it does not seem to be a good idea to use
the editline API in EditlineTest.cpp, I think it should remain encapsulated in Editline.cpp, like now.

Given all above it seems it is really problematic to write a test for this now.
My suggestion probably is to go without it (given that patch fixes an obvious mistype).

[labath]

I guess it should be possible to trigger this from dotest tests via pexpect. I know we try to avoid it, but given that this is specifically testing terminal interaction, using it here seems appropriate.

[grimar]

In D56014#1341129, @labath wrote:

I guess it should be possible to trigger this from dotest tests via pexpect. I know we try to avoid it, but given that this is specifically testing terminal interaction, using it here seems appropriate.

Thanks for the suggestion. I'll try to investigate it and return with the results during the next week I guess.

[grimar]

I ended up with the following test case:

TestHistoryCrash.py:

"""We crashed when navigated through the history. Check we do not now."""

from __future__ import print_function

import os
import sys
import lldb
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
from lldbsuite.test import configuration
from lldbsuite.test import lldbutil

class TestNoCrashForHistoryRecall(TestBase):

    mydir = TestBase.compute_mydir(__file__)

    # If your test case doesn't stress debug info, the
    # set this to true.  That way it won't be run once for
    # each debug info format.
    NO_DEBUG_INFO_TESTCASE = True

    def setUp(self):
        TestBase.setUp(self)

    @expectedFailureAll(
        oslist=["windows"],
        bugnumber="llvm.org/pr22274: need a pexpect replacement for windows")

    def test_history_recall(self):
        self.child_prompt = '(lldb) '
        self.sample_test()

    def sample_test(self):
        import pexpect

        self.child = pexpect.spawn(
            '%s %s' %
            (lldbtest_config.lldbExec, self.lldbOption))
        child = self.child
        child.sendline('print')
        child.expect_exact('Enter expressions')
        child.sendline('\x1b[A')
        child.close()

        self.assertEqual(child.exitstatus, 0)

It works, but not in all cases. Problem is that history patch is hardcoded and history is stored in the home directory (~/.lldb/lldb-expr-history):
https://github.com/llvm-mirror/lldb/blob/master/source/Host/common/Editline.cpp#L180

When the history file exists and is empty (has a header, but no history lines), the test case can trigger the crash successfully.
But if the history file does not yet exist, or if it is not empty, it does not work (always pass), because does not trigger a line changed.
And in the ideal world on a build bot, I believe for a clean build it should never exist (though I guess bots are not cleaning the ~/.lldb folder perhaps).

It is not a good idea to touch or modify files outside the build folder from the test, so that does not seem what I can change.
And so I am not sure it is worth to add such test, it can help only in some cases, for a local test runs, for example.
Also, It does not work on windows because all pexpect tests seem ban windows.

Given above I suggest going without a test, perhaps.

[JDevlieghere]

Although I don't want to condone checking things in without a test case, given that the fix is obvious and that testing it is non-trivial, I think it's fine to check this in as is.

[grimar]

In D56014#1358769, @JDevlieghere wrote:

Although I don't want to condone checking things in without a test case, given that the fix is obvious and that testing it is non-trivial, I think it's fine to check this in as is.

Thanks, Jonas!

lldb/trunk/source/Host/common/Editline.cpp

if (!m_in_history) {
     // Save any edits to the "live" entry in case we return by moving forward
     // in history (it would be more bash-like to save over any current entry,
     // but libedit doesn't offer the ability to add entries anywhere except the
     // end.)
     SaveEditedLine();
     m_live_history_lines = m_input_lines;
     m_in_history = true;
   } else {
-    if (history_w(pHistory, &history_event, earlier ? H_NEXT : H_PREV) == -1) {
+    if (history_w(pHistory, &history_event, earlier ? H_PREV : H_NEXT) == -1) {
       // Can't move earlier than the earliest entry
       if (earlier)
         return CC_ERROR;
 
       // ... but moving to newer than the newest yields the "live" entry
       new_input_lines = m_live_history_lines;
       m_in_history = false;
     }