This is an archive of the discontinued LLVM Phabricator instance.

Don't try to link against a release python during a debug build.
ClosedPublic

Authored by zturner on Jul 9 2014, 1:33 PM.

Details

Summary

When doing a debug build of LLDB, python headers automatically force a link against python27_d.lib. Since obtaining a copy of python27_d.lib requires building python manually, we attempted to avoid requiring LLDB developers to build python themselves as a matter of convenience. This leads to undefined behavior.

To reproduce the failures, you can do the following:

  1. Build a debug build of LLDB on Windows with python enabled.
  2. Run LLDB
  3. Type "script" with no arguments.

When the python interpreter launches, you get a CRT error from the Visual Studio runtime. This is most likely due to the fact that in order to make this work, we #undef'ed _DEBUG, then included stdlib.h, then included python.h, then #define'd _DEBUG again. This leads to stdlib.h being included with a different defines in different translation units. For example, a TU which indirectly includes lldb-python.h will see a different pre-processed stdlib.h than a translation unit which does not include lldb-python.h

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 11219.Jul 9 2014, 1:33 PM
zturner retitled this revision from to Don't try to link against a release python during a debug build..
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: deepak2427, tfiala.
zturner added a subscriber: Unknown Object (MLST).

Note that this means that if someone on Windows wants to enable python support and/or run tests, they will have to build Python themselves. This is unfortunate, but it seems like the way it has to be.

We can try to streamline this in the future by checking in a hermetic copy of Python with necessary build files, but until that day comes, building manually seems like the best option. Note that this only applies if you have LLDB_DISABLE_PYTHON=0, which is NOT the default.

zturner updated this revision to Diff 11227.Jul 9 2014, 3:36 PM

Stop normalizing the case of .py files when they're copied over. Python module names are case-sensitive, so this can lead to errors.

tfiala edited edge metadata.Jul 10 2014, 11:00 AM

We can try to streamline this in the future by checking in a hermetic

copy of Python with necessary build files, but until that day comes,
building manually seems like the best option. Note that this only applies
if you have LLDB_DISABLE_PYTHON=0, which is NOT the default.

FWIW this is highly unlikely to happen due to the purity of license that
LLVM wants to adhere to - we have way too many organizations using
LLVM-based code to want to pollute the license in any way.

I'll have a look at the patch as soon as I get a few cycles. Should be
today.

Looks fine, I'm giving it a test run now...

scripts/Python/finishSwigPythonLLDB.py
154 ↗(On Diff #11227)

Hey what was the normcase() trying to take care of?

Good question, I don't know. But I don't see how it could be correct,
because python module names are case-sensitive, and existing python code is
written against the un-normalized names.

zturner updated this revision to Diff 11293.Jul 10 2014, 2:11 PM
zturner edited edge metadata.
tfiala accepted this revision.Jul 10 2014, 2:49 PM
tfiala edited edge metadata.

LGTM. No issues on Linux and MacOSX. No new test failures on either. Assuming this is working on Windows, all good here.

This revision is now accepted and ready to land.Jul 10 2014, 2:49 PM
zturner closed this revision.Jul 10 2014, 4:56 PM
zturner updated this revision to Diff 11302.

Closed by commit rL212785 (authored by @zturner).