This is an archive of the discontinued LLVM Phabricator instance.

Add android-ndk-standalone test; add armv7 and thumb include and lib paths
ClosedPublic

Authored by chh on May 24 2016, 3:23 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

chh updated this revision to Diff 58330.May 24 2016, 3:23 PM
chh retitled this revision from to Update basic_android_tree and add armv7 include and lib paths.
chh updated this object.

Hi Chih-Hung,

Thanks for working on this, it could really help cross compilation.

I have a few inline comments, mostly to use the target parser instead of string parsing, but overall, looks good.

I'm also interested why you changed the version of the GCC (to 4.9). Does that change in any way from previous versions, or is that just to common up 4.4.3 and 4.8 into a single one? If it is, why not just keep 4.8 and avoid spurious changes?

cheers,
--renato

lib/Driver/ToolChains.cpp
1476 ↗(On Diff #58330)

Why the /* static */ comment? Looks unnecessary. :)

1488 ↗(On Diff #58330)

Please, use

if (ARM::parseArchVersion(Arch) == 7)
1495 ↗(On Diff #58330)

We don't want to encourage "thumb" arches, and the TargetParser already deals with that (via parseArchISA), so you can simplify this method considerably by:

bool Thumb = Args.hasFlag(options::OPT_mthumb, options::OPT_mno_thumb, false) ||
             ARM::parseArchISA(Arch) == ARM::IK_THUMB;
1499 ↗(On Diff #58330)

Same thing here... just change Thumb and create a Version variable and use them in your decision making, it should reduce to a single if / else if / else.

4398 ↗(On Diff #58330)

Same thing here, use the target parser.

chh added a comment.May 25 2016, 10:06 AM

Renato,

Thanks for the suggestions. I will upload a new patch using the target parser.

About the test input files and gcc versions, I made those changes to match current Android NDK file structure, which uses GCC 4.9. The version number is less important but file locations are different in newer NDK, or they were just not there in current test input. They are necessary for newer test cases to check if clang driver finds header file and library directories. I hope this patch does not affect other platforms.

In D20600#439441, @chh wrote:

About the test input files and gcc versions, I made those changes to match current Android NDK file structure, which uses GCC 4.9. The version number is less important but file locations are different in newer NDK, or they were just not there in current test input. They are necessary for newer test cases to check if clang driver finds header file and library directories. I hope this patch does not affect other platforms.

Oh, the patch certainly *will* affect all Linux/GCC platforms, but I'm counting on it. So, please, don't remove the old tests, only add new ones. 4.9 may be the new NDK, but people could still be using the old one, even on Android. We'd also need a non-Android test, to make sure this works well on plain Linux, too. It should be fine to test on x86_64 only.

Once the patch is ready, I'll test in on my side (various Linaro toolchains, ARM, AArch64, Arch Linux) and see if it behaves well.

cheers,
--renato

Adding some people that have touched the search path / toolchain part of the Clang driver, as this may change more things than it intends, and I'm a bit rusty with that code.

chh updated this revision to Diff 58519.May 25 2016, 3:09 PM
chh retitled this revision from Update basic_android_tree and add armv7 include and lib paths to Add android-ndk-standalone test; add armv7 and thumb include and lib paths.
chh updated this object.
chh marked 4 inline comments as done.
chh added inline comments.
lib/Driver/ToolChains.cpp
1476 ↗(On Diff #58330)

It looks like some coding style. I found it used in CollectLibDirsAndTriples and Linux::GCCVersion::Parse.

1488 ↗(On Diff #58330)

I changed it to also check SubArch, in case -march flag is not used:

llvm::ARM::parseArchVersion(Arch) == 7 ||
(Triple.getSubArch() == llvm::Triple::ARMSubArch_v7 && Arch == "")
atanasyan edited edge metadata.May 25 2016, 3:17 PM
  • You do not need to add .keep file to the folder if this folder contains sub-folders or files. For example test/Driver/Inputs/basic_android_ndk_tree/include/c++/4.9/mipsel-linux-android/.keep is redundant.
  • I am not sure but maybe something like basic_android_tree_v2 is better than basic_android_ndk_tree because in that case it is clear how to name new directories tree when NDK will change it in the future once again.
  • Did you try to use MultilibSet infrastructure to select sub-libraries? It looks like MultilibSet AndroidMipsMultilibs in the ToolChains.cpp does very similar things.
chh added a comment.May 26 2016, 10:16 AM

When I started adding armv7-a library subdirectories, I tried to minimize changes and impacts. So I avoided MultilibSet. Now to handle more vaiants and include directories, I am going to try MultilibSet but limit to Android's ARM target.

I will remove redundant .keep files in the next patch.

The Android NDK tree changes were not big although NDK is now release 12 going 13. I think no one has planned big or incompatible change in the future yet. So I thought it will be better just use one basic_android_ndk_tree test tree and add more test cases to it in the future. The old basic_android_tree test has fewer cases and I was thinking about replacing it in my first patch. We can remove it later.

In D20600#441114, @chh wrote:

The Android NDK tree changes were not big although NDK is now release 12 going 13. I think no one has planned big or incompatible change in the future yet. So I thought it will be better just use one basic_android_ndk_tree test tree and add more test cases to it in the future. The old basic_android_tree test has fewer cases and I was thinking about replacing it in my first patch. We can remove it later.

OK. Sounds reasonable.

rengolin added inline comments.May 26 2016, 2:00 PM
lib/Driver/ToolChains.cpp
1501 ↗(On Diff #58519)

Still looks like you're repeating too many lines... I think you have to get all possibilities into as few variables as possible, and then have only one if/else. For example:

llvm::StringRef Arch = Args.getLastArgValue(options::OPT_march_EQ);
bool Thumb =
         Args.hasFlag(options::OPT_mthumb, options::OPT_mno_thumb, false) ||
         llvm::ARM::parseArchISA(Arch) == llvm::ARM::IK_THUMB ||
         Triple.getArch() == llvm::Triple::thumb;
bool V7 = (Arch && llvm::ARM::parseArchVersion(Arch) == 7) ||
          (Triple.getSubArch() == llvm::Triple::ARMSubArch_v7);

if (V7 && Thumb)
  SubLibDirs.append(begin(ARMV7aThumbLibDirs), end(ARMV7aThumbLibDirs));
else if (V7)
  SubLibDirs.append(begin(ARMV7aLibDirs), end(ARMV7aLibDirs));
else if (Thumb)
  SubLibDirs.append(begin(ARMThumbLibDirs), end(ARMThumbLibDirs));
4401 ↗(On Diff #58519)

Same comments here about bundling up all V7s and Thumbs into the same variable, and using only once.

chh updated this revision to Diff 58731.May 26 2016, 5:58 PM
chh edited edge metadata.
chh added a comment.May 26 2016, 6:00 PM

New diff3 uses Multilib and should be much simpler than previous patches.
Please take a look again. Thanks.

atanasyan added inline comments.May 26 2016, 10:19 PM
lib/Driver/ToolChains.cpp
2272 ↗(On Diff #58731)

Using Multilibs you can simplify the code for selection. Take a look at findBiarchMultilibs or findMIPSMultilibs. The idea is to specify folders used by each library and flags which trigger selection of this library. Note that the flags do not have to match to command line flags one-to-one. You define them by yourself. Then you join all defined libraries together either in a plain array or using Either and Maybe "operators". After that you define set of flags (addMultilibFlag) and call MultilibSet::select only once. You do not have to check existence of file paths explicitly. The MultilibSet::select does it for you.

chh updated this revision to Diff 58861.May 27 2016, 4:17 PM

Now use MultilibSet.

atanasyan accepted this revision.May 27 2016, 9:54 PM
atanasyan edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 27 2016, 9:54 PM
rengolin requested changes to this revision.May 27 2016, 9:57 PM
rengolin edited edge metadata.

Wait, this is not an Android-only change and can benefit ARM Linux, too. Please wait until I get to test it properly on a cross-linux environment.

Thanks!
--renato

This revision now requires changes to proceed.May 27 2016, 9:57 PM

Also, do you really need to change the executable bit on the fake tools? I think you should rollback all of the state changes, so that we don't erroneously mark them as changed, when they actually not.

chh updated this revision to Diff 59108.May 31 2016, 12:03 PM
chh edited edge metadata.

Do not set executable bit of fake executable files in test input.

chh added a comment.May 31 2016, 12:10 PM

I set executable bits the same way as the old basic_android_tree input.
I think they are unnecessary so now they are removed.

Please check if this patch works for other ARM toolchains.
Since I only tested the Android NDK toolchain, I felt safer to make this change only to Android.
Thanks.

So, it seems that on Linux, there are slight changes. For instance, it's not "armv7-a" but "armv7-ar". Despite of that, the mechanism is the same, and everything else seems similar.

Can I encourage you to remove the "Android" from the name, just call it "ARMMultilibs" and have a property (local variable) that changes where it's isAndroid() or not?

chh added a comment.Jun 1 2016, 11:40 AM

Renato,

I am not familiar with Linux ARM toolchain tree structure
and cannot find "armv7-ar" in clang source or test files either.
In fact I am not sure if my current change will cause any trouble
to non-Android ARM toolchain. So I added the Android qualification.

Would it be okay with you to have incremental changes?
If we have this patch first, it should be easier for you or anyone
to modify it to work with other toolchains, right?
I suppose a new toolchain test and input tree could be added and
the driver code could have a ARMMultilibs that works for multiple
toolchains, if that's possible with Multilibs.
Or we have one Multiblibs for each toolchain.

rengolin accepted this revision.Jun 1 2016, 11:42 AM
rengolin edited edge metadata.

I agree. Let's do incremental changes with this one. I'll see if we can pick up this one later on Linux.

Thanks! LGTM.

This revision is now accepted and ready to land.Jun 1 2016, 11:42 AM
This revision was automatically updated to reflect the committed changes.