Details
- Reviewers
rengolin kristina aaron.ballman erik.pilkington rsmith rjmccall - Group Reviewers
Restricted Project - Commits
- rG77a4adc4f914: Add Hurd target to Clang driver (2/2)
rL347833: Add Hurd target to Clang driver (2/2)
rC347833: Add Hurd target to Clang driver (2/2)
Diff Detail
- Repository
- rC Clang
Event Timeline
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++.
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. |
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 |
lib/Driver/ToolChains/Hurd.cpp | ||
---|---|---|
137 | Is this some hurd standard or personal taste? |
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=/ |
This includes version finding the gcc Hurd triplet (i[3456]-gnu) in the Gcc detector.
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. |
lib/Driver/Driver.cpp | ||
---|---|---|
392 | Same as previous comment regarding this type of function. |
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. |
lib/Driver/Driver.cpp | ||
---|---|---|
407 | Reference to a local variable? |
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. |
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. |
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.
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. |
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.
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.
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. |
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 |
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?
Missing full stop at the end of the comment.