This is an archive of the discontinued LLVM Phabricator instance.

Add Multilib selection machinery to Clang
ClosedPublic

Authored by jroelofs on Jan 10 2014, 6:45 PM.

Details

Summary

This patch improves the support for picking Multilibs from gcc installations. It also provides a better approximation for the flags -print-multi-directory and -print-multi-lib.

Diff Detail

Event Timeline

gribozavr added inline comments.Jan 11 2014, 5:53 AM
lib/Driver/Multilib.cpp
34–48

There is no reason to put these constructors out-of-line.

silvas added inline comments.Jan 11 2014, 9:07 PM
lib/Driver/Multilib.cpp
34–48

Could default arguments reduce the boilerplate here?

atanasyan added inline comments.
include/clang/Driver/Multilib.h
37

I think it's ineffective to use std::string as a type of a function's argument. Maybe StringRef or a const reference to std::string?

73
  • It's better to pass Other by const reference.
  • Do not add a space between a function name and an open brace. You can run the clang-format on your patch to find and fix all formatting issues.
lib/Driver/ArgParse.h
1 ↗(On Diff #6420)

Is it really necessary to move all these functions to the separate h/cpp files in this commit?

lib/Driver/Driver.cpp
778

Maybe add a FIXME comment here to attract attention,

lib/Driver/Multilib.cpp
2

We need test cases cover the Multilib functionality.

176

It's potentially dangerous to join two string represent arbitrary paths. Can we trust that we never get double slashes // and no one slash at all at the point of joining?

lib/Driver/ToolChains.cpp
1563

Magical code on this line and below. Why do we compare DebianMipsMultilibs.size() and 3? Why do we combine Multilibs and FSFMipsMultilibs if FSFMipsMultilibs.size() > CSMipsMultilibs.size() etc?

jroelofs added inline comments.Jan 12 2014, 9:49 AM
include/clang/Driver/Multilib.h
37

Ok. StringRef would be better.

73

Oh, I didn't know I could run it on a patch... cool.

lib/Driver/ArgParse.h
1 ↗(On Diff #6420)

A few of these would end up being duplicated if I left them as static functions. Would it be better to do this in a separate commit? Do you have a better suggestion for the name of this file? (I don't really like 'ArgParse', but it's the best I could come up with).

lib/Driver/Driver.cpp
778

Ok

lib/Driver/Multilib.cpp
2

Like the ones in clang/unittests or clang/test? Both?

34–48

Ok, I'll combine them and put the declaration inline with the class.

176

Should I add assertions? Would it be better to use llvm/Support/Path.h for these suffixes?

lib/Driver/ToolChains.cpp
1563

Yes, this is a bit of 'Magic', but I'm not sure of a better solution. The problem here is that each of the MultilibSets overlap a little, so you get partial matches when looking at one installation and comparing it with the 'wrong' MultilibSet. I could add a comment explaining this, or maybe you a have a suggestion for a better approach :) ?

atanasyan requested changes to this revision.Jan 16 2014, 5:46 AM

Please update the patch against the current Clang source code.

include/clang/Driver/Multilib.h
84

The Clang shows the following warning here:

'clang::driver::MultilibSet::FilterCallback' has virtual functions but non-virtual destructor
lib/Driver/ArgParse.h
1 ↗(On Diff #6420)

As far as I can see all these functions except hasMipsN32ABIArg are used in the single file. Let's keep these function where they are used. As to hasMipsN32ABIArg we can declare it in the Tools.h file. There are namespace arm and namespace hexagon in this file. Let's create mips namespace and put the declaration to it.

lib/Driver/Multilib.cpp
2

I thought about new test in clang/test/Driver. But probably it's better to create a unit test for this class.

176

If there is some sort of a contract that say base suffix never ends by a slash and new suffix always starts by a slash we can put asserts here. If not it's better to try to use llvm::sys::path here.

lib/Driver/ToolChains.cpp
1563

Probably a comment would be enough.

jroelofs updated this revision to Unknown Object (????).Jan 21 2014, 10:14 PM

Addresses the review comments, and fixes a couple of nuances I realized while writing the unit tests.

jroelofs added inline comments.Jan 21 2014, 10:22 PM
tools/clang-format/clang-format-diff.py
34 ↗(On Diff #6564)

This file (tools/clang-format/clang-format-diff.py) shouldn't have been in the diff... oops.

unittests/Driver/Debug+Asserts/.dir
1 ↗(On Diff #6564)

This file shouldn't have been in the diff... oops.

unittests/Driver/Debug+Asserts/MultilibTest.d
1 ↗(On Diff #6564)

This file (unittests/Driver/Debug+Asserts/MultilibTest.d) shouldn't have been in the diff... oops.

unittests/Driver/Makefile
2

this line should read:

- unittests/Driver/Makefile ---------------------------*- Makefile -*-
atanasyan requested changes to this revision.Jan 29 2014, 2:09 AM

I'm sorry for the delay with review. In general the patch looks good to me.

  • Please cleanup the patch and remove unnecessary files from it.
  • Fix issues with -std=c++11 flag and LLVM_OVERRIDE macro. See inline comments below.
  • Ask Rafael Espindola or Chandler Carruth about review. The changes are rather significant and I would like to see more LGTM.
include/clang/Driver/Multilib.h
43

g++ shows an error on this line. It does not know about std::string::front() method if you do not specify -std=c++11 flag. Have you built the clang by g++ and using autotools configure script or CMake with default options?

53

Ditto

63

Ditto

75

Ditto

lib/Driver/Multilib.cpp
336

Why do you need LLVM_OVERRIDE here?

jroelofs added inline comments.Feb 3 2014, 4:02 PM
include/clang/Driver/Multilib.h
43

I've been using the configure script + make on a mac, where it uses Apple clang to build everything. Looks like the default flags are set slightly differently. I'll fix these and make sure it also builds on a linux box with g++.

lib/Driver/Multilib.cpp
74

Same thing about std::string::front() applies here.

Also, this should be '||', not 'xor', otherwise it could crash instead of failing the assert.

83

Same thing about std::string::front() applies here too.

336

Oh, you're right. This function isn't virtual. I'll remove it.

jroelofs updated this revision to Unknown Object (????).Feb 3 2014, 4:03 PM

Fixed issues from most recent round of review.

I didn't realize mail was bouncing to Rafael's other address. Hopefully this is the correct one?

atanasyan closed this revision.Mar 2 2014, 1:25 AM

Closed by commit rL201205 (authored by @jroelofs).