[LLDB] Silence warnings from ScriptedProcessPythonInterface.cpp

[mib]

Summary

This patch fixes ScriptedProcessPythonInterface::GetGenericInteger to
avoid compiler warning emitted due to size_t being 32 bit when built
on 32 bit paltform.

[mib]

Hello @omjavaid, apologies for only looking at this now but I was on vacation last week. This changes make sense but I'd rather conform to the Python interface types. Also, I'm still working on Scripted Processes and I'm planning to land a patch with these changes soon. Thanks for letting me know about this oversight.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp

187–188

I'd use to unsigned long long to match the interface.

[mib]

Conform ScriptedProcessPythonInterface to SWIG python types

[teemperor]

Some nits but otherwise this seems like the right direction. Thanks!

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp

66–72

Unrelated to the change here, but that looks like a typo in the string.

143

Could we return an llvm::Optional<...> here? Then we don't need the invalid_address variable but instead just return llvm::None in the early exits.

298–303

I think this should return false when the underlying object is invalid? Bit out of scope for this patch, so feel free to fix this in another review.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h

54

Could you actually make a typedef for this? typedef unsigned long long PythonInteger or so. cpython really like to change their API between versions and this would make the potential fixups a bit easier if they decide to change the return value.

We might actually want to put this typedef into the PythonObject API int the future, but that's probably out-of-scope for this patch.

[mib]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp

66–72

Thanks for catching that!

298–303

This is just the interface between Python and C++, the object sanity check is done above in ScriptedProcess::IsAlive.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h

54

There is actually a PythonInteger class already but in order to extract the scalar, I have to make a proxy StructuredData::Integer to get the uint64_t value. Notice, in this case, the return type is not unsigned long long anymore ...

[mib]

Address @teemperor comments.

[teemperor]

I think this is the last round of review so I'll just accept this modulo a few nits.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp

190

As discussed offline, I think with the API we got here I would much rather have the unhandled llvm::Expected from the old code than the strange StructuredData code that just silently hides errors. Let's revert this back to the old but with llvm::None and put a big FIXME there.

llvm::Expected<unsigned long long> size = py_return.AsUnsignedLongLong();
if (!size) {
  // FIXME: Handle error.
  return llvm::None;
}
return *size;

293–294

Could you spell the auto out here? llvm::Optional<uint64_t> is I think simple enough.

298–303

Same as above.

[mib]

Address last comments.

[mib]

Hi @omjavaid , please let me know if you're still seeing the warnings. Thanks.

[omjavaid]

In D105788#2897981, @mib wrote:

Hi @omjavaid , please let me know if you're still seeing the warnings. Thanks.

Yes all warnings fixed now. Thanks for taking care of this.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h

public:
 
   StructuredData::DictionarySP GetLoadedImages() override;
 
   lldb::pid_t GetProcessID() override;
 
   bool IsAlive() override;
 
 protected:
-  size_t GetGenericInteger(llvm::StringRef method_name);
+  llvm::Optional<uint64_t> GetGenericInteger(llvm::StringRef method_name);
   Status GetStatusFromMethod(llvm::StringRef method_name);
 
 private:
   // The lifetime is managed by the ScriptInterpreter
   ScriptInterpreterPythonImpl &m_interpreter;
   StructuredData::GenericSP m_object_instance_sp;
 };
 } // namespace lldb_private
 
 #endif // LLDB_ENABLE_PYTHON
 #endif // LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_SCRIPTEDPROCESSPYTHONINTERFACE_H

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp

Status ScriptedProcessPythonInterface::Launch() {
   return GetStatusFromMethod("launch");
 }
 
 Status ScriptedProcessPythonInterface::Resume() {
   return GetStatusFromMethod("resume");
 }
 
 bool ScriptedProcessPythonInterface::ShouldStop() {
-  return GetGenericInteger("shuold_stop");
+  llvm::Optional<unsigned long long> should_stop =
+      GetGenericInteger("should_stop");
+
+  if (!should_stop)
+    return false;
+
+  return static_cast<bool>(*should_stop);
 }
 
 Status ScriptedProcessPythonInterface::Stop() {
   return GetStatusFromMethod("stop");
 }
 
 Status ScriptedProcessPythonInterface::GetStatusFromMethod(
     llvm::StringRef method_name) {
if (status.Fail())
       return Status("error: %s", status.AsCString());
 
     return status;
   }
 
   return Status("Returned object is null.");
 }
 
-size_t
+llvm::Optional<unsigned long long>
 ScriptedProcessPythonInterface::GetGenericInteger(llvm::StringRef method_name) {
   Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
                  Locker::FreeLock);
 
   if (!m_object_instance_sp)
-    return LLDB_INVALID_ADDRESS;
+    return llvm::None;
 
   if (!m_object_instance_sp)
-    return LLDB_INVALID_ADDRESS;
+    return llvm::None;
   PythonObject implementor(PyRefType::Borrowed,
                            (PyObject *)m_object_instance_sp->GetValue());
 
   if (!implementor.IsAllocated())
-    return LLDB_INVALID_ADDRESS;
+    return llvm::None;
 
   PythonObject pmeth(
       PyRefType::Owned,
       PyObject_GetAttrString(implementor.get(), method_name.str().c_str()));
 
   if (PyErr_Occurred())
     PyErr_Clear();
 
   if (!pmeth.IsAllocated())
-    return LLDB_INVALID_ADDRESS;
+    return llvm::None;
 
   if (PyCallable_Check(pmeth.get()) == 0) {
     if (PyErr_Occurred())
       PyErr_Clear();
-    return LLDB_INVALID_ADDRESS;
+    return llvm::None;
   }
 
   if (PyErr_Occurred())
     PyErr_Clear();
 
   PythonObject py_return(PyRefType::Owned,
                          PyObject_CallMethod(implementor.get(),
                                              method_name.str().c_str(),
                                              nullptr));
 
   if (PyErr_Occurred()) {
     PyErr_Print();
     PyErr_Clear();
   }
 
-  if (py_return.get()) {
-    auto size = py_return.AsUnsignedLongLong();
-    return (size) ? *size : LLDB_INVALID_ADDRESS;
-  }
-  return LLDB_INVALID_ADDRESS;
+  if (!py_return.get())
+    return llvm::None;
+
+  llvm::Expected<unsigned long long> size = py_return.AsUnsignedLongLong();
+  // FIXME: Handle error.
+  if (!size)
+    return llvm::None;
+
+  return *size;
 }
 
 lldb::MemoryRegionInfoSP
 ScriptedProcessPythonInterface::GetMemoryRegionContainingAddress(
     lldb::addr_t address) {
   // TODO: Implement
   return nullptr;
 }

 }
 
 StructuredData::DictionarySP ScriptedProcessPythonInterface::GetLoadedImages() {
   // TODO: Implement
   return nullptr;
 }
 
 lldb::pid_t ScriptedProcessPythonInterface::GetProcessID() {
-  size_t pid = GetGenericInteger("get_process_id");
-
-  return (pid >= std::numeric_limits<lldb::pid_t>::max())
-             ? LLDB_INVALID_PROCESS_ID
-             : pid;
+  llvm::Optional<unsigned long long> pid = GetGenericInteger("get_process_id");
+  return (!pid) ? LLDB_INVALID_PROCESS_ID : *pid;
 }
 
 bool ScriptedProcessPythonInterface::IsAlive() {
-  return GetGenericInteger("is_alive");
+  llvm::Optional<unsigned long long> is_alive = GetGenericInteger("is_alive");
+
+  if (!is_alive)
+    return false;
+
+  return static_cast<bool>(*is_alive);
 }
 
 #endif