This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][NFC] Reliability fixes for IOHandlerCursesGUI
ClosedPublic

Authored by fixathon on Aug 10 2022, 1:08 PM.

Details

Summary
  • checking retval of function calls
  • signed/unsigned mismatch
  • dead code removal
  • null dereference fix

Diff Detail

Event Timeline

fixathon created this revision.Aug 10 2022, 1:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 1:08 PM
fixathon requested review of this revision.Aug 10 2022, 1:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 1:08 PM
clayborg added inline comments.Aug 10 2022, 8:59 PM
lldb/source/Core/IOHandlerCursesGUI.cpp
391–394

the underlying curses functions return "int". Were there places where people were comparing them to unsigned?

fixathon added inline comments.Aug 11 2022, 12:23 AM
lldb/source/Core/IOHandlerCursesGUI.cpp
391–394

A function HorizontalLine() at line 2582 requires non-negative input. Good point about the underlying function possibly returning a negative still.

2582

Here we have HorizontalLine() function that requires non-negative input

Responded to comments

lldb/source/Core/IOHandlerCursesGUI.cpp
391–394

I took another look, and I am not sure what the complaint is. Both the HorizontalLine() and its underlying call to ::whline take input typed as "signed int". Perhaps ::whline is unable to handle negative input?

void HorizontalLine(int n, chtype h_char = ACS_HLINE) {
  ::whline(m_window, h_char, n);
}
clayborg added inline comments.Aug 11 2022, 11:09 AM
lldb/source/Core/IOHandlerCursesGUI.cpp
391–394

Not sure either. I don't want to change these as they should return "int" like the original API.

2582

But surface.HorizontalLine(...) function takes an "int", and now we are passing it a uint32_t. If this function requires a >= 0 value, it should do the check internally

fixathon updated this revision to Diff 451957.Aug 11 2022, 1:08 PM

Address review comments about the signed/unsigned issue

clayborg accepted this revision.Aug 11 2022, 5:34 PM
This revision is now accepted and ready to land.Aug 11 2022, 5:34 PM
fixathon marked 3 inline comments as done.Aug 11 2022, 6:30 PM

Addressed the comments. Will deal with the signed/unsigned issue in a separate diff if still needed.

lldb/source/Core/IOHandlerCursesGUI.cpp
2582

I agree. I've reverted the function signatures back to signed int and will revisit the signed/unsigned issue in another diff.

This revision was automatically updated to reflect the committed changes.
fixathon marked an inline comment as done.