classification
Title: Do not run pgen when it is not going to be used (cross-compiling)
Type: compile error Stage: resolved
Components: Cross-Build Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Alex.Willmer, doko, martin.panter, python-dev, thomas.perl, xdegaye
Priority: normal Keywords: patch

Created on 2016-07-11 22:42 by thomas.perl, last changed 2016-07-28 04:32 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
pgen_dependencies.patch thomas.perl, 2016-07-22 10:17 review
Messages (12)
msg270211 - (view) Author: Thomas Perl (thomas.perl) * Date: 2016-07-11 22:42
Problem description: Trying to cross-compile $(LIBRARY) (libpython2.7.a for example) causes "pgen" to be built, even when it's not used in the cross-compilation case (only a file copy is done to generate $(GRAMMAR_H) and $(GRAMMAR_C)).


The current rule for $(PGEN) in Makefile.pre.in does not include $(CFLAGS):

https://hg.python.org/cpython/file/tip/Makefile.pre.in#l810

This causes problems when $(CFLAGS) changes the ARM float ABI, e.g.:

CFLAGS="-mfloat-abi=hard"

This causes the following issues at link time:

 1. The .o files that get linked into "pgen" are built with CFLAGS
    (which is good, because some of them are used for libpython as well)
 2. When the "pgen" binary gets built, the $(CFLAGS) are not used
 3. Compiler fails to build "pgen" with different float ABI settings

=====

[...]
arm-none-eabi-gcc -c -fno-strict-aliasing -march=armv6k -mtune=mpcore -mfloat-abi=hard -mtp=soft -fomit-frame-pointer -ffunction-sections -DARM11 -D_3DS -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes  -I. -IInclude -I./Include   -DPy_BUILD_CORE -o Parser/pgenmain.o Parser/pgenmain.c
arm-none-eabi-gcc -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes  Parser/acceler.o Parser/grammar1.o Parser/listnode.o Parser/node.o Parser/parser.o Parser/parsetok.o Parser/bitset.o Parser/metagrammar.o Parser/firstsets.o Parser/grammar.o Parser/pgen.o Objects/obmalloc.o Python/mysnprintf.o Python/pyctype.o Parser/tokenizer_pgen.o Parser/printgrammar.o Parser/pgenmain.o  -o Parser/pgen
/Users/thp/pkg/devkitPro/devkitARM/bin/../lib/gcc/arm-none-eabi/5.3.0/../../../../arm-none-eabi/bin/ld: error: Parser/acceler.o uses VFP register arguments, Parser/pgen does not
/Users/thp/pkg/devkitPro/devkitARM/bin/../lib/gcc/arm-none-eabi/5.3.0/../../../../arm-none-eabi/bin/ld: failed to merge target specific data of file Parser/acceler.o
[...]

=====

Note that the error message is repeated for all .o files linked into pgen, I've only included one here for demonstration purposes. The following patch (against a Python 2.7.12 tarball, similar fix for Hg tip and Python 3) fixes the issue for me:

=====
diff -u Python-2.7.12/Makefile.pre.in Python-2.7.12-fix/Makefile.pre.in
--- Python-2.7.12/Makefile.pre.in	2016-06-25 23:49:31.000000000 +0200
+++ Python-2.7.12-fix/Makefile.pre.in	2016-07-12 00:17:02.000000000 +0200
@@ -698,7 +698,7 @@
 	fi
 
 $(PGEN):	$(PGENOBJS)
-		$(CC) $(OPT) $(LDFLAGS) $(PGENOBJS) $(LIBS) -o $(PGEN)
+		$(CC) $(OPT) $(CFLAGS) $(LDFLAGS) $(PGENOBJS) $(LIBS) -o $(PGEN)
 
 Parser/grammar.o:	$(srcdir)/Parser/grammar.c \
 				$(srcdir)/Include/token.h \
=====

Also note that the same $(CFLAGS) needs to be added to the rule for $(BUILDPYTHON) if one wants to build that as well, but in my case, I only did a "make libpython2.7.a", and that indirectly depends on pgen ($(LIBRARY) -> $(LIBRARY_OBJS) -> $(PYTHON_OBJS) -> Python/graminit.o -> $(GRAMMAR_C) -> $(GRAMMAR_H) -> $(PGEN), which results in that error message, so libpython2.7.a can't be built).

Another fix could be to make it so that $(GRAMMAR_H) does not depend on $(PGEN) if $(cross_compiling) is "yes" (if you read the rule contents for $(GRAMMAR_H), you'll find that indeed $(PGEN) isn't used at all if $(cross_compiling) is "yes". At least for GNU make, it might be possible to avoid building "pgen" in that case as follows and removing $(PGEN) from the default dependencies of $(GRAMMAR_H):

ifneq ($(cross_compiling),yes)
$(GRAMMAR_H): $(PGEN)
endif

If this is a more acceptable solution, one could probably rewrite the "test "$(cross_compiling" != "yes"; then..." part of the make rules from $(GRAMMAR_H) and $(GRAMMAR_C) with Make's ifeq, here's a patch for that instead (this also makes the dependencies more clear, since $(GRAMMAR_H) does not depend on $(GRAMMAR_INPUT) for the cross-input case, as it is not used):

=====
diff -u Python-2.7.12/Makefile.pre.in Python-2.7.12-fix/Makefile.pre.in
--- Python-2.7.12/Makefile.pre.in	2016-06-25 23:49:31.000000000 +0200
+++ Python-2.7.12-fix/Makefile.pre.in	2016-07-12 00:37:43.000000000 +0200
@@ -680,22 +680,21 @@
 
 Modules/pwdmodule.o: $(srcdir)/Modules/pwdmodule.c $(srcdir)/Modules/posixmodule.h
 
+ifeq ($(cross_compiling),yes)
+$(GRAMMAR_H): $(srcdir)/Include/graminit.h
+	@$(MKDIR_P) Include
+	cp $(srcdir)/Include/graminit.h $(GRAMMAR_H).tmp
+	mv $(GRAMMAR_H).tmp $(GRAMMAR_H)
+$(GRAMMAR_C): $(srcdir)/Python/graminit.c
+	cp $(srcdir)/Python/graminit.c $(GRAMMAR_C).tmp
+	mv $(GRAMMAR_C).tmp $(GRAMMAR_C)
+else
 $(GRAMMAR_H): $(GRAMMAR_INPUT) $(PGEN)
 	@$(MKDIR_P) Include
-	# Avoid copying the file onto itself for an in-tree build
-	if test "$(cross_compiling)" != "yes"; then \
-		$(PGEN) $(GRAMMAR_INPUT) $(GRAMMAR_H) $(GRAMMAR_C); \
-	else \
-		cp $(srcdir)/Include/graminit.h $(GRAMMAR_H).tmp; \
-		mv $(GRAMMAR_H).tmp $(GRAMMAR_H); \
-	fi
+	$(PGEN) $(GRAMMAR_INPUT) $(GRAMMAR_H) $(GRAMMAR_C)
 $(GRAMMAR_C): $(GRAMMAR_H)
-	if test "$(cross_compiling)" != "yes"; then \
-		touch $(GRAMMAR_C); \
-	else \
-		cp $(srcdir)/Python/graminit.c $(GRAMMAR_C).tmp; \
-		mv $(GRAMMAR_C).tmp $(GRAMMAR_C); \
-	fi
+	touch $(GRAMMAR_C)
+endif
 
 $(PGEN):	$(PGENOBJS)
 		$(CC) $(OPT) $(LDFLAGS) $(PGENOBJS) $(LIBS) -o $(PGEN)
=====

See also (slightly related, the "avoid running" part is fixed, but the "avoid building" part is not fixed): https://bugs.python.org/issue22625
msg270214 - (view) Author: Thomas Perl (thomas.perl) * Date: 2016-07-11 23:02
Also related: https://bugs.python.org/issue22359
msg270292 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-13 03:13
I don’t know the details of how Python uses CFLAGS etc, but according to <https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html#index-CFLAGS> and <https://www.gnu.org/software/make/manual/html_node/Catalogue-of-Rules.html#index-ld>, the general convention may be that CFLAGS and LDFLAGS are only for _compiler_ and _linker_ flags respectively, and CFLAGS may not be used for a link step.

Why don’t you either use CC="arm-none-eabi-gcc -mfloat-abi=hard", or LDFLAGS="-mfloat-abi=hard"? If these options work for you, perhaps what we really need is better documentation of how to build Python.

Ideally I would also like to make it so that compiling pgen is not necessary. But I think we can only use Gnu Make extensions if they are harmless in other operating systems (e.g. we have to keep the makefile compatible with BSD Make, though maybe it is okay to require Gnu Make for cross compiling).

In <https://bugs.python.org/issue26662#msg270162> I left some rambling thoughts on ways to make the file regeneration like pgen does less automatic and more up to the user to control.
msg270304 - (view) Author: Thomas Perl (thomas.perl) * Date: 2016-07-13 10:25
Yes setting "CC" would fix the problem, but i guess the CFLAGS issue was just the original symptom (and my first reaction to fixing it), whereas the underlying problem is that pgen gets built in cases where it shouldn't be built at all.

The solution with "ifeq" does seem to require GNU make, and since these conditionals would still appear in the Makefile even for non-cross-builds, we can't really use this if compatibility with non-GNU make is a requirement.

Based on http://gallium.inria.fr/blog/portable-conditionals-in-makefiles/, here is something that seems to work ($(PGEN_DEPS) will be $(PGEN) when cross_compiling=no, and empty when cross_compiling=yes, and configure.ac errors out if cross_compiling is "maybe", so we do not need to handle that case at the moment):

=====
diff -ru Python-2.7.12/Makefile.pre.in Python-2.7.12-fix/Makefile.pre.in
--- Python-2.7.12/Makefile.pre.in	2016-06-25 23:49:31.000000000 +0200
+++ Python-2.7.12-fix/Makefile.pre.in	2016-07-13 12:21:27.000000000 +0200
@@ -246,6 +246,8 @@
 ##########################################################################
 # Parser
 PGEN=		Parser/pgen$(EXE)
+PGEN_DEPS0=     ${cross_compiling:yes=}
+PGEN_DEPS=      ${PGEN_DEPS0:no=$(PGEN)}
 
 PSRCS=		\
 		Parser/acceler.c \
@@ -680,7 +682,7 @@
 
 Modules/pwdmodule.o: $(srcdir)/Modules/pwdmodule.c $(srcdir)/Modules/posixmodule.h
 
-$(GRAMMAR_H): $(GRAMMAR_INPUT) $(PGEN)
+$(GRAMMAR_H): $(GRAMMAR_INPUT) $(PGEN_DEPS)
 	@$(MKDIR_P) Include
 	# Avoid copying the file onto itself for an in-tree build
 	if test "$(cross_compiling)" != "yes"; then \
=====
msg270309 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2016-07-13 14:19
> the underlying problem is that pgen gets built in cases where it shouldn't

This statement does not take into account the following note of your initial post: "note that the same $(CFLAGS) needs to be added to the rule for $(BUILDPYTHON) if one wants to build that as well".

I agree with Martin that you should either use CC or set LDFLAGS to fix the problems that occur at the link stages of your cross-build.
msg270316 - (view) Author: Thomas Perl (thomas.perl) * Date: 2016-07-13 15:30
Adding "-mfloat-abi=hard" to LDFLAGS fixes the issue for me, and also allows $(BUILDPYTHON) to be built correctly in addition to $(PGEN).

So I guess the resolution to this issue is "works for me" (with setting CC or LDFLAGS properly for cross-compilation being the resolution/workaround).

Does it make sense to create a new bug "Do not build pgen when it's not going to be used" as follow-up to this discussion (with a patch similar to the one in http://bugs.python.org/msg270304)? If so, I'll create one. Or maybe there should be a generic configure flag "do not run any generators" (like https://bugs.python.org/issue26662#msg270162, but including not running pgen), with the flag only issuing warnings instead of failing.
msg270670 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-18 00:24
I am happy to repurpose this bug to improve how code generators like pgen work if you want. Or open a separate report if you think that is best.

My problem with the proposal involving PGEN_DEPS0 is that it relies on makefile syntax intended for changing filename suffixes, and it is hard to understand. I wonder if it is better to solve the problem with e.g. documentation and manually overriding it with ‘make PGEN_DEP="" ’.
msg270983 - (view) Author: Thomas Perl (thomas.perl) * Date: 2016-07-22 10:17
Repurposing this bug as "do not run pgen".

Documenting and using 'make PGEN_DEP=""' might also work; however, given that the configure script uses autoconf, and there's also code in place for PYTHON_FOR_BUILD, I've attached a patch that makes the PGEN dependency just a autoconf substitution -- this might make it clear and not depend on any make substitution features?

Patch attached against current Hg cpython default branch tip, a similar patch also applies against 2.7.
msg270985 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-22 11:44
This patch looks okay to me. I will commit it in a few days, unless anyone comes up with a better option.
msg271478 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-28 01:33
New changeset c6476003e67f by Martin Panter in branch '3.5':
Issue #27490: Do not build pgen when cross-compiling
https://hg.python.org/cpython/rev/c6476003e67f

New changeset 98df00834c58 by Martin Panter in branch 'default':
Issue #27490: Merge pgen cross-compile logic from 3.5
https://hg.python.org/cpython/rev/98df00834c58
msg271492 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-28 04:20
New changeset ec27886f675f by Martin Panter in branch '2.7':
Issue #27490: Do not build pgen when cross-compiling
https://hg.python.org/cpython/rev/ec27886f675f
msg271495 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-28 04:32
FWIW I suspect this technique could be expanded to prevent Programs/_freeze_importlib from being cross-compiled, if anyone was interested.
History
Date User Action Args
2016-07-28 04:32:55martin.pantersetstatus: open -> closed
resolution: fixed
messages: + msg271495

stage: patch review -> resolved
2016-07-28 04:20:04python-devsetmessages: + msg271492
2016-07-28 01:33:50python-devsetnosy: + python-dev
messages: + msg271478
2016-07-23 19:10:21ned.deilysetnosy: + doko
2016-07-22 11:44:21martin.pantersetstage: patch review
messages: + msg270985
versions: + Python 3.5
2016-07-22 10:17:35thomas.perlsetfiles: + pgen_dependencies.patch
keywords: + patch
messages: + msg270983

title: ARM cross-compile: pgen built without $(CFLAGS) as $(LIBRARY) dependency -> Do not run pgen when it is not going to be used (cross-compiling)
2016-07-18 00:24:09martin.pantersetmessages: + msg270670
2016-07-13 15:30:31thomas.perlsetmessages: + msg270316
2016-07-13 14:19:50xdegayesetnosy: + xdegaye
messages: + msg270309
2016-07-13 10:25:28thomas.perlsetmessages: + msg270304
2016-07-13 03:13:57martin.pantersetnosy: + martin.panter
messages: + msg270292
2016-07-11 23:02:23thomas.perlsetmessages: + msg270214
2016-07-11 22:42:18thomas.perlcreate