Page MenuHomePhabricator

Support Sourcery CodeBench MIPS toolchain
ClosedPublic

Authored by atanasyan on Apr 9 2013, 2:16 AM.

Details

Reviewers
rafael
Summary

Sourcery CodeBench and modern FSF Mips toolchains require a bit more
complicated algorithm to calculate headers, libraries and sysroot
paths than implemented by Clang driver now. The main problem is that all
these paths depend on a set of command line arguments additionally to a target
triple value. For example, let $TC is a toolchain installation directory.
If we compile big-endian 32-bit mips code, crtbegin.o is in
the $TC/lib/gcc/mips-linux-gnu/4.7.2 folder and the toolchain's linker
requires --sysroot=$TC/mips-linux-gnu/libc argument. If we compile
little-endian 32-bit soft-float mips code, crtbegin.o is in
the $TC/lib/gcc/mips-linux-gnu/4.7.2/soft-float/el folder and the toolchain's
linker requires --sysroot=$TC/mips-linux-gnu/libc/soft-float/el argument.

This patch supports Sourcery CodeBench toolchain directories tree.
The FSF's tree slightly differs and is not supported yet.

  1. Calculate MultiarchSuffix using all necessary command line options and use this MultiarchSuffix to detect crtbegin.o location in the GCCInstallationDetector::ScanLibDirForGCCTriple() routine.
  2. If a user does not provide --sysroot argument to the driver explicitly, calculate new sysroot value based on command line options. Then use this calculated sysroot path: a. Use this sysroot in the Linux::Linux() constructor to populate file search paths list. b. Use this sysroot in the Linux::AddClangSystemIncludeArgs() routine to find Mips toolchain specific include headers directories. c. Pass the sysroot argument to the linker.

Diff Detail

Event Timeline

atanasyan updated this revision to Unknown Object (????).Apr 16 2013, 10:49 PM

Added support for micromips targets.

Mostly nits. Sorry for taking so long to review this. Would you mind uploading an updated patch with the comments addressed. I will be much faster to review the next version.

lib/Driver/ToolChains.cpp
1125

mips-linux-gnu is used for both LE and BE?

1301–1310

s/get/append/ since this modifies the Path.

1304

Why?

1328

Can you please merge this with the above so that the entire funciton looks like

getTargetMultiarchSuffix(...) {
​ if (isMipsArch(TargetArch)) {

 getMipsTargetSuffix(..);
return ...;

}

// not mips case.

}

1332

s/get/append/

1339

Cant you merge this to getMipsTargetSuffix?

1343

Just

return llvm::sys::fs::exists(Path + Suffix.str() + "/crtbegin.o";

no?

2186

Checking both Sysroot and D.SysRoot is redundant given

​std::string Linux::computeSysRoot(const ArgList &Args) const {

		​  if (!getDriver().SysRoot.empty())
		​    return getDriver().SysRoot;

no?

Thanks for your review. I will send a new patch soon.

lib/Driver/ToolChains.cpp
1125

Yes, some Mips toolchains put everything into the different directories under mips-linux-gnu root. For example, big-endian stuff go to the mips-linux-gnu/<version>/ folder, while little-endian stuff go to the mips-linux-gnu/<version>/el/ folder

1301–1310

Agree. Fixed in the new patch.

1304

Because it was a getter function. I remove this code in the new patch.

1328

Done in the new patch.

1332

I suggest s/get/find/. This function searches valid path suffix and returns the search result.

1339

Unfortunately no. Some paths require suffix without 64 tail.

1343

No, because if the path does not exist we need to check another suffix.

2186

Good point. Fixed.

atanasyan updated this revision to Unknown Object (????).Apr 19 2013, 8:45 AM
  • Removed redundant checking
  • Renamed some functions
  • Rearranged code of getTargetMultiarchSuffix() routine.
rafael accepted this revision.Apr 19 2013, 12:29 PM

LGTM with the Twine issue fixed.

lib/Driver/ToolChains.cpp
2379

It is dangerous to use Twine like this. I think this case is safe, but a change to, for example, compute a string in getInstallPath instead of just returning a StringRef to a member would break this.

Can you add a trivial addExternCSystemIncludeIfExits with a Twine argument to avoid this? Or maybe addExternCSystemInclude itself could check if thee directory exit?

atanasyan added inline comments.Apr 19 2013, 2:04 PM
lib/Driver/ToolChains.cpp
2379

I prefer to make addExternCSystemIncludeIfExits(). It's a good idea to move path existence checking to the addExternCSystemInclude(). But I think this change should be made by a separate commit.

rafael closed this revision.Apr 24 2013, 10:15 AM

This was committed already.