Correct makefile.rules to use arm/aarch64 target specific AR and OBJCOPY

[omjavaid]

Summary

This patch makes sure that we are using correct target specific ar and objcopy executables.

I have added logic to extract ar and objcopy from CC compiler name in case of arm and aarch64 targets.

In case of android this is overidden by android specific logic implemented in makefile.rules.

[labath]

This adds a lot of code duplication, which should be trivial to remove. Please address that first.

packages/Python/lldbsuite/test/make/Makefile.rules

164

This looks like it duplicates the replace_cc_with logic in the Android specific-code. Please move the macro to a common location and then use it here. I'd like to avoid adding more cryptic make code.

In fact, the android specific definitions can probably be removed after this goes in (we can do that as a followup if you don't have the ability to test it).

[omjavaid]

Sorry about the delay updating this. I lost track of this rev earlier.

Have updated diff to use macro already present within Android specific block for all cases.

Tested with no regressions on arm/aarch64 linux and android targets.

This possibly should address similar issues arising where there is architecture difference between host and target.

[tberghammer]

Pavel is OOO this week but the change looks good.

One request: Can you move the following part out of the android specific part as well?

ifdef PIE
    LDFLAGS += -pie
endif

It will make no difference at the moment but will make the code more generic and also it will get rid of some of the android specific hacks from this file.

[omjavaid]

I have updated previous patch which handles compiler binaries which have version string appended at the end like gcc-4.9 or clang-3.5.

Kindly give it a review and let me know if its breaking any buildbots.

[omjavaid]

reopening this for new review.

[omjavaid]

Adding context.

[labath]

This will break a several use cases we are relying on. More details inline.

packages/Python/lldbsuite/test/make/Makefile.rules

288

Please remove the if android part and move it to the generic section above.

294

This code will now be under "android-specific options" although it is not. Please move it up, above "windows specific options".

304

I think like we should replace ar with gcc-ar always, and not just when we don't have a version suffix present. When I have a gcc installed to a custom prefix, I get $prefix/gcc-ar, but not $prefix/ar. (Previously we would just use ar unconditionally, which was probably a bug.)

Next, when you specify a clang compiler as clang-3.5, you will produce ar-3.5, which almost certainly does not exist. I think that in case of clang we should just strip the version suffix (produce XXX-ar) and hope for the best (should work for all our current use cases).

Finally, I don't think objcopy is ever versioned with gcc (correct me if I am wrong), so I think that in case of objcopy we should strip the version suffix unconditionally XXX-objcopy.

So, to summarize, these are the transformations I think are wrong:
foo/gcc -> foo/ar (should be foo/gcc-ar)
clang-3.5 -> ar-3.5 (should be ar)
gcc-4.8 -> objcopy-4.8 (should be objcopy)

Let me know if these are compatible with your requirements. If we can't find a set of rules that work everywhere, we will have to abandon the magic (or maybe just leave a simple one), and require the user to specify the paths manually...

317

This will override the darwin-specific setting of AR on line 130, which it probably shouldn't. I guess this should be ifneq $(OS) darwin

[omjavaid]

packages/Python/lldbsuite/test/make/Makefile.rules

304

I checked with gcc people in my team and here is what they have to say.
omjavaid, gcc-ar is a version of ar with LTO support
omjavaid, ar is just ar
omjavaid, likewise with gcc-nm and gcc-ranlib
omjavaid, ar comes from binutils, gcc-ar comes from gcc

I agree with all other points you raised except for using gcc-ar in all cases. We should decide between ar (binutils) or gcc-ar (LTO support gcc packaged).

Also present code doesnt generate obcopy-4.8 for me when i specify gcc-4.8. If it does for you let me know.

[labath]

packages/Python/lldbsuite/test/make/Makefile.rules

304

I agree with all other points you raised except for using gcc-ar in all cases. We should decide between ar (binutils) or gcc-ar (LTO support gcc packaged).

Ok, makes sense. Since we presently do not care about LTO support. I propose we just go with ar always. It will make things predictible, as we will avoid the whole version mess (we can just strip it). It will also mean we can treat objcopy and ar the same way.
I.e., the transformation rule should be:
optional-path/optional-triple-gcc-optional.version -> optional-path/optional-triple-$TOOL.

This will not cover all of the cases for us right now, because in case of a gcc installed to a custom prefix, there will not be an ar next to it, but I can fix that by adding a couple of symlinks.

Would that work for you?

Also present code doesn't generate obcopy-4.8 for me when i specify gcc-4.8. If it does for you let me know.

It does not add the version suffix for me either. I must have got something wrong yesterday. Please ignore that remark.

[omjavaid]

Updated with suggestion incorporated.

Here's what I am getting on different inputs:

Testing make clean CC=gcc
ar
objcopy

Testing make clean CC=clang
ar
objcopy

Testing make clean CC=gcc-5
ar
objcopy

Testing make clean CC=clang-3.8
ar
objcopy

Testing make clean CC=arm-linux-gnueabihf-gcc ARCH=arm
arm-linux-gnueabihf-ar
arm-linux-gnueabihf-objcopy

Testing make clean CC=arm-linux-gnueabihf-gcc-5 ARCH=arm
arm-linux-gnueabihf-ar
arm-linux-gnueabihf-objcopy

Testing make clean CC=/home/omair/work/toolchains/arm32-android-toolchain/bin/arm-linux-androideabi-gcc ARCH=arm
/home/omair/work/toolchains/arm32-android-toolchain/bin/arm-linux-androideabi-ar
/home/omair/work/toolchains/arm32-android-toolchain/bin/arm-linux-androideabi-objcopy

Testing make clean CC=/home/omair/work/toolchains/arm32-android-toolchain/bin/arm-linux-androideabi-gcc-4.9 ARCH=arm
/home/omair/work/toolchains/arm32-android-toolchain/bin/arm-linux-androideabi-ar
/home/omair/work/toolchains/arm32-android-toolchain/bin/arm-linux-androideabi-objcopy

[labath]

Looks great, just give me a while to prepare the new paths on the buildbots. Should be done by tomorrow.

[labath]

Hm... there is one more problem here.. when we run the tests against a top-of-tree clang that we have just built, we will compute AR and OBJCOPY to be in the build directory. They will obviously not be there, and we cannot just create the symlinks there by hand because the directory gets wiped after every build. I'll need to think about this a bit...

[labath]

Ok. After some though I have come up with this:

• leave the tool computation logic as it is in the last version of your patch
• add the ability to override the tool values via environment variables. Basically this would mean using TOOL ?= value instead of TOOL = value. I don't really like that pattern but it is already used in a lot of places, so it's not anything new.

Then, we could use --env OBJCOPY=objcopy dotest.py argument to override the default tool computation logic, where it does not compute the correct values. There is only one gotcha here, and that is that make already provides a default value for AR, so AR ?= whatever would be a noop. Therefore, we would need to use a different variable name for the tool (ARCHIVE?). I don't think that the different name would be too confusing, since in the case of darwin we are already setting the value to libtool, which is in no relation to AR.

What do you think about that?

[omjavaid]

I like your suggestions and I dont think we have any other way but to use preset environment variable to detect what kind of TOOLCHAIN we want to use apart from some standard cases where we have the ability to detect through proposed hack logic.

I agree we should have the ability to override this hack as well just for the fact that it allows us to use various other versions of AR, OBJCOPY etc.

[labath]

Sounds that we're agreed then. Can you please update the patch to use ?= as appropriate? I will update the buildbots to set these variables explicitly as needed, and then we can get this in.

[omjavaid]

so I have used ?= now with following new changes

OBJCOPY ?= $(call replace_cc_with,objcopy)
ARCHIVER ?= $(call replace_cc_with,ar)
override AR = $(ARCHIVER)

[labath]

I have updated the buildbots to appropriately set these variables. Hopefully we will have a smooth transition.

Thanks for your patience.

packages/Python/lldbsuite/test/make/Makefile.rules

else
   endif
   ifeq "$(ARCH)" "powerpc"
     override ARCH := $(subst powerpc,32,$(ARCH))
   endif
   ifeq "$(ARCH)" "powerpc64"
     override ARCH := $(subst powerpc64,64,$(ARCH))
   endif
   ifeq "$(ARCH)" "aarch64"
     override ARCH :=
     override ARCHFLAG :=
   endif
   ifeq "$(ARCH)" "arm"
     override ARCH :=
     override ARCHFLAG :=
   endif
   ifeq "$(ARCH)" "s390x"
     override ARCH :=
cxx_linker_notdir = $(if $(findstring clang,$(1)), \
                               $(subst icc,icpc,$(1)), \
                               $(if $(findstring llvm-gcc,$(1)), \
                                    $(subst llvm-gcc,llvm-g++,$(1)), \
                                    $(if $(findstring gcc,$(1)), \
                                         $(subst gcc,g++,$(1)), \
                                         $(subst cc,c++,$(1))))))
 cxx_linker = $(if $(findstring /,$(1)),$(join $(dir $(1)), $(call cxx_linker_notdir,$(notdir $(1)))),$(call cxx_linker_notdir,$(1)))
 
-OBJCOPY := $(CROSS_COMPILE)objcopy
-
 #----------------------------------------------------------------------
 # Windows specific options
 #----------------------------------------------------------------------
 ifeq "$(OS)" "Windows_NT"
   ifneq (,$(findstring clang,$(CC)))
     # Clang for Windows doesn't support C++ Exceptions
     CXXFLAGS += -fno-exceptions
     CXXFLAGS += -D_HAS_EXCEPTIONS=0
     ifeq "$(VisualStudioVersion)" "14.0"
       CXXFLAGS += -fms-compatibility-version=19.0
     endif
     # The MSVC linker doesn't understand long section names
     # generated by the clang compiler.
     LDFLAGS += -fuse-ld=lld
   endif
 endif
 
 #----------------------------------------------------------------------
 # Android specific options
 #----------------------------------------------------------------------
 ifeq "$(OS)" "Android"
     ifdef PIE
         LDFLAGS += -pie
     endif
-    replace_with = $(if $(findstring clang,$(1)), \
-                        $(subst clang,$(2),$(1)), \
-                        $(if $(findstring gcc,$(1)), \
-                             $(subst gcc,$(2),$(1)), \
-                             $(subst cc,$(2),$(1))))
-    ifeq "$(notdir $(CC))" "$(CC)"
-        replace_cc_with = $(call replace_with,$(CC),$(1))
-    else
-        replace_cc_with = $(join $(dir $(CC)),$(call replace_with,$(notdir $(CC)),$(1)))
-    endif
-    OBJCOPY = $(call replace_cc_with,objcopy)
-    AR = $(call replace_cc_with,ar)
-endif
+endif
+
+CLANG_OR_GCC := $(strip $(if $(findstring clang,$(CC)), \
+                             $(findstring clang,$(CC)), \
+                             $(if $(findstring gcc,$(CC)), \
+                                  $(findstring gcc,$(CC)), \
+                                  cc)))
+
+CC_LASTWORD := $(strip $(lastword $(subst -, ,$(CC))))
+
+replace_with = $(strip $(if $(findstring $(3),$(CC_LASTWORD)), \
+                       $(subst $(3),$(1),$(2)), \
+                       $(if $(findstring ar,$(1)), \
+                            $(if $(findstring gcc,$(3)), \
+                                 $(subst gcc,gcc-ar,$(2)), \
+                                 $(subst $(3),$(1),$(2))), \
+                            $(subst $(3),$(1),$(subst -$(CC_LASTWORD),,$(2))))))
+
+    replace_cc_with = $(call replace_with,$(1),$(CC),$(CLANG_OR_GCC))
+    replace_cc_with = $(join $(dir $(CC)),$(call replace_with,$(1),$(notdir $(CC)),$(CLANG_OR_GCC)))
+
 
 #----------------------------------------------------------------------
 # C++ standard library options
 #----------------------------------------------------------------------
 ifeq (1,$(USE_LIBSTDCPP))
   # Clang requires an extra flag: -stdlib=libstdc++
   ifneq (,$(findstring clang,$(CC)))
     CXXFLAGS += -stdlib=libstdc++ -DLLDB_USING_LIBSTDCPP