This is an archive of the discontinued LLVM Phabricator instance.

[mlir][lldb] Fix several gcc warnings in mlir and lldb
ClosedPublic

Authored by stella.stamenova on Feb 26 2021, 2:58 PM.

Details

Summary

These warnings are raised when compiling with gcc due to either having too few or too many commas, or in the case of lldb, the possibility of a nullptr.

Diff Detail

Event Timeline

stella.stamenova requested review of this revision.Feb 26 2021, 2:58 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 26 2021, 2:58 PM
mehdi_amini accepted this revision.Feb 27 2021, 10:01 AM
This revision is now accepted and ready to land.Feb 27 2021, 10:01 AM
shafik added a subscriber: shafik.Feb 28 2021, 6:31 PM

lldb change LGTM

teemperor added inline comments.
lldb/source/Commands/CommandObjectTrace.cpp
119

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.

lldb/source/Commands/CommandObjectTrace.cpp
119

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.

This revision was landed with ongoing or failed builds.Mar 1 2021, 1:50 PM
This revision was automatically updated to reflect the committed changes.
teemperor added inline comments.Mar 2 2021, 1:33 AM
lldb/source/Commands/CommandObjectTrace.cpp
119

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.