Page MenuHomePhabricator

[LLDB] Silence warnings from ScriptedProcessPythonInterface.cpp

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



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.


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!


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


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.


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.


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.

Thanks for catching that!


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


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.


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;

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


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.