Issues with https://reviews.llvm.org/D26082

Issues have been reported in https://reviews.llvm.org/D26082 which I have locally reproduced.

What’s the best course of action?

I can revert the change until problems are fixed. I don’t think libclang’s Python bindings tests get run as part of CI.

regards,

Jon

Issues have been reported in https://reviews.llvm.org/D26082 which I have
locally reproduced.

What's the best course of action?

Hi Jonathan,

This is before we branched, so whatever action we take need to be
back-ported to the 4.0 branch.

Adding Hans FYI.

I can revert the change until problems are fixed. I don't think libclang's
Python bindings tests get run as part of CI.

Is this affecting buildbots or some kind of external CI?

It seems to me that we should test Python's bindings as a standard CI
run, if they're added in Clang in any way.

I'm not proposing running Python itself, as that can be hard to match
system expectations, but some kind of unit test to make sure the
bindings are correct on all supported versions of Python would go a
long way of avoiding this scenario in the future.

As to revert vs. not revert, it depends on what's broken and how quick
is the fix. The questions I always ask myself are:

1. Is the breakage stopping any CI look from reporting further bugs?
This also depends on how often the CI loop runs and how much relies on
it.
2. Is there someone looking at the problem? If no, then reverting it
the *only* course of action. If yes, we'll need time-frames for a
possible fix before deciding to revert.

If we hadn't crossed the branch barrier, I'd be all for reverting it.
But we have. So, to avoid multiple reversions and re-applies and
multiple RCs, I think we need to be a bit more cautious here.

cheers,
--renato

> Issues have been reported in https://reviews.llvm.org/D26082 which I
have
> locally reproduced.
>
> What's the best course of action?

Hi Jonathan,

This is before we branched, so whatever action we take need to be
back-ported to the 4.0 branch.

Adding Hans FYI.

> I can revert the change until problems are fixed. I don't think
libclang's
> Python bindings tests get run as part of CI.

Is this affecting buildbots or some kind of external CI?

I've not seen any failures due to this, I don't think python tests get run.

It seems to me that we should test Python's bindings as a standard CI
run, if they're added in Clang in any way.

Sounds like a good idea, I'd be keen to be involved with this.

I'm not proposing running Python itself, as that can be hard to match
system expectations, but some kind of unit test to make sure the
bindings are correct on all supported versions of Python would go a
long way of avoiding this scenario in the future.

As to revert vs. not revert, it depends on what's broken and how quick
is the fix. The questions I always ask myself are:

1. Is the breakage stopping any CI look from reporting further bugs?
This also depends on how often the CI loop runs and how much relies on
it.
2. Is there someone looking at the problem? If no, then reverting it
the *only* course of action. If yes, we'll need time-frames for a
possible fix before deciding to revert.

I'm not confident that I can patch this quickly.

If we hadn't crossed the branch barrier, I'd be all for reverting it.
But we have. So, to avoid multiple reversions and re-applies and
multiple RCs, I think we need to be a bit more cautious here.

As a point of information, before this patch clang bindings in python3
would segfault. Behaviour of bindings in python2 is unaffected by the patch.

best,

Jon

cheers,

> Issues have been reported in ⚙ D26082 Support for Python 3 in libclang python bindings which I
have
> locally reproduced.
>
> What's the best course of action?

FWIW I'd be in favour of reverting this patch. Broken python3 support seems
worse than no python3 support.

Was the behaviour before an error or just segfault?

Is the new behaviour an error or just silent bad behaviour?

--renato

> FWIW I'd be in favour of reverting this patch. Broken python3 support
seems
> worse than no python3 support.

Was the behaviour before an error or just segfault?

Just a segfault.

Is the new behaviour an error or just silent bad behaviour?

I don't see any issues on macOS (Sierra). I get errors using Fedora 23 with
the patch.

Just a segfault.

Sigh...

I don't see any issues on macOS (Sierra). I get errors using Fedora 23 with
the patch.

Sigh!!!

Ok, those are both bad situations.

Is there any change we can emit an error when the python version is 3?

If so, I'd be inclined to suggest we revert the patch and apply a new
one to add the error message, then back-port to 4.0.

cheers,
--renato

> Just a segfault.

Sigh...

> I don't see any issues on macOS (Sierra). I get errors using Fedora 23
with
> the patch.

Sigh!!!

Ok, those are both bad situations.

Is there any change we can emit an error when the python version is 3?

It would be very easy to raise an error for an unsupported Python version.
Would you like me to revert my patch and do this?

best,

Jon

Yes, please!

Also, please create a bug in our bugzilla, and make it block
https://llvm.org/bugs/show_bug.cgi?id=31622. Once reverted and
reapplied, post the commit revisions, so Hans can apply them to the
4.0 branch.

Thanks!
--renato

> It would be very easy to raise an error for an unsupported Python
version.
> Would you like me to revert my patch and do this?

Yes, please!

Also, please create a bug in our bugzilla, and make it block
https://llvm.org/bugs/show_bug.cgi?id=31622. Once reverted and
reapplied, post the commit revisions, so Hans can apply them to the
4.0 branch.

Added https://llvm.org/bugs/show_bug.cgi?id=31628.

Would be cool if you could sanity check it. I'm still new to this.

Thanks,

Jon

Sure! Please add the reviews to Phabricator
(https://reviews.llvm.org/differential/) and I'll review.

cheers,
--renato

> Added 31628 – Python 3 support for libclang python bindings causes memory access errors in linux.
>
> Would be cool if you could sanity check it. I'm still new to this.

Sure! Please add the reviews to Phabricator
(https://reviews.llvm.org/differential/) and I'll review.

I've reverted the problematic patch and added
https://reviews.llvm.org/D28682 to fix trivial language incompatibilities
and fail if Python version !=2.

regards,

Jon

I’ve raised https://llvm.org/bugs/show_bug.cgi?id=31673 to track the version number check fix.

I think this should be merged to the 4.0 branch.

regards,

Jon

Merged it in r292339.

Cheers,
Hans