Page MenuHomePhabricator

Fix TestTargetAPI.py test for users who use Swig 3.0.5 or greater.
ClosedPublic

Authored by amccarth on Oct 12 2015, 3:11 PM.

Details

Reviewers
zturner
clayborg
Summary

How Swig handles varargs functions changed between 3.0.2 and 3.0.5, which causes one of our tests to fail. This fix adds a Swig directive to the SBError::SetErrorSTringWithFormat method and extends a test to call it with various numbers of arguments.

I've tested this fix with Swig 3.0.2, 3.0.5, and 3.0.7.

In Python, there are only two calls to this method, both in the sb_error.py test, and one of those is the failing line. It's unclear how many external scripts may be affected.

The caveat here is that, when calling from Python, all of the arguments must be strings. From the Swig documentation, "Doing anything more advanced than this is likely to involve a serious world of pain. In order to use a library like libffi, you will need to know the underlying calling conventions and details of the C++ ABI."

Background:

The message SBError::SetErrorStringWithFormat is a varargs function. In the Swig interface file, it's:

int
SetErrorStringWithFormat (const char *format, ...);

With 3.0.2, this creates a Python wrapper with this signature:

def SetErrorStringWithFormat(self, *args):
    """SetErrorStringWithFormat(SBError self, str const * format) -> int"""
    return _lldb.SBError_SetErrorStringWithFormat(self, *args)

But in 3.0.5 (and later), it comes up with:

def SetErrorStringWithFormat(self, format):
    """SetErrorStringWithFormat(SBError self, str const * format) -> int"""
    return _lldb.SBError_SetErrorStringWithFormat(self, format)

Note that it no longer takes a variable number of arguments. This causes a test failure when the Python code attempts to invoke the method with more than a format string.

======================================================================
ERROR: test_SBError (TestDefaultConstructorForAPIObjects.APIDefaultConstructorTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\src\llvm\llvm\tools\lldb\test\lldbtest.py", line 480, in wrapper
    return func(self, *args, **kwargs)
  File "D:\src\llvm\llvm\tools\lldb\test\lldbtest.py", line 521, in wrapper
    return func(self, *args, **kwargs)
  File "D:\src\llvm\llvm\tools\lldb\test\python_api\default-constructor\TestDefaultConstructorForAPIObjects.py", line 131, in test_SBError
    sb_error.fuzz_obj(obj)
  File "D:\src\llvm\llvm\tools\lldb\test\python_api\default-constructor\sb_error.py", line 19, in fuzz_obj
    obj.SetErrorStringWithFormat("%s!", "error")
TypeError: SetErrorStringWithFormat() takes exactly 2 arguments (3 given)
Config=i686-D:\src\llvm\build\ninja\bin\clang.exe
----------------------------------------------------------------------

This is consistent with the Swig documentation on varargs functions. See: http://www.swig.org/Doc3.0/Varargs.html#Varargs

Diff Detail

Event Timeline

amccarth updated this revision to Diff 37189.Oct 12 2015, 3:11 PM
amccarth retitled this revision from to Fix TestTargetAPI.py test for users who use Swig 3.0.5 or greater..
amccarth updated this object.
amccarth added a reviewer: zturner.
amccarth added a subscriber: lldb-commits.
zturner edited edge metadata.Oct 12 2015, 3:49 PM

Prior to this patch, what was the behavior of using SetErrorStringWithFormat with an argument that was not a string?

Can you add a test to fuzz_obj that calls SetErrorStringWithFormat("%d", 10) and run it against SWIG 3.0.2 and see what happens?

With Swig 3.0.2, non-string arguments work. I'm not sure why. Even the
Swig 1.x documentation says that what we're doing shouldn't work.

Adding Greg Clayton for advice on how to proceed.

Swig documentation talked about the peril of varargs methods: http://www.swig.org/Doc3.0/Varargs.html#Varargs

The fact that it appears to work with 3.0.2 seems to be a bug in 3.0.2.

My proposal is in this patch. An alternate idea would be to remove SetErrorStringWithFormat from the SB API, but I know we don't like to remove things from the API.

clayborg accepted this revision.Oct 12 2015, 4:42 PM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Oct 12 2015, 4:42 PM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in r250188.