Page MenuHomePhabricator

Add Hurd toolchain support to Clang
ClosedPublic

Authored by sthibaul on Nov 10 2018, 6:03 AM.

Diff Detail

Repository
rC Clang

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

The Hurd::Hurd constructor would actually need to do the same gcc inclusion path detection as on Linux, but let's leave this aside for now, this commit is enough for a build without libc++.

kristina added inline comments.
lib/Driver/ToolChains/Clang.cpp
533–534

Please keep line lengths to 80 columns at most, it's one of the few hard rules outlined in the developer policy.

kristina added reviewers: kristina, Restricted Project.Nov 10 2018, 7:26 AM

A few style naming/comments.

lib/Driver/ToolChains/Hurd.cpp
37

Does this need a switch? Wouldn't an if be sufficient?

107

Variable names should be capitalized.

126

Default should generally be the first case, if present.

sthibaul marked 3 inline comments as done.Nov 10 2018, 7:59 AM

I commented one of them, and will fix the rest.

lib/Driver/ToolChains/Hurd.cpp
37

An if would work, yes, it's just to make it easily extensible for future cases, just like the getMultiarchTriple function in Linux.cpp

sthibaul updated this revision to Diff 173515.Nov 10 2018, 8:00 AM
krytarowski added inline comments.Nov 10 2018, 3:03 PM
lib/Driver/ToolChains/Hurd.cpp
137

Is this some hurd standard or personal taste?

sthibaul added inline comments.Nov 10 2018, 3:50 PM
lib/Driver/ToolChains/Hurd.cpp
137

I copied this from the Linux.cpp file.

Actually it happens to be a standard in the pure GNU system which does not define a /usr. Debian GNU/Hurd eventually migrated to having a real /usr just like other Debian ports to keep things coherent, but the GNU system is supposed to have prefix=/

sthibaul updated this revision to Diff 173599.Nov 11 2018, 4:44 PM

In this version, the Driver introduces the "-hurd-" part

sthibaul updated this revision to Diff 173601.Nov 11 2018, 4:53 PM
sthibaul updated this revision to Diff 173604.Nov 11 2018, 5:15 PM

This includes version finding the gcc Hurd triplet (i[3456]-gnu) in the Gcc detector.

kristina retitled this revision from Add Hurd support to Add Hurd toolchain support to Clang.Nov 11 2018, 6:24 PM
kristina edited the summary of this revision. (Show Details)

Alright, once this is fully reviewed, if accepted, I can land the LLVMSupport change and add your new target and close the stack. In the meantime, adding some more reviewers who have more experience with Clang CR than me and who can hopefully weigh in some opinions. (Sorry about the delays, looks like I'll be the one seeing addition of this target through, not landed the LLVMSupport change yet since I want to make sure this set of patches is good, otherwise the LLVMSupport change on its own holds little weight)

Added first batch of comments regarding the patch. Some style, some semantics-related.

lib/Basic/Targets/OSTargets.h
283

__MACH__ and __HURD__ seem appropriate? Apple Mach (Darwin/XNU) uses __MACH__ with __APPLE__, Hurd should probably follow a similar convention, I don't think there are many places aside from XNU build where __MACH__ is used on its own.

lib/Driver/ToolChains/Hurd.cpp
73

No space here.

79

I'm not quite sure I like this. Also early return should be for the "bad" case, not for the good case, at least IMO, this is not a huge issue but I'll see what others say. I think this may just be subjective.

85

Move semantics? Or is this guaranteed elision (I'm not sure)?

119

Until needed I wouldn't use an array here, also drop const.

kristina added inline comments.Nov 15 2018, 4:40 PM
lib/Driver/Driver.cpp
392

Same as previous comment regarding this type of function.

kristina requested changes to this revision.Nov 15 2018, 4:40 PM
This revision now requires changes to proceed.Nov 15 2018, 4:40 PM
kristina added a comment.EditedNov 15 2018, 4:48 PM

Phab lost this inline comment for some reason, but please leave some comment regarding why that part in Driver.cpp does what it does (summarize the conclusion of the discussion in D54378).

lib/Driver/Driver.cpp
405

Please comment the reasoning behind this (ie. the discussion in D54378) for any other maintainers.

The other lost comment was regarding the functions where you're using strcpy instead of idiomatic LLVM and also creating unnecessary temporary std::string instances on the stack.

Commented on that particular idiom, there's two instances where it's used, aside from variable naming issues (pos should be Pos) it's very non idiomatic as far as rest of LLVM code goes, don't pass string literals around as const char*, prefer StringRef instead, that would also save you from needing to resort to using strlen later (sorry for previous comment, I didn't mean strcpy).

lib/Driver/Driver.cpp
400

Please avoid that kind of code and avoid strlen, you should pass things as StringRef as that's the general way of doing things unless you have a good reason for doing so otherwise. This entire function/part that uses it should be rewritten, I especially don't like the temporary std::string on the stack. It may be worth considering SmallString which is a variation of SmallVector or just manipulating the StringRef. You very certainly don't need strlen however, StringRef provides the needed operators, same goes for using std::string::find, just use StringRef instead.

kristina added inline comments.Nov 16 2018, 1:41 AM
lib/Driver/Driver.cpp
407

Reference to a local variable?

kristina added inline comments.Nov 16 2018, 2:00 AM
lib/Driver/Driver.cpp
407

Hm, actually this is fine I guess, just avoid strlen and pass literals as StringRefs. I would make sure the triple actually matches before you construct a local though, otherwise you're forcing doing it for every target, which in majority of the cases isn't going to be Hurd. I would still use SmallString instead of std::string but that's more of a nitpick.

kristina added a comment.EditedNov 16 2018, 2:12 AM

Also, this needs unit tests and lit/FileCheck tests.

rjmccall added inline comments.Nov 16 2018, 7:44 PM
lib/Driver/ToolChains/Hurd.cpp
85

If you're talking about the initialization of SysRoot, yes, initializing a local variable from a temporary is a guaranteed copy-elision, and adding std::move will actually inhibit the optimization.

kristina added a comment.EditedNov 16 2018, 7:48 PM

As discussed in #hurd, a few additional comments. The Hurd codepath should first check if the triple is eligible, ie. minimizing any cost to the driver invocation for most targets, which are not going to be Hurd. In fact I would wrap it in LLVM_UNLIKELY but that's just a suggestion. Once you confirmed that the triple in question is the one you're looking for (from the suffix), you can alter the existing triple. The std::string should be avoided here since even a zero length SmallVector will guarantee not allocating anything unless used. This may be worth factoring out into a separate function entirely, and another important point is avoiding any sort of unneeded overhead unless you've confirmed that it's the Hurd triple.

In general, I've looked over it again and I'd ask to get rid of switches that can be replaced with if statements. If there's a need later, you can add it later. For now, there's no need so I would avoid it. Switch is not generally an easy construct for humans to parse. Avoid arrays with 1 element until needed as well please. You can always introduce further changes as they're needed, however, for now, I'm strongly against adding "clutter" because it may be useful in the future.

I'll end this comment with this: In general when structuring your code, the performance penalty for other targets when the conditions that can be easily tested are not met should pretty much be close to nonexistent. I would suggest keeping that in mind when submitting revisions.

sthibaul marked 9 inline comments as done.Nov 17 2018, 10:56 AM
sthibaul added inline comments.
lib/Basic/Targets/OSTargets.h
283

There is actually no __HURD__ macro, it's the __GNU__ macro which has that role. __MACH__ should however be there too indeed, as well as __gnu_hurd__ similarly to Linux' __gnu_linux__.

lib/Driver/ToolChains/Hurd.cpp
79

Well, this is inspired from clang/lib/Driver/ToolChains/Linux.cpp, which additionally has some gcc tests, which I'll include in a later patch. That argues for using this way of doing the test since that is how it will be in the end.

sthibaul marked 4 inline comments as done.Nov 17 2018, 10:57 AM

In general when structuring your code, the performance penalty for other targets when the conditions that can be easily tested are not met should pretty much be close to nonexistent. I would suggest keeping that in mind when submitting revisions.

I know, as discussed on IRC I just hadn't managed to find a way to achieve it in this case :)

But thanks to the IRC discussion I'll be able to do it.

sthibaul updated this revision to Diff 174526.Nov 17 2018, 7:15 PM
sthibaul edited the summary of this revision. (Show Details)
sthibaul marked 5 inline comments as done.EditedNov 17 2018, 7:18 PM

I believe this version addresses all the comments.
I could run this with check-all on a linux-amd64 box.

I'll re-review when I'm up, from a quick glance it looks much better but I'll have to patch it over my fork and try out a few things (Mostly x86_64 Linux and Darwin test suites). I think the test is lacking a bit, there's a lot of stuff that isn't covered, and there's a lack of negative tests (ie. when invalid input is supplied and matches the triple suffix). Feel free to run tests though, may find something before me, I'm a bit too tired to reconfigure my buildbot to do what I want right now, so I'll leave it until when I'm up. So yeah I may be being a bit nitpicky but I think tests could cover a little bit more.

It would also be great to get at least one other Clang reviewer to sign off on this. I can sign off on this myself once I test it, but I feel like getting another set of eyes would be good. But yeah if this is all good and someone else can also skim through this and sign off on it, I can commit the stack when I'm up and when I've done some stuff. If you can, try to build/test with a recent Clang as that usually brings out some benign warnings one may miss if using an older SDK.

But very good job in general, this seems a lot better and streamlined than the previous revision.

So that said to other Clang reviewers: Gentle ping, would really love a second set of eyes though, though it can wait until Monday at worst since I'd imagine a lot of people are off right now.

Thank you.

sthibaul updated this revision to Diff 174536.Nov 18 2018, 5:24 AM

I have added a few checks (the ld.so dynamic linker specification, the ../lib32 paths, and /usr/lib/i386-gnu)

About negative tests, what kind of invalid input are you thinking about?

Ping. Would like someone else to co-review this, everything looks fine aside from tests (I think a portion of this could use a short unit test with regards to creating the actual target instance). Also your FileCheck test is only for invoking clang -cc1 from the looks, can you add coverage for static linker invocations (as driver is also generally used to invoke the linker, unless that's explicitly not the case on Hurd)?

I don't know enough about hurd to review those portions of it, but I have some general comments on the patch.

lib/Basic/Targets/OSTargets.h
279

Missing full stop at the end of the comment.

lib/Driver/ToolChains/Hurd.cpp
66–67

Formatting looks off here as well.

75

What GCC logic above?

we currently -> we are currently

121

This doesn't look unreachable to me?

159

Spurious newline.

162

Can elide braces.

lib/Driver/ToolChains/Hurd.h
23–24

Formatting looks off here; did clang-format produce this?

32–34

Why are these virtual functions?

getDynamicLinker() is implemented but never called in this patch -- is it needed?

36

This appears to be entirely unused.

sthibaul marked 13 inline comments as done.Nov 25 2018, 2:50 PM

I'll update the diff according to the comments.

lib/Driver/ToolChains/Hurd.cpp
75

That was from Linux.cpp. I have not yet included the gcc pieces above, so the comment doesn't make sense yet indeed.

121

For now it is, we only have x86 architecture for the Hurd. This is like Linux::getDynamicLinker which uses llvm_unreachable in the default case.

lib/Driver/ToolChains/Hurd.h
32–34

I don't know the rationale, I am just reusing the same principle as in Linux.{h,cpp}.

getDynamicLinker is used by Gnu.cpp's ConstructJob.

36

Again, it is used by Gnu.cpp's ConstructJob

sthibaul updated this revision to Diff 175186.Nov 25 2018, 2:50 PM
sthibaul marked 4 inline comments as done.

(there still needs to be work on the test part)

sthibaul updated this revision to Diff 175188.Nov 25 2018, 3:42 PM

I have added static and shared library tests

a short unit test with regards to creating the actual target instance)

I'm not sure what you mean? The tests create an actual binary for the target.

Alright, will patch it into my fork in a bit and try it out, if everything looks good, I'll land the stack. (Again huge sorry about this taking some time, have lots on my hands at the moment)

Your tests don't pass:

********************
Testing Time: 21.51s
********************
Failing Tests (1):
    Clang :: Driver/hurd.c

  Expected Passes    : 470
  Expected Failures  : 3
  Unsupported Tests  : 71
  Unexpected Failures: 1

Seems to be an issue with this particular path:

/q/src/llvm-tainted-8.0/tools/clang/test/Driver/hurd.c:9:11: error: CHECK: expected string not found in input
// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"

I don't have this issue with make check-all in clang/. Just to make sure: are the .keep files of the patch getting created? Otherwise the i386-gnu/ directories will not get created and then it's not surprising that clang doesn't add them to the research paths.

Hm, yes you're right, the files aren't in the diff that Phab gives me, this is annoying. I wonder if it's stripping them from the diff for some reason, can you upload the raw diff?

kristina accepted this revision as: kristina.Nov 28 2018, 7:08 PM

Actually nevermind, I'll just create them myself, seems like a strange bug.

kristina accepted this revision.Nov 28 2018, 7:09 PM

Yes everything passes now, my bad for not noticing.

This revision is now accepted and ready to land.Nov 28 2018, 7:09 PM
This revision was automatically updated to reflect the committed changes.