Page MenuHomePhabricator

[LLDB] Silence warnings from ScriptedProcessPythonInterface.cpp
ClosedPublic

Authored by mib on Jul 11 2021, 8:51 PM.

Details

Summary

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

Diff Detail

Event Timeline

omjavaid created this revision.Jul 11 2021, 8:51 PM
omjavaid requested review of this revision.Jul 11 2021, 8:51 PM
mib commandeered this revision.Jul 19 2021, 3:19 AM
mib edited reviewers, added: omjavaid; removed: 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 updated this revision to Diff 360857.Jul 22 2021, 9:32 AM
mib edited the summary of this revision. (Show Details)
mib added a reviewer: teemperor.
mib added a project: Restricted Project.

Conform ScriptedProcessPythonInterface to SWIG python types

teemperor requested changes to this revision.Jul 22 2021, 10:29 AM

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.

This revision now requires changes to proceed.Jul 22 2021, 10:29 AM
mib marked 4 inline comments as done.Jul 22 2021, 12:25 PM
mib added inline comments.
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 updated this revision to Diff 360925.Jul 22 2021, 12:28 PM
mib marked 3 inline comments as done.

Address @teemperor comments.

teemperor accepted this revision.Jul 22 2021, 1:16 PM

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.

This revision is now accepted and ready to land.Jul 22 2021, 1:16 PM
mib marked 3 inline comments as done.Jul 22 2021, 1:45 PM
mib updated this revision to Diff 360958.Jul 22 2021, 1:47 PM

Address last comments.

This revision was landed with ongoing or failed builds.Jul 22 2021, 1:48 PM
This revision was automatically updated to reflect the committed changes.
mib added a comment.Jul 22 2021, 2:01 PM

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

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.