[PATCH] D54379: Add Hurd toolchain support to Clang

This revision was automatically updated to reflect the committed changes. Closed by commit rC347833: Add Hurd target to Clang driver (2/2) (authored by kristina, committed by ). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/ https://reviews.llvm.org/D543

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina accepted this revision. kristina added a comment. This revision is now accepted and ready to land. Yes everything passes now, my bad for not noticing. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/ https://reviews.llvm.org/D54379 ___

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina accepted this revision as: kristina. kristina added a comment. Actually nevermind, I'll just create them myself, seems like a strange bug. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/ https://reviews.llvm.org/D54379 ___

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. 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? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new

[PATCH] D54379: Add Hurd toolchain support to Clang

sthibaul added a comment. 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. Reposi

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. 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

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. 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) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D54379: Add Hurd toolchain support to Clang

sthibaul added a comment. 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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D54379: Add Hurd toolchain support to Clang

sthibaul updated this revision to Diff 175188. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/ https://reviews.llvm.org/D54379 Files: lib/Basic/Targets.cpp lib/Basic/Targets/OSTargets.h lib/Driver/CMakeLists.txt lib/Driver/Driver.cpp lib/Driver/

[PATCH] D54379: Add Hurd toolchain support to Clang

sthibaul updated this revision to Diff 175186. sthibaul marked 4 inline comments as done. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/ https://reviews.llvm.org/D54379 Files: lib/Basic/Targets.cpp lib/Basic/Targets/OSTargets.h lib/Driver/CMakeList

[PATCH] D54379: Add Hurd toolchain support to Clang

sthibaul added a comment. (there still needs to be work on the test part) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/ https://reviews.llvm.org/D54379 ___ cfe-commits mailing list cfe-commits@lists.llvm.o

[PATCH] D54379: Add Hurd toolchain support to Clang

sthibaul marked 13 inline comments as done. sthibaul added a comment. I'll update the diff according to the comments. Comment at: lib/Driver/ToolChains/Hurd.cpp:74 + + // Similar to the logic for GCC above, if we currently running Clang inside + // of the requested system roo

[PATCH] D54379: Add Hurd toolchain support to Clang

aaron.ballman added a comment. I don't know enough about hurd to review those portions of it, but I have some general comments on the patch. Comment at: lib/Basic/Targets/OSTargets.h:279 +MacroBuilder &Builder) const override { +// Hurd defines; list ba

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. 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 look

[PATCH] D54379: Add Hurd toolchain support to Clang

sthibaul updated this revision to Diff 174536. sthibaul added a comment. 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? Repository: rC Clang https://revie

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. 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

[PATCH] D54379: Add Hurd toolchain support to Clang

sthibaul marked 5 inline comments as done. sthibaul added a comment. I believe this version handles all the comments. I could run this with check-all on a linux-amd64 box. Repository: rC Clang https://reviews.llvm.org/D54379 ___ cfe-commits maili

[PATCH] D54379: Add Hurd toolchain support to Clang

sthibaul updated this revision to Diff 174526. sthibaul edited the summary of this revision. Repository: rC Clang https://reviews.llvm.org/D54379 Files: lib/Basic/Targets.cpp lib/Basic/Targets/OSTargets.h lib/Driver/CMakeLists.txt lib/Driver/Driver.cpp lib/Driver/ToolChains/Clang.cpp

[PATCH] D54379: Add Hurd toolchain support to Clang

sthibaul added a comment. > 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 di

[PATCH] D54379: Add Hurd toolchain support to Clang

sthibaul marked 9 inline comments as done. sthibaul added inline comments. Comment at: lib/Basic/Targets/OSTargets.h:283 +Builder.defineMacro("__GLIBC__"); +Builder.defineMacro("__ELF__"); +if (Opts.POSIXThreads) kristina wrote: > `__MACH__` and `__HU

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. 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

[PATCH] D54379: Add Hurd toolchain support to Clang

rjmccall added inline comments. Comment at: lib/Driver/ToolChains/Hurd.cpp:84 + const Driver &D = getDriver(); + std::string SysRoot = computeSysRoot(); + kristina wrote: > Move semantics? Or is this guaranteed elision (I'm not sure)? If you're talking about th

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Also, this needs unit tests and FileCheck tests. Repository: rC Clang https://reviews.llvm.org/D54379 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added inline comments. Comment at: lib/Driver/Driver.cpp:418 + replaceString(T, "-pc-gnu", "-pc-hurd-gnu"); + TargetTriple = T; + kristina wrote: > Reference to a local variable? Hm, actually this is fine I guess, just avoid `strlen` and pass literals

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added inline comments. Comment at: lib/Driver/Driver.cpp:418 + replaceString(T, "-pc-gnu", "-pc-hurd-gnu"); + TargetTriple = T; + Reference to a local variable? Repository: rC Clang https://reviews.llvm.org/D54379 __

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. 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

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D54379 __

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Phab lost this inline command 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 https://reviews.llvm.org/D54378). Comment at: lib/Driver/Driver.cpp:

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added inline comments. Comment at: lib/Driver/Driver.cpp:392 +static void replaceString(std::string &S, + const char *Other, Same as previous comment regarding this type of function. Repository: rC Clang https://reviews.ll

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Added first batch of comments regarding the patch. Some style, some semantics-related. Comment at: lib/Basic/Targets/OSTargets.h:283 +Builder.defineMacro("__GLIBC__"); +Builder.defineMacro("__ELF__"); +if (Opts.POSIXThreads) --

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added reviewers: aaron.ballman, erik.pilkington, rsmith. kristina added a comment. 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