Support for Python 3 in libclang python bindings

[jbcoe]

Summary

Python bindings tests now pass in Python 3.

map in Python 3 is lazily evaluated so the method by which functions are registered needed updating.

Strings are unicode in Python 3 not UTF-8, I've tried to create an new c_types-like class (c_string_p) to automate the conversion.

String conversions made explicit where required.

It would be useful to add Python 3 tests to continuous integration.

[jbcoe]

Python bindings tests now pass in Python 3.

[jbcoe]

Remove mistakenly committed debugging output.

[compnerd]

It would be nice if there is a simple way to handle the possible performance impact for python 2. Worst case, we can deal with it when it becomes an issue.

bindings/python/clang/cindex.py

518

IIRC, range and xrange did have some performance difference. This would slow down the bindings on python 2. The difference is obviously not immediately visible unless count is very high. I wonder if we can do anything here to detect the python version and dispatch to xrange in python 2.

[mgorny]

A few comments/questions. However, please note that those are generic Python comments and I haven't used or tested the clang Python API yet.

bindings/python/clang/cindex.py

77

What is the Python3 version you're aiming to support? If I recall correctly, u"" is allowed (again) since 3.4. And even then, the condition looks weird and makes me think a while before I figure out what it's supposed to mean.

518

The difference is that range() used to construct a complete list in Python 2. In Python 3, xrange() (that uses iterator) got renamed to range().

If you want to avoid performance impact (not sure if it's really measurable here), you can do something alike C for loop:

i = 0
while i < count:
    #...
    i += 1

It's not really idiomatic Python though. OTOH, it won't take more lines than the conditional.

623

Why are you changing this? The old version seems to be correct for Python3.

2365

Seems to be mistakenly added.

2566–2568

I may be wrong but I think you could use a list comprehension here.

args_array = (c_string_p * len(args))([c_string_p(x) for x in args])

You can also try without [] to avoid the overhead of constructing list, if the function can take an iterator.

bindings/python/tests/cindex/test_translation_unit.py

62 ↗

(On Diff #76344)

Could you try inverting this? Python 2.7 already has io.StringIO, so that branch is much more likely to work.

[jbcoe]

Respond to review comments.

bindings/python/clang/cindex.py

77

Replaced with a simple version check

518

I've defined xrange to be range for python3. A bit confusing but then we can use xrange uniformly.

623

filter has changed in Python 3 and the replaced code does not behave the same as this simple list comprehension. I've not delved deeper into why but see test failures without this change.

2566–2568

I can't get this to work with a comprension or generator, I agree either would be an improvement.

bindings/python/tests/cindex/test_translation_unit.py

62 ↗

(On Diff #76344)

Python 2.7's io.StringIO needs a unicode string.

[MathieuDuponchelle]

I get tons of errors when running these tests with python 3 (version 3.4.3) with a fresh build of clang and llvm. The number of failures vs errors vary wildly between each invocation, the number of total failures + errors stays the same, which led me to think there was some sort of memory corruption.

Running the tests under valgrind make them pass, but also shows errors which I assume are ctypes-related.

This is valgrind's output for  LD_LIBRARY_PATH=/home/meh/devel/sandbox/llvm/build/lib valgrind python3 -m nose tests.cindex.test_cursor:test_get_template_argument_value as an example

valgrind_output.txt18 KBDownload

[jbcoe]

Thanks Mathieu.

Tests ran without issues when I submitted my patch but I tested on macOS.

I'll have a look into this and see if I can reproduce the problem.

[MathieuDuponchelle]

Hey, for what it's worth when skipping the free at this line: https://github.com/llvm-mirror/clang/blob/master/bindings/python/clang/cindex.py#L181 the issues are gone, I haven't investigated the reason why.

[jbcoe]

Hi Mathieu,
What platform did you run the tests on? How did you run them? I don't get any failures when running tests on macOS 10.12.1.

command: env PYTHONPATH=$(pwd) LD_LIBRARY_PATH=/Users/jon/DEV/LLVM/build-ninja/lib nosetests-3.4 .

python: Python 3.5.2 (default, Oct 11 2016, 04:59:56)
[GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.38)] on darwin

Python installed using homebrew.

I'm keen to get to the bottom of this failure. I've been distracted by other things recently.

[MathieuDuponchelle]

Hey, I'm on Linux, Fedora 23 to be specific.

[MathieuDuponchelle]

In D26082#621496, @jbcoe wrote:

Hi Mathieu,
What platform did you run the tests on? How did you run them? I don't get any failures when running tests on macOS 10.12.1.

Hey @jbcoe, do you think you can have a look at this issue? Please see my earlier comment for a way to reproduce this (LD_LIBRARY_PATH=/home/meh/devel/sandbox/llvm/build/lib valgrind python3 -m nose tests.cindex.test_cursor:test_get_template_argument_value is a valid way to reproduce this issue)

Please note that I'm thankful for you doing the porting work, much appreciated, with this issue fixed I'll be perfectly happy :)

[jbcoe]

@MathieuDuponchelle I'm still keen to get to the bottom of this failure. I've not had time to reproduce it yet.

I'm thankful for you taking a look at this and reporting the issue. We need to get this working well for everyone.

[jbcoe]

@MathieuDuponchelle I've set up a docker image with Fedora 23 and reproduced the errors as you reported.

I've no immediate idea what the problem might be. Might be worth looking with address-sanitizer.

[MathieuDuponchelle]

In D26082#644402, @jbcoe wrote:

@MathieuDuponchelle I've set up a docker image with Fedora 23 and reproduced the errors as you reported.

I've no immediate idea what the problem might be. Might be worth looking with address-sanitizer.

Hey, good to know!

Two questions:

• Did you try running the test with valgrind on OSX?
• Doesn't the fact that commenting out this line: https://github.com/llvm-mirror/clang/blob/master/bindings/python/clang/cindex.py#L181 "solve" the problem give you a good starting point?

[jbcoe]

Sadly I can't run valgrind on macOS Sierra.

I'll try and look at the effects of bindings/python/clang/cindex.py L181 in more detail today.

Just out of curiosity, will you be using the Python 3 bindings for anything? If so what?

[jbcoe]

I've reverted this change and submitted https://reviews.llvm.org/D28682 to explicitly check the Python version.

I'll pick this up again, I do want Python 3 support.

[MathieuDuponchelle]

In D26082#645361, @jbcoe wrote:

I've reverted this change and submitted https://reviews.llvm.org/D28682 to explicitly check the Python version.

I'll pick this up again, I do want Python 3 support.

I'm not entirely sure why you reverted the patch altogether, apart from the problem I raised it worked flawlessly, and in any case did not create any problems when the bindings were used from python 2.

bindings/python/clang/cindex.py

 import clang.enumerations
 
 # ctypes doesn't implicitly convert c_void_p to the appropriate wrapper
 # object. This is a problem, because it means that from_parameter will see an
 # integer and pass the wrong value on platforms where int != void*. Work around
 # this by marshalling object arguments as void**.
 c_object_p = POINTER(c_void_p)
 
+# Python 3 strings are unicode, translate them to/from utf8 for C-interop
+if type(u"") == str:
+    class c_string_p(c_char_p):
+        def __init__(self, p=None):
+            if p is None:
+                p = ""
+            if type(p) == str:
+                p = p.encode("utf8")
+            super(c_char_p, self).__init__(p)
+
+        def __str__(self):
+            return self.value
+
+        @property
+        def value(self):
+            return super(c_char_p, self).value.decode("utf8")
+
+        @classmethod
+        def from_param(cls, param):
+                return cls(param)
+else:
+    c_string_p = c_char_p
+
+
 callbacks = {}
 
 ### Exception Classes ###
 
 class TranslationUnitLoadError(Exception):
     """Represents an error that occurred when loading a TranslationUnit.
 
     This is raised in the case where a TranslationUnit could not be
def __get__(self, instance, instance_type=None):
         setattr(instance, self.wrapped.__name__, value)
 
         return value
 
 
 class _CXString(Structure):
     """Helper for transforming CXString results."""
 
-    _fields_ = [("spelling", c_char_p), ("free", c_int)]
+    _fields_ = [("spelling", c_string_p), ("free", c_int)]
 
     def __del__(self):
         conf.lib.clang_disposeString(self)
 
     @staticmethod
     def from_result(res, fn, args):
         assert isinstance(res, _CXString)
         return conf.lib.clang_getCString(res)
def get_tokens(tu, extent):
         # anything and potentially call a destructor on nothing.
         if count < 1:
             return
 
         tokens_array = cast(tokens_memory, POINTER(Token * count)).contents
 
         token_group = TokenGroup(tu, tokens_memory, tokens_count)
 
         for i in xrange(0, count):
             token = Token()
             token.int_data = tokens_array[i].int_data
             token.ptr_data = tokens_array[i].ptr_data
             token._tu = tu
             token._group = token_group
 
             yield token
 
class BaseEnumeration(object):
     respectively.
 
     """
 
     def __init__(self, value):
         if value >= len(self.__class__._kinds):
             self.__class__._kinds += [None] * (value - len(self.__class__._kinds) + 1)
         if self.__class__._kinds[value] is not None:
-            raise ValueError,'{0} value {1} already loaded'.format(
-                str(self.__class__), value)
+            raise ValueError('{0} value {1} already loaded'.format(
+                str(self.__class__), value))
         self.value = value
         self.__class__._kinds[value] = self
         self.__class__._name_map = None
 
 
     def from_param(self):
         return self.value
 
     @property
     def name(self):
         """Get the enumeration name of this cursor kind."""
         if self._name_map is None:
             self._name_map = {}
             for key, value in self.__class__.__dict__.items():
                 if isinstance(value, self.__class__):
                     self._name_map[value] = key
         return self._name_map[self]
 
     @classmethod
     def from_id(cls, id):
         if id >= len(cls._kinds) or cls._kinds[id] is None:
-            raise ValueError,'Unknown template argument kind %d' % id
+            raise ValueError('Unknown template argument kind %d' % id)
         return cls._kinds[id]
 
     def __repr__(self):
         return '%s.%s' % (self.__class__, self.name,)
 
 
 class CursorKind(BaseEnumeration):
     """
     A CursorKind describes the kind of entity that a cursor points to.
     """
 
     # The required BaseEnumeration declarations.
     _kinds = []
     _name_map = None
 
     @staticmethod
     def get_all_kinds():
         """Return all CursorKind enumeration instances."""
         return filter(None, CursorKind._kinds)
 
     def is_declaration(self):
         """Test if this is a declaration kind."""
         return conf.lib.clang_isDeclaration(self)
 
     def is_reference(self):
         """Test if this is a reference kind."""
         return conf.lib.clang_isReference(self)
class StorageClass(object):
     # The unique kind objects, index by id.
     _kinds = []
     _name_map = None
 
     def __init__(self, value):
         if value >= len(StorageClass._kinds):
             StorageClass._kinds += [None] * (value - len(StorageClass._kinds) + 1)
         if StorageClass._kinds[value] is not None:
-            raise ValueError,'StorageClass already loaded'
+            raise ValueError('StorageClass already loaded')
         self.value = value
         StorageClass._kinds[value] = self
         StorageClass._name_map = None
 
     def from_param(self):
         return self.value
 
     @property
     def name(self):
         """Get the enumeration name of this storage class."""
         if self._name_map is None:
             self._name_map = {}
             for key,value in StorageClass.__dict__.items():
                 if isinstance(value,StorageClass):
                     self._name_map[value] = key
         return self._name_map[self]
 
     @staticmethod
     def from_id(id):
         if id >= len(StorageClass._kinds) or not StorageClass._kinds[id]:
-            raise ValueError,'Unknown storage class %d' % id
+            raise ValueError('Unknown storage class %d' % id)
         return StorageClass._kinds[id]
 
     def __repr__(self):
         return 'StorageClass.%s' % (self.name,)
 
 StorageClass.INVALID = StorageClass(0)
 StorageClass.NONE = StorageClass(1)
 StorageClass.EXTERN = StorageClass(2)
def get_size(self):
         Retrieve the size of the record.
         """
         return conf.lib.clang_Type_getSizeOf(self)
 
     def get_offset(self, fieldname):
         """
         Retrieve the offset of a field in the record.
         """
-        return conf.lib.clang_Type_getOffsetOf(self, c_char_p(fieldname))
+        return conf.lib.clang_Type_getOffsetOf(self, c_string_p(fieldname))
 
     def get_ref_qualifier(self):
         """
         Retrieve the ref-qualifier of the type.
         """
         return RefQualifierKind.from_id(
                 conf.lib.clang_Type_getCXXRefQualifier(self))
 
def __init__(self, obj):
         self.obj = self._as_parameter_ = obj
 
     def from_param(self):
         return self._as_parameter_
 
 
 class _CXUnsavedFile(Structure):
     """Helper for passing unsaved file arguments."""
-    _fields_ = [("name", c_char_p), ("contents", c_char_p), ('length', c_ulong)]
+    _fields_ = [("name", c_string_p), ("contents", c_string_p), ('length', c_ulong)]
 
 # Functions calls through the python interface are rather slow. Fortunately,
 # for most symboles, we do not need to perform a function call. Their spelling
 # never changes and is consequently provided by this spelling cache.
 SpellingCache = {
             # 0: CompletionChunk.Kind("Optional"),
             # 1: CompletionChunk.Kind("TypedText"),
             # 2: CompletionChunk.Kind("Text"),
def availability(self):
         return availabilityKinds[res]
 
     @property
     def briefComment(self):
         if conf.function_exists("clang_getCompletionBriefComment"):
             return conf.lib.clang_getCompletionBriefComment(self.obj)
         return _CXString()
 
     def __repr__(self):
         return " | ".join([str(a) for a in self]) \
                + " || Priority: " + str(self.priority) \
                + " || Availability: " + str(self.availability) \
                + " || Brief comment: " + str(self.briefComment.spelling)
 
 availabilityKinds = {
             0: CompletionChunk.Kind("Available"),
             1: CompletionChunk.Kind("Deprecated"),
def from_source(cls, filename, args=None, unsaved_files=None, options=0,
         if unsaved_files is None:
             unsaved_files = []
 
         if index is None:
             index = Index.create()
 
         args_array = None
         if len(args) > 0:
-            args_array = (c_char_p * len(args))(* args)
+            args_array = (c_string_p * len(args))()
+            for i,a in enumerate(args):
+                args_array[i] = c_string_p(a)
 
         unsaved_array = None
         if len(unsaved_files) > 0:
             unsaved_array = (_CXUnsavedFile * len(unsaved_files))()
             for i, (name, contents) in enumerate(unsaved_files):
                 if hasattr(contents, "read"):
                     contents = contents.read()
 
-                unsaved_array[i].name = name
-                unsaved_array[i].contents = contents
+                unsaved_array[i].name = c_string_p(name)
+                unsaved_array[i].contents = c_string_p(contents)
                 unsaved_array[i].length = len(contents)
 
         ptr = conf.lib.clang_parseTranslationUnit(index, filename, args_array,
                                     len(args), unsaved_array,
                                     len(unsaved_files), options)
 
         if not ptr:
             raise TranslationUnitLoadError("Error parsing translation unit.")
def reparse(self, unsaved_files=None, options=0):
         unsaved_files_array = 0
         if len(unsaved_files):
             unsaved_files_array = (_CXUnsavedFile * len(unsaved_files))()
             for i,(name,value) in enumerate(unsaved_files):
                 if not isinstance(value, str):
                     # FIXME: It would be great to support an efficient version
                     # of this, one day.
                     value = value.read()
-                    print value
+                    print(value)
                 if not isinstance(value, str):
-                    raise TypeError,'Unexpected unsaved file contents.'
+                    raise TypeError('Unexpected unsaved file contents.')
                 unsaved_files_array[i].name = name
                 unsaved_files_array[i].contents = value
                 unsaved_files_array[i].length = len(value)
         ptr = conf.lib.clang_reparseTranslationUnit(self, len(unsaved_files),
                 unsaved_files_array, options)
 
     def save(self, filename):
         """Saves the TranslationUnit to a file.
def codeComplete(self, path, line, column, unsaved_files=None,
         unsaved_files_array = 0
         if len(unsaved_files):
             unsaved_files_array = (_CXUnsavedFile * len(unsaved_files))()
             for i,(name,value) in enumerate(unsaved_files):
                 if not isinstance(value, str):
                     # FIXME: It would be great to support an efficient version
                     # of this, one day.
                     value = value.read()
-                    print value
+                    print(value)
                 if not isinstance(value, str):
-                    raise TypeError,'Unexpected unsaved file contents.'
+                    raise TypeError('Unexpected unsaved file contents.')
                 unsaved_files_array[i].name = name
                 unsaved_files_array[i].contents = value
                 unsaved_files_array[i].length = len(value)
         ptr = conf.lib.clang_codeCompleteAt(self, path, line, column,
                 unsaved_files_array, len(unsaved_files), options)
         if ptr:
             return CodeCompletionResults(ptr)
         return None

 functionList = [
   ("clang_annotateTokens",
    [TranslationUnit, POINTER(Token), c_uint, POINTER(Cursor)]),
 
   ("clang_CompilationDatabase_dispose",
    [c_object_p]),
 
   ("clang_CompilationDatabase_fromDirectory",
-   [c_char_p, POINTER(c_uint)],
+   [c_string_p, POINTER(c_uint)],
    c_object_p,
    CompilationDatabase.from_result),
 
   ("clang_CompilationDatabase_getAllCompileCommands",
    [c_object_p],
    c_object_p,
    CompileCommands.from_result),
 
   ("clang_CompilationDatabase_getCompileCommands",
-   [c_object_p, c_char_p],
+   [c_object_p, c_string_p],
    c_object_p,
    CompileCommands.from_result),
 
   ("clang_CompileCommands_dispose",
    [c_object_p]),
 
   ("clang_CompileCommands_getCommand",
    [c_object_p, c_uint],
functionList = [
    _CXString,
    _CXString.from_result),
 
   ("clang_CompileCommand_getNumArgs",
    [c_object_p],
    c_uint),
 
   ("clang_codeCompleteAt",
-   [TranslationUnit, c_char_p, c_int, c_int, c_void_p, c_int, c_int],
+   [TranslationUnit, c_string_p, c_int, c_int, c_void_p, c_int, c_int],
    POINTER(CCRStructure)),
 
   ("clang_codeCompleteGetDiagnostic",
    [CodeCompletionResults, c_int],
    Diagnostic),
 
   ("clang_codeCompleteGetNumDiagnostics",
    [CodeCompletionResults],
    c_int),
 
   ("clang_createIndex",
    [c_int, c_int],
    c_object_p),
 
   ("clang_createTranslationUnit",
-   [Index, c_char_p],
+   [Index, c_string_p],
    c_object_p),
 
   ("clang_CXXConstructor_isConvertingConstructor",
    [Cursor],
    bool),
 
   ("clang_CXXConstructor_isCopyConstructor",
    [Cursor],
#  [CXTUResourceUsage]),
    _CXString),
 
   ("clang_getCompletionPriority",
    [c_void_p],
    c_int),
 
   ("clang_getCString",
    [_CXString],
-   c_char_p),
+   c_string_p),
 
   ("clang_getCursor",
    [TranslationUnit, SourceLocation],
    Cursor),
 
   ("clang_getCursorDefinition",
    [Cursor],
    Cursor,
#  CXTUResourceUsage),
    c_longlong),
 
   ("clang_getEnumDeclIntegerType",
    [Cursor],
    Type,
    Type.from_result),
 
   ("clang_getFile",
-   [TranslationUnit, c_char_p],
+   [TranslationUnit, c_string_p],
    c_object_p),
 
   ("clang_getFileName",
    [File],
    _CXString), # TODO go through _CXString.from_result?
 
   ("clang_getFileTime",
    [File],
#  CXTUResourceUsage),
 
   ("clang_getTranslationUnitSpelling",
    [TranslationUnit],
    _CXString,
    _CXString.from_result),
 
   ("clang_getTUResourceUsageName",
    [c_uint],
-   c_char_p),
+   c_string_p),
 
   ("clang_getTypeDeclaration",
    [Type],
    Cursor,
    Cursor.from_result),
 
   ("clang_getTypedefDeclUnderlyingType",
    [Cursor],
#  CXTUResourceUsage),
    [Cursor],
    bool),
 
   ("clang_isVolatileQualifiedType",
    [Type],
    bool),
 
   ("clang_parseTranslationUnit",
-   [Index, c_char_p, c_void_p, c_int, c_void_p, c_int, c_int],
+   [Index, c_string_p, c_void_p, c_int, c_void_p, c_int, c_int],
    c_object_p),
 
   ("clang_reparseTranslationUnit",
    [TranslationUnit, c_int, c_void_p, c_int],
    c_int),
 
   ("clang_saveTranslationUnit",
-   [TranslationUnit, c_char_p, c_uint],
+   [TranslationUnit, c_string_p, c_uint],
    c_int),
 
   ("clang_tokenize",
    [TranslationUnit, SourceRange, POINTER(POINTER(Token)), POINTER(c_uint)]),
 
   ("clang_visitChildren",
    [Cursor, callbacks['cursor_visit'], py_object],
    c_uint),
#  CXTUResourceUsage),
    c_longlong),
 
   ("clang_Type_getClassType",
    [Type],
    Type,
    Type.from_result),
 
   ("clang_Type_getOffsetOf",
-   [Type, c_char_p],
+   [Type, c_string_p],
    c_longlong),
 
   ("clang_Type_getSizeOf",
    [Type],
    c_longlong),
 
   ("clang_Type_getCXXRefQualifier",
    [Type],
def register_functions(lib, ignore_errors):
 
     This must be called as part of library instantiation so Python knows how
     to call out to the shared library.
     """
 
     def register(item):
         return register_function(lib, item, ignore_errors)
 
-    map(register, functionList)
+    for f in functionList:
+        register(f)
 
 class Config:
     library_path = None
     library_file = None
     compatibility_check = True
     loaded = False
 
     @staticmethod