Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conditionally disable command line length work-around in Makefile.in #2098

Open
jaganmn opened this issue Oct 19, 2024 · 7 comments
Open

Conditionally disable command line length work-around in Makefile.in #2098

jaganmn opened this issue Oct 19, 2024 · 7 comments

Comments

@jaganmn
Copy link

jaganmn commented Oct 19, 2024

The work-around in question is here:

flint/Makefile.in

Lines 411 to 430 in 74907c3

ifneq ($(SHARED), 0)
shared: $(FLINT_DIR)/$(FLINT_LIB_FULL)
# The following is to avoid reaching the maximum length of command line
# arguments, mainly present on MinGW.
define xxx_merged_lobj_rule
$(BUILD_DIR)/$(1)_merged.lo: $($(1)_LOBJS) | $(BUILD_DIR)
@$(LD) -r $($(1)_LOBJS) -o $(BUILD_DIR)/$(1)_merged.lo
endef
$(foreach dir, $(DIRS), $(eval $(call xxx_merged_lobj_rule,$(dir))))
MERGED_LOBJS:=$(foreach dir, $(DIRS),$(BUILD_DIR)/$(dir)_merged.lo)
$(FLINT_DIR)/$(FLINT_LIB_FULL): $(MERGED_LOBJS)
@echo "Building $(FLINT_LIB_FULL)"
@$(CC) $(CFLAGS) -shared $(EXTRA_SHARED_FLAGS) $(MERGED_LOBJS) -o $(FLINT_LIB_FULL) $(LDFLAGS) $(LIBS)
@$(RM_F) $(FLINT_LIB)
@$(RM_F) $(FLINT_LIB_MAJOR)
@$(LN_S) $(FLINT_LIB_FULL) $(FLINT_LIB)
@$(LN_S) $(FLINT_LIB_FULL) $(FLINT_LIB_MAJOR)
endif

Unfortunately, not all linkers support the -r option. Notably the MinGW backend for lld, the LLVM linker, does not support it, preventing cross-compiling of FLINT under Unix for aarch64 Windows: https://github.com/llvm/llvm-project/blob/main/lld/MinGW/Options.td

Can this work-around be used only on Windows and systems where it is known to be necessary?

@albinahlback
Copy link
Collaborator

They must have something to merge files, no?

@jaganmn
Copy link
Author

jaganmn commented Oct 19, 2024

IIUC, on Windows one would use lib.exe to build object libraries from sets of object files, then use link.exe to link the object libraries. Since the MinGW backend of lld was designed to emulate link.exe, there need not be a simple replacement for ld -r.

Disabling the work-around on systems without known command line length limitations seems safest/easiest.

And, to be clear, we are just talking about one of the lld backends. The ELF backend fully supports -r.

@albinahlback
Copy link
Collaborator

I'm sure I am misunderstanding something; you would like to disable merging of files for MinGW, that one system that actually requires it in order compile FLINT?

@jaganmn
Copy link
Author

jaganmn commented Oct 19, 2024

No. We are running make under a Linux without limitations on command line length. However, because we are cross-compiling for Windows (building on Linux binaries to be distributed on Windows), we are using as our linker the MinGW backend of lld. This linker does not support -r, so we are hitting a linker error under Linux when building FLINT for Windows.

The proposal is to "merge" only if the system running make is known to have limitations on command line length.

@albinahlback
Copy link
Collaborator

I have thought about this for a little bit, and I do not think this is an issue about FLINT. I believe this is a pretty standard feature of linkers, and so I would first like to see this discussed at LLVM before trying to solve it here.

However, I am still opening to merging relevant PRs that may resolve this issue.

@jaganmn
Copy link
Author

jaganmn commented Oct 21, 2024

OK. In the mean time, we will work around this with a patch against the latest release (3.1.3-p1). I've just tested the following patch under each of x86_64-pc-linux-gnu and aarch64-apple-darwin23.6.0:

diff -ruN flint-3.1.3-p1/Makefile.in flint-3.1.3-p1-patched/Makefile.in
--- flint-3.1.3-p1/Makefile.in	2024-05-24 08:52:40
+++ flint-3.1.3-p1-patched/Makefile.in	2024-10-20 23:32:18
@@ -416,6 +416,10 @@
 ifneq ($(SHARED), 0)
 shared: $(FLINT_DIR)/$(FLINT_LIB_FULL)
 
+ifneq ($(strip $(filter gnu% linux-gnu%,@FLINT_BUILD_OS@)),)
+# No command line length limitations under build_os=gnu*|linux-gnu*
+MERGED_LOBJS := $(foreach dir,$(DIRS),$($(dir)_LOBJS))
+else
 # The following is to avoid reaching the maximum length of command line
 # arguments, mainly present on MinGW.
 define xxx_merged_lobj_rule
@@ -424,6 +428,7 @@
 endef
 $(foreach dir, $(DIRS), $(eval $(call xxx_merged_lobj_rule,$(dir))))
 MERGED_LOBJS:=$(foreach dir, $(DIRS),$(BUILD_DIR)/$(dir)_merged.lo)
+endif
 
 $(FLINT_DIR)/$(FLINT_LIB_FULL): $(MERGED_LOBJS)
 	@echo "Building $(FLINT_LIB_FULL)"
@@ -437,6 +442,10 @@
 ifneq ($(STATIC), 0)
 static: $(FLINT_DIR)/$(FLINT_LIB_STATIC)
 
+ifneq ($(strip $(filter gnu% linux-gnu%,@FLINT_BUILD_OS@)),)
+# No command line length limitations under build_os=gnu*|linux-gnu*
+MERGED_OBJS := $(foreach dir,$(DIRS),$($(dir)_OBJS))
+else
 # The following is to avoid reaching the maximum length of command line
 # arguments, mainly present on MinGW.
 define xxx_merged_obj_rule
@@ -445,6 +454,7 @@
 endef
 $(foreach dir, $(DIRS), $(eval $(call xxx_merged_obj_rule,$(dir))))
 MERGED_OBJS:=$(foreach dir, $(DIRS),$(BUILD_DIR)/$(dir)_merged.o)
+endif
 
 $(FLINT_DIR)/$(FLINT_LIB_STATIC): $(MERGED_OBJS)
 	@echo "Building $(FLINT_LIB_STATIC)"
diff -ruN flint-3.1.3-p1/configure.ac flint-3.1.3-p1-patched/configure.ac
--- flint-3.1.3-p1/configure.ac	2024-05-24 08:52:40
+++ flint-3.1.3-p1-patched/configure.ac	2024-10-20 23:25:02
@@ -129,7 +129,11 @@
 
 dnl Get system triplet
 dnl NOTE: This is already invoked from LT_INIT
+dnl AC_CANONICAL_BUILD
 dnl AC_CANONICAL_HOST
+
+FLINT_BUILD_OS="${build_os}"
+AC_SUBST(FLINT_BUILD_OS)
 
 ################################################################################
 # configure headers

It differs from my proposal by "merging" except under GNU/Linux, rather than only under MinGW.

@kalibera
Copy link

For reference, the error output from the linker is simply

lld: error: unknown argument: -r

Asking for -r to be supported just based on problems with command line length limits might be a hard sell. Perhaps instead of merging object files, you could iteratively keep adding them to a static library, and later use that one for linking?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants