Convert CommandObjectCommands functions to return StringRefs

[shivammittal99]

I don't know why is clang-tidy failing. I haven't changed the import or any other file than CommandObjectCommands.cpp. @jingham @aprantl Could you please help?

[jankratochvil]

Have you run the build with address sanitizer?

lldb/source/Commands/CommandObjectCommands.cpp

529–530

The raw_command_string definition does not need to be moved above the ParseOptionsAndNotify call, it could stay in place lower.

1173

I see no need for the conditional there.

1175

I see no need for the conditional there.

[shivammittal99]

@jankratochvil When I use LLVM_USE_SANITIZER=Address and then run ninja check-lldb I get other errors:

$ ninja check-lldb -j 1
[1/1981] Building DiagnosticDriverKinds.inc...
FAILED: tools/clang/include/clang/Basic/DiagnosticDriverKinds.inc 
cd /home/shivam/Desktop/projects/github/llvm-project/build && /home/shivam/Desktop/projects/github/llvm-project/build/bin/clang-tblgen -gen-clang-diags-defs -clang-component=Driver -I /home/shivam/Desktop/projects/github/llvm-project/clang/include -I /home/shivam/Desktop/projects/github/llvm-project/clang/include/clang/Basic -I /home/shivam/Desktop/projects/github/llvm-project/llvm/include /home/shivam/Desktop/projects/github/llvm-project/clang/include/clang/Basic/Diagnostic.td --write-if-changed -o tools/clang/include/clang/Basic/DiagnosticDriverKinds.inc -d tools/clang/include/clang/Basic/DiagnosticDriverKinds.inc.d

=================================================================
==22777==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 67584 byte(s) in 1 object(s) allocated from:
    #0 0x7fdc5bc1cae8 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10dae8)
    #1 0x561564416caa in RegisterHandlers() (/home/shivam/Desktop/projects/github/llvm-project/build/bin/clang-tblgen+0x500caa)
    #2 0x561564419c86 in llvm::sys::AddSignalHandler(void (*)(void*), void*) (/home/shivam/Desktop/projects/github/llvm-project/build/bin/clang-tblgen+0x503c86)
    #3 0x561563fb1d25 in main (/home/shivam/Desktop/projects/github/llvm-project/build/bin/clang-tblgen+0x9bd25)
    #4 0x7fdc5b5991e2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x271e2)

SUMMARY: AddressSanitizer: 67584 byte(s) leaked in 1 allocation(s).
ninja: build stopped: subcommand failed.

[jankratochvil]

I do not have the testsuite running with ASAN (it is also failing for me) but I expected to at least run by hand ASAN-built LLDB for the code modified by this patch.

[shivammittal99]

run by hand ASAN-built LLDB for the code modified by this patch.

How do I do that? This is my first PR to llvm, so I am not aware of the tools. Please bear with me.

[jankratochvil]

In D76569#1936796, @shivammittal99 wrote:

How do I do that? This is my first PR to llvm, so I am not aware of the tools. Please bear with me.

No big deal, just compile it with -DLLVM_USE_SANITIZER=Address as you have already done above and run ./bin/lldb to test the code changed in this patch. Thanks for this work.

[aprantl]

In D76569#1936796, @shivammittal99 wrote:

run by hand ASAN-built LLDB for the code modified by this patch.

How do I do that? This is my first PR to llvm, so I am not aware of the tools. Please bear with me.

FYI, after the fact, if you've already committed the patch, you can also check the sanitized bot, which runs the entire testsuite using an ASAN+UBSAN lldb: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-sanitized/

[shivammittal99]

Remove conditional and move raw_command_string line

[labath]

Looks good. I wouldn't be too worried about asan as a StringRef provides the same lifetime guarantees (== none) as a const char *.

[jankratochvil]

In D76569#1938577, @labath wrote:

I wouldn't be too worried about asan as a StringRef provides the same lifetime guarantees (== none) as a const char *.

That's true but ASAN failed for me for the first patch. But that looks as some GCC bug which I cannot reproduce on a minimal testcase.

=================================================================
==722160==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7fffffffa4c0 at pc 0x7fffd9c605fc bp 0x7fffffff9cc0 sp 0x7fffffff9cb0
READ of size 1 at 0x7fffffffa4c0 thread T0
    #0 0x7fffd9c605fb in void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>(char const*, char const*, std::forward_iterator_tag) (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0x28d45fb)
    #1 0x7fffdb165322 in lldb_private::CommandObject::CommandObject(lldb_private::CommandInterpreter&, llvm::StringRef, llvm::StringRef, llvm::StringRef, unsigned int) (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0x3dd9322)
    #2 0x7fffdb16aed0 in lldb_private::CommandObjectRegexCommand::CommandObjectRegexCommand(lldb_private::CommandInterpreter&, llvm::StringRef, llvm::StringRef, llvm::StringRef, unsigned int, unsigned int, bool) (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0x3ddeed0)
    #3 0x7fffe2c933c7 in CommandObjectCommandsAddRegex::DoExecute(lldb_private::Args&, lldb_private::CommandReturnObject&) (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0xb9073c7)
    #4 0x7fffdb160ae1 in lldb_private::CommandObjectParsed::Execute(char const*, lldb_private::CommandReturnObject&) (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0x3dd4ae1)
    #5 0x7fffdb149b62 in lldb_private::CommandInterpreter::HandleCommand(char const*, lldb_private::LazyBool, lldb_private::CommandReturnObject&, lldb_private::ExecutionContext*, bool, bool) (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0x3dbdb62)
    #6 0x7fffdb14f1dc in lldb_private::CommandInterpreter::IOHandlerInputComplete(lldb_private::IOHandler&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0x3dc31dc)
    #7 0x7fffdad58e63 in lldb_private::IOHandlerEditline::Run() (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0x39cce63)
    #8 0x7fffdacc8321 in lldb_private::Debugger::RunIOHandlers() (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0x393c321)
    #9 0x7fffdb1032fc in lldb_private::CommandInterpreter::RunCommandInterpreter(bool, bool, lldb_private::CommandInterpreterRunOptions&) (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0x3d772fc)
    #10 0x7fffd9e53e6b in lldb::SBDebugger::RunCommandInterpreter(bool, bool) (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0x2ac7e6b)
    #11 0x4134c6 in Driver::MainLoop() (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/lldb+0x4134c6)
    #12 0x42339d in main (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/lldb+0x42339d)
    #13 0x7fffd544f1a2 in __libc_start_main ../csu/libc-start.c:308
    #14 0x4078ad in _start (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/lldb+0x4078ad)

Address 0x7fffffffa4c0 is located in stack of thread T0 at offset 944 in frame
    #0 0x7fffe2c92aa3 in CommandObjectCommandsAddRegex::DoExecute(lldb_private::Args&, lldb_private::CommandReturnObject&) (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0xb906aa3)

  This frame has 39 object(s):
    [32, 33) '<unknown>'
    [48, 49) '<unknown>'
    [64, 65) '<unknown>'
    [80, 81) '<unknown>'
    [96, 97) '<unknown>'
    [112, 113) '<unknown>'
    [128, 129) '<unknown>'
    [144, 145) '<unknown>'
    [160, 161) '<unknown>'
    [176, 177) '<unknown>'
    [192, 193) '<unknown>'
    [208, 209) '<unknown>'
    [224, 225) '<unknown>'
    [240, 241) '<unknown>'
    [256, 264) '<unknown>'
    [288, 304) '<unknown>'
    [320, 336) 'name' (line 990)
    [352, 368) '<unknown>'
    [384, 400) '<unknown>'
    [416, 432) 'io_handler_sp' (line 999)
    [448, 464) '<unknown>'
    [480, 496) '<unknown>'
    [512, 528) '<unknown>'
    [544, 560) '<unknown>'
    [576, 592) '<unknown>'
    [608, 624) '<unknown>'
    [640, 656) '<unknown>'
    [672, 688) '<unknown>'
    [704, 720) '<unknown>'
    [736, 752) '<unknown>'
    [768, 784) '<unknown>'
    [800, 816) '<unknown>'
    [832, 848) '<unknown>'
    [864, 880) '<unknown>'
    [896, 912) 'cmd_sp' (line 1130)
    [928, 960) '<unknown>' <== Memory access at offset 944 is inside this variable
    [992, 1024) '<unknown>'
    [1056, 1096) 'error' (line 989)
    [1136, 1176) '<unknown>'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-use-after-scope (/quad/home/jkratoch/redhat/llvm-monorepo2-gccassertdebugasan/bin/../lib/liblldb.so.11git+0x28d45fb) in void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>(char const*, char const*, std::forward_iterator_tag)
Shadow bytes around the buggy address:
  0x10007fff7440: 01 f2 00 f2 f2 f2 00 00 f2 f2 00 00 f2 f2 00 00
  0x10007fff7450: f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 f2 f2 00 00
  0x10007fff7460: f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 f2 f2 f8 f8
  0x10007fff7470: f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 f2 f2 00 00
  0x10007fff7480: f2 f2 00 00 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2 00 00
=>0x10007fff7490: f2 f2 00 00 f2 f2 f8 f8[f8]f8 f2 f2 f2 f2 f8 f8
  0x10007fff74a0: f8 f8 f2 f2 f2 f2 00 00 00 00 00 f2 f2 f2 f2 f2
  0x10007fff74b0: 00 00 00 00 00 f3 f3 f3 f3 f3 00 00 00 00 00 00
  0x10007fff74c0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
  0x10007fff74d0: f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2
  0x10007fff74e0: f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2 00 00 f2 f2
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==722160==ABORTING

cmake ../llvm-monorepo/llvm/ -DCMAKE_BUILD_TYPE=Debug  -DLLVM_USE_LINKER=gold -DLLVM_ENABLE_PROJECTS="lldb;clang;lld"  -DLLVM_USE_SPLIT_DWARF=ON -DCMAKE_EXE_LINKER_FLAGS="-fuse-ld=gold  -Wl,--gdb-index" -DCMAKE_SHARED_LINKER_FLAGS="-fuse-ld=gold  -Wl,--gdb-index" -DLLVM_ENABLE_ASSERTIONS=ON  -DLLVM_OPTIMIZED_TABLEGEN=ON -DLLVM_USE_SANITIZER=Address
# Producer is GNU C++14 9.2.1 20190827 (Red Hat 9.2.1-1) -mtune=generic -march=x86-64 -g -gsplit-dwarf -O1 -std=c++14 -fPIC -fvisibility-inlines-hidden -fno-omit-frame-pointer -fsanitize=address -fsanitize-address-use-after-scope -fno-exceptions -fno-rtti.

[jankratochvil]

The ASan GCC report was valid ( https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94299#c6 ) but that was really just the first/previous version of the patch.
I will try to check why clang ASan does not report it (and also GCC ASan on a minimal reproducer does not report it for me).

[labath]

In D76569#1942271, @jankratochvil wrote:

The ASan GCC report was valid ( https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94299#c6 )

Ah, that is cute. :) Thanks for following this up.

Now that the condition is gone (as it should be), we should be fine.

[shivammittal99]

Can someone please land this patch for me? I do not have commit access.

lldb/source/Commands/CommandObjectCommands.cpp

bool DoExecute(llvm::StringRef raw_command_line,
     if (raw_command_line.empty()) {
       result.AppendError("'command alias' requires at least two arguments");
       return false;
     }
 
     ExecutionContext exe_ctx = GetCommandInterpreter().GetExecutionContext();
     m_option_group.NotifyOptionParsingStarting(&exe_ctx);
 
     OptionsWithRaw args_with_suffix(raw_command_line);
-    const char *remainder = args_with_suffix.GetRawPart().c_str();
 
     if (args_with_suffix.HasArgs())
       if (!ParseOptionsAndNotify(args_with_suffix.GetArgs(), result,
                                  m_option_group, exe_ctx))
         return false;
 
-    llvm::StringRef raw_command_string(remainder);
+    llvm::StringRef raw_command_string = args_with_suffix.GetRawPart();
     Args args(raw_command_string);
 
     if (args.GetArgumentCount() < 2) {
       result.AppendError("'command alias' requires at least two arguments");
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
void OptionParsingStarting(ExecutionContext *execution_context) override {
       m_help.clear();
       m_syntax.clear();
     }
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
       return llvm::makeArrayRef(g_regex_options);
     }
 
-    // TODO: Convert these functions to return StringRefs.
-    const char *GetHelp() {
-      return (m_help.empty() ? nullptr : m_help.c_str());
-    }
+    llvm::StringRef GetHelp() { return m_help; }
 
-    const char *GetSyntax() {
-      return (m_syntax.empty() ? nullptr : m_syntax.c_str());
-    }
+    llvm::StringRef GetSyntax() { return m_syntax; }
 
   protected:
     // Instance variables to hold the values for command options.
 
     std::string m_help;
     std::string m_syntax;
   };