Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[lldb] Rewrite Swift completion logic in REPL to not crash [WIP] #662

Closed
wants to merge 4 commits into from
Closed

[lldb] Rewrite Swift completion logic in REPL to not crash [WIP] #662

wants to merge 4 commits into from

Conversation

Teemperor
Copy link

This fixes the crashes in the Swift REPL.

Fixes rdar://58854651

@Teemperor Teemperor requested a review from dcci January 27, 2020 19:39
@Teemperor Teemperor force-pushed the FixCompletionSwift branch 2 times, most recently from 3cef7d5 to cfbb899 Compare January 27, 2020 20:32
dcci
dcci reviewed Jan 27, 2020
View reviewed changes
lldb/include/lldb/Utility/CompletionRequest.h
@@ -104,7 +104,7 @@ class CompletionRequest {

llvm::StringRef GetRawLine() const { return m_command; }
llvm::StringRef GetRawLineUntilCursor() const {
return m_command.substr(0, m_cursor_index);
return m_command.substr(0, m_raw_cursor_pos);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a way to test this change? Is it tested as part of your other commits?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two of the commits here are not Swift-specific so I'll make a separate PR for them to the stable branch once this passes the test.

Copy link
Author

@Teemperor Teemperor Jan 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, somehow my commented ended up in this thread.... I just pushed a unit test for this change too.

@Teemperor
Copy link
Author

@swift-ci test

@Teemperor
Copy link
Author

@swift-ci test

@Teemperor
Copy link
Author

Seems like the REPL fails to start as it doesn't reach the start prompt >1 and just gets a EOF. The test works fine locally so I wonder what's going on there...

@Teemperor
Copy link
Author

@swift-ci test

labath and others added 3 commits January 28, 2020 13:29
@labath @Teemperor
[lldb/pexpect] Force-set the TERM environment variable
bd31559 
In some environments (typically, buildbots), this variable may not be
available. This can cause tests to behave differently.

Explicitly set the variable to "vt100" to ensure consistent test
behavior. It should not matter that we do not inherit the process TERM
variable, as the child process runs in a new virtual terminal anyway.
@Teemperor
[lldb] Fix that CompletionRequestGetRawLineUntilCursor returns the wr…
0f32e4d 
…ong result

By using m_cursor_index and not m_raw_cursor_pos in this function
we return the characters until the argument index (not the character index).

This means that for "foo" (cursor at the end of the string)
this returns "" instead of "foo" (as the argument index is 0 for the
first argument and not 4 which is the character index at the end).

This lead to a crash in the REPL completion where it would cause that
we would complete the string "ABC" to "     " as we think the line
is actually empty and we want to provide the indentation string.

Note that the function has already been deleted upstream so this bug
doesn't exist there.
@Teemperor
[lldb] Move REPL::CompleteCode interface to use CompletionRequest
b8ea404
@Teemperor
Copy link
Author

@swift-ci test

@Teemperor
[lldb] Rewrite Swift REPL completion to use CompletionRequest
28f284d 
This removes all the workaround and problematic code that was needed
for the old interface. With the CompletionRequest we no longer need
to calculate the common prefix ourselves and we don't need to calculate
the magic return value. On the other hand we can no longer provide
'appendix' completions (E.g. completing "Suff" with "ix", instead
we now need to provide the completion "Suffix"), so in one case
we now need to prefix the completion with the existing prefix from
the CompletionRequest (as Swift only returns the string to append
and not the whole token).

Also rewrites the TestSwiftREPLCompletion.py to test all the
problematic cases and some more.
@Teemperor
Copy link
Author

@swift-ci test

dcci
dcci approved these changes Jan 28, 2020
View reviewed changes
Copy link
Member

@dcci dcci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Thanks for taking care of it.

@dcci
Copy link
Member

dcci commented Jan 28, 2020

[We should try to get this to 4.2]

@dcci
Copy link
Member

dcci commented Jan 28, 2020

I mean, 5.2

@Teemperor
Copy link
Author

Split up in #678 and #679 (and made that against 5.2).

@marcrasi
Copy link

marcrasi commented Feb 13, 2020 •
edited
Loading

Do you still plan to merge this PR into swift/master? I'm asking because my REPL built at swift/master crashes when completing things and I wonder if this will fix it. e.g.:

marcrasi@marcrasi:~/swift-base-master/build/Ninja-ReleaseAssert/lldb-linux-x86_64$ bin/lldb -r
Welcome to Swift version 5.2-dev (LLVM 3bf86cf22b, Swift f4cef5715a).
Type :help for assistance.
  1> prin<TAB>

causes:

terminate called after throwing an instance of 'std::out_of_range'
  what():  basic_string::substr: __pos (which is 4) > this->size() (which is 2)
 #0 0x0000000000421ec4 PrintStackTraceSignalHandler(void*) (bin/lldb+0x421ec4)
 #1 0x000000000041ff5e llvm::sys::RunSignalHandlers() (bin/lldb+0x41ff5e)
 #2 0x000000000042244c SignalHandler(int) (bin/lldb+0x42244c)
 #3 0x00007fe2113e1520 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x13520)
 #4 0x00007fe208727081 raise (/lib/x86_64-linux-gnu/libc.so.6+0x3a081)
 #5 0x00007fe208712535 abort (/lib/x86_64-linux-gnu/libc.so.6+0x25535)
 #6 0x00007fe208961643 (/lib/x86_64-linux-gnu/libstdc++.so.6+0x9a643)
 #7 0x00007fe20896d066 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xa6066)
 #8 0x00007fe20896d0b1 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xa60b1)
 #9 0x00007fe20896d2e5 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xa62e5)
#10 0x00007fe208963e0d (/lib/x86_64-linux-gnu/libstdc++.so.6+0x9ce0d)
#11 0x00007fe209972329 lldb_private::Editline::TabCommand(int) (/usr/local/google/home/marcrasi/swift-base-master/build/Ninja-ReleaseAssert/lldb-linux-x86_64/bin/../lib/liblldb.so.7git+0xd2e329)
#12 0x00007fe209975039 lldb_private::Editline::ConfigureEditor(bool)::$_14::__invoke(editline*, int) (/usr/local/google/home/marcrasi/swift-base-master/build/Ninja-ReleaseAssert/lldb-linux-x86_64/bin/../lib/liblldb.so.7git+0xd31039)
#13 0x00007fe2082fe792 el_wgets (/lib/x86_64-linux-gnu/libedit.so.2+0x10792)
#14 0x00007fe209974842 lldb_private::Editline::GetLines(int, lldb_private::StringList&, bool&) (/usr/local/google/home/marcrasi/swift-base-master/build/Ninja-ReleaseAssert/lldb-linux-x86_64/bin/../lib/liblldb.so.7git+0xd30842)
#15 0x00007fe2098e7324 lldb_private::IOHandlerEditline::Run() (/usr/local/google/home/marcrasi/swift-base-master/build/Ninja-ReleaseAssert/lldb-linux-x86_64/bin/../lib/liblldb.so.7git+0xca3324)
#16 0x00007fe2098cbb56 lldb_private::Debugger::ExecuteIOHandlers() (/usr/local/google/home/marcrasi/swift-base-master/build/Ninja-ReleaseAssert/lldb-linux-x86_64/bin/../lib/liblldb.so.7git+0xc87b56)
#17 0x00007fe2098cfb7f lldb_private::Debugger::IOHandlerThread(void*) (/usr/local/google/home/marcrasi/swift-base-master/build/Ninja-ReleaseAssert/lldb-linux-x86_64/bin/../lib/liblldb.so.7git+0xc8bb7f)
#18 0x00007fe20997e062 lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(void*) (/usr/local/google/home/marcrasi/swift-base-master/build/Ninja-ReleaseAssert/lldb-linux-x86_64/bin/../lib/liblldb.so.7git+0xd3a062)
#19 0x00007fe2113d6fb7 start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8fb7)
#20 0x00007fe2087e72df clone (/lib/x86_64-linux-gnu/libc.so.6+0xfa2df)
********************

edit: Indeed, I patched this in and it fixes that crash.

@Teemperor
Copy link
Author

Ah, yeah, this patch landed in 5.2, I need to do the same PR for all the other branches I guess. Thanks for the hint

@Teemperor
Copy link
Author

@marcrasi it's on master now, thanks!

@Teemperor Teemperor closed this Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcci dcci dcci approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Teemperor @dcci @marcrasi @labath