Differential D20386
Correct makefile.rules to use arm/aarch64 target specific AR and OBJCOPY Authored by omjavaid on May 18 2016, 2:23 PM.
Details
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.
Diff Detail
Event TimelineComment Actions This adds a lot of code duplication, which should be trivial to remove. Please address that first.
Comment Actions 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. Comment Actions 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
endifIt 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. Comment Actions 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. Comment Actions This will break a several use cases we are relying on. More details inline.
Comment Actions Updated with suggestion incorporated. Here's what I am getting on different inputs: Testing make clean CC=gcc Testing make clean CC=clang Testing make clean CC=gcc-5 Testing make clean CC=clang-3.8 Testing make clean CC=arm-linux-gnueabihf-gcc ARCH=arm Testing make clean CC=arm-linux-gnueabihf-gcc-5 ARCH=arm Testing make clean CC=/home/omair/work/toolchains/arm32-android-toolchain/bin/arm-linux-androideabi-gcc ARCH=arm Testing make clean CC=/home/omair/work/toolchains/arm32-android-toolchain/bin/arm-linux-androideabi-gcc-4.9 ARCH=arm Comment Actions Looks great, just give me a while to prepare the new paths on the buildbots. Should be done by tomorrow. Comment Actions 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... Comment Actions Ok. After some though I have come up with this:
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? Comment Actions 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. Comment Actions 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. Comment Actions so I have used ?= now with following new changes OBJCOPY ?= $(call replace_cc_with,objcopy) ARCHIVER ?= $(call replace_cc_with,ar) override AR = $(ARCHIVER) Comment Actions I have updated the buildbots to appropriately set these variables. Hopefully we will have a smooth transition. Thanks for your patience. | |||||||||||||||||||||||||||||||||||||