teemperor added inline comments.

================
Comment at: lldb/source/Commands/CommandObjectTrace.cpp:119
       lldb::TraceSP trace_sp = traceOrErr.get();
-      if (m_options.m_verbose)
+      if (m_options.m_verbose && trace_sp)
         result.AppendMessageWithFormat("loading trace with plugin %s\n",
----------------
stella.stamenova wrote:
> teemperor wrote:
> > Do you still have that GCC warning around (or the GCC version that produced 
> > that warning)? I'm kinda curious what GCC is assuming here. The interface 
> > never returns a "null" shared_ptr, the shared_ptr either contains a valid 
> > object or llvm::Error is returned. IIRC the Expected<ptr> return is the 
> > "new" version of how the Plugin interface should work (the old was just a 
> > ptr with `nullptr` being an error), so if some GCC version doesn't 
> > understand the Expected<ptr> pattern then we should maybe have some more 
> > centralized workaround instead of adding all the unnecessary nullptr checks.
> > 
> > But that shouldn't block this patch, so feel free to land.
> The gcc version is:
> 
> `gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0`
> 
> The warning is:
> 
> ```
> /mnt/_work/9/s/llvm-project/lldb/source/Commands/CommandObjectTrace.cpp: In 
> member function ‘virtual bool 
> CommandObjectTraceLoad::DoExecute(lldb_private::Args&, 
> lldb_private::CommandReturnObject&)’:
> /mnt/_work/9/s/llvm-project/lldb/source/Commands/CommandObjectTrace.cpp:120:39:
>  error: ‘%s’ directive argument is null [-Werror=format-overflow=]
>   120 |         result.AppendMessageWithFormat("loading trace with plugin 
> %s\n",
>       |         
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   121 |                                        
> trace_sp->GetPluginName().AsCString());
>       |                                        
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /mnt/_work/9/s/llvm-project/lldb/source/Commands/CommandObjectTrace.cpp:120:39:
>  error: ‘%s’ directive argument is null [-Werror=format-overflow=]
> ```
> 
> Interestingly, this is the only place in all of llvm (well, llvm, lld, clang 
> and lldb) that produces this warning.
> 
Ah, the problem is that we call `AsCString` and because ConstString's API was 
literally developed in hell this function returns `nullptr` if the ConstString 
is empty (or the weird null state). I'm gonna make a patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97586/new/

https://reviews.llvm.org/D97586

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to