Page MenuHomePhabricator

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

Authored by omjavaid on May 18 2016, 2:23 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

omjavaid updated this revision to Diff 57679.May 18 2016, 2:23 PM
omjavaid retitled this revision from to Correct makefile.rules to use arm/aarch64 target specific AR and OBJCOPY.
omjavaid updated this object.
omjavaid added reviewers: labath, tberghammer.
omjavaid added a subscriber: lldb-commits.
labath requested changes to this revision.May 19 2016, 3:48 AM
labath edited edge metadata.

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

packages/Python/lldbsuite/test/make/Makefile.rules
165 ↗(On Diff #57679)

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).

This revision now requires changes to proceed.May 19 2016, 3:48 AM
omjavaid updated this revision to Diff 66258.Jul 31 2016, 5:12 PM
omjavaid edited edge metadata.

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 edited edge metadata.Aug 1 2016, 5:38 AM

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.

tberghammer accepted this revision.Aug 1 2016, 5:38 AM
tberghammer edited edge metadata.
This revision was automatically updated to reflect the committed changes.
omjavaid updated this revision to Diff 67228.Aug 8 2016, 3:04 PM
omjavaid edited edge metadata.
omjavaid removed rL LLVM as the repository for this revision.

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 reopened this revision.Aug 8 2016, 3:04 PM

reopening this for new review.

omjavaid updated this revision to Diff 67229.Aug 8 2016, 3:07 PM

Adding context.

labath requested changes to this revision.Aug 9 2016, 6:24 AM
labath edited edge metadata.

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

packages/Python/lldbsuite/test/make/Makefile.rules
288 ↗(On Diff #67229)

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

294 ↗(On Diff #67229)

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

304 ↗(On Diff #67229)

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 ↗(On Diff #67229)

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

This revision now requires changes to proceed.Aug 9 2016, 6:24 AM
omjavaid added inline comments.Aug 9 2016, 1:18 PM
packages/Python/lldbsuite/test/make/Makefile.rules
304 ↗(On Diff #67229)

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 added inline comments.Aug 10 2016, 4:07 AM
packages/Python/lldbsuite/test/make/Makefile.rules
304 ↗(On Diff #67229)

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 this revision to Diff 67669.Aug 11 2016, 3:35 AM
omjavaid edited edge metadata.

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

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

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...

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?

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.

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 updated this revision to Diff 68339.Aug 17 2016, 5:27 AM
omjavaid edited edge metadata.

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 accepted this revision.Aug 17 2016, 7:19 AM
labath edited edge metadata.

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

Thanks for your patience.

This revision is now accepted and ready to land.Aug 17 2016, 7:19 AM
This revision was automatically updated to reflect the committed changes.