[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/D54379
Files:
lib/Basic/Targets.cpp
lib/Basic/Targets/OSTargets.h
lib/Driver/CMakeLists.txt
lib/Driver/Driver.cpp
lib/Driver/ToolChains/Clang.cpp
lib/Driver/ToolChains/Gnu.cpp
lib/Driver/ToolChains/Hurd.cpp
lib/Driver/ToolChains/Hurd.h
lib/Frontend/InitHeaderSearch.cpp
test/Driver/Inputs/basic_hurd_tree/include/.keep
test/Driver/Inputs/basic_hurd_tree/lib/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/lib32/.keep
test/Driver/Inputs/basic_hurd_tree/usr/include/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/usr/lib/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/usr/lib32/.keep
test/Driver/hurd.c
Index: test/Driver/hurd.c
===
--- test/Driver/hurd.c
+++ test/Driver/hurd.c
@@ -0,0 +1,62 @@
+// RUN: %clang -no-canonical-prefixes %s -### 2>&1 \
+// RUN: --target=i386-pc-gnu \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN: | FileCheck --check-prefix=CHECK %s
+// CHECK-NOT: warning:
+// CHECK: "-cc1"
+// CHECK: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK: "-dynamic-linker" "/lib/ld.so"
+// CHECK: "crtbegin.o"
+// CHECK: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/lib"
+// CHECK: "-L[[SYSROOT]]/usr/lib"
+
+// RUN: %clang -no-canonical-prefixes %s -### 2>&1 \
+// RUN: --target=i386-pc-gnu -static \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN: | FileCheck --check-prefix=CHECK-STATIC %s
+// CHECK-STATIC-NOT: warning:
+// CHECK-STATIC: "-cc1"
+// CHECK-STATIC: "-static-define"
+// CHECK-STATIC: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-STATIC: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK-STATIC: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK-STATIC: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK-STATIC: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK-STATIC: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-STATIC: "-static"
+// CHECK-STATIC: "crtbeginT.o"
+// CHECK-STATIC: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK-STATIC: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK-STATIC: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK-STATIC: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK-STATIC: "-L[[SYSROOT]]/lib"
+// CHECK-STATIC: "-L[[SYSROOT]]/usr/lib"
+
+// RUN: %clang -no-canonical-prefixes %s -### 2>&1 \
+// RUN: --target=i386-pc-gnu -shared \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN: | FileCheck --check-prefix=CHECK-SHARED %s
+// CHECK-SHARED-NOT: warning:
+// CHECK-SHARED: "-cc1"
+// CHECK-SHARED: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-SHARED: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK-SHARED: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK-SHARED: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK-SHARED: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK-SHARED: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-SHARED: "crtbeginS.o"
+// CHECK-SHARED: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK-SHARED: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK-SHARED: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK-SHARED: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK-SHARED: "-L[[SYSROOT]]/lib"
+// CHECK-SHARED: "-L[[SYSROOT]]/usr/lib"
Index: lib/Driver/ToolChains/Hurd.cpp
===
--- lib/Driver/ToolChains/Hurd.cpp
+++ lib/Driver/ToolChains/Hurd.cpp
@@ -0,0 +1,169 @@
+//===--- Hurd.cpp - Hurd ToolChain Implementations *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Hurd.h"
+#include "CommonArgs.h"
+#include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
+#include "clang/Driver/Options.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
+
+using namespace clang::driver;
+using namespace clang::driver::toolchains;
+using namespace clang;
+using namespace llvm::opt;
+
+using tools::addPathIfExists;
+
+/// Get our best guess at the multiarch triple for
[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 ___ 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 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 ___ 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 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/ 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
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. 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.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 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" 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.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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/D54379/new/ 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
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.org/D54379/new/ 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
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/ToolChains/Clang.cpp
lib/Driver/ToolChains/Gnu.cpp
lib/Driver/ToolChains/Hurd.cpp
lib/Driver/ToolChains/Hurd.h
lib/Frontend/InitHeaderSearch.cpp
test/Driver/Inputs/basic_hurd_tree/include/.keep
test/Driver/Inputs/basic_hurd_tree/lib/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/lib32/.keep
test/Driver/Inputs/basic_hurd_tree/usr/include/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/usr/lib/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/usr/lib32/.keep
test/Driver/hurd.c
Index: test/Driver/hurd.c
===
--- test/Driver/hurd.c
+++ test/Driver/hurd.c
@@ -0,0 +1,62 @@
+// RUN: %clang -no-canonical-prefixes %s -### 2>&1 \
+// RUN: --target=i386-pc-gnu \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN: | FileCheck --check-prefix=CHECK %s
+// CHECK-NOT: warning:
+// CHECK: "-cc1"
+// CHECK: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK: "-dynamic-linker" "/lib/ld.so"
+// CHECK: "crtbegin.o"
+// CHECK: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/lib"
+// CHECK: "-L[[SYSROOT]]/usr/lib"
+
+// RUN: %clang -no-canonical-prefixes %s -### 2>&1 \
+// RUN: --target=i386-pc-gnu -static \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN: | FileCheck --check-prefix=CHECK-STATIC %s
+// CHECK-STATIC-NOT: warning:
+// CHECK-STATIC: "-cc1"
+// CHECK-STATIC: "-static-define"
+// CHECK-STATIC: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-STATIC: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK-STATIC: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK-STATIC: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK-STATIC: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK-STATIC: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-STATIC: "-static"
+// CHECK-STATIC: "crtbeginT.o"
+// CHECK-STATIC: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK-STATIC: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK-STATIC: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK-STATIC: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK-STATIC: "-L[[SYSROOT]]/lib"
+// CHECK-STATIC: "-L[[SYSROOT]]/usr/lib"
+
+// RUN: %clang -no-canonical-prefixes %s -### 2>&1 \
+// RUN: --target=i386-pc-gnu -shared \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN: | FileCheck --check-prefix=CHECK-SHARED %s
+// CHECK-SHARED-NOT: warning:
+// CHECK-SHARED: "-cc1"
+// CHECK-SHARED: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-SHARED: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK-SHARED: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK-SHARED: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK-SHARED: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK-SHARED: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-SHARED: "crtbeginS.o"
+// CHECK-SHARED: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK-SHARED: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK-SHARED: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK-SHARED: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK-SHARED: "-L[[SYSROOT]]/lib"
+// CHECK-SHARED: "-L[[SYSROOT]]/usr/lib"
Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderSearch.cpp
+++ lib/Frontend/InitHeaderSearch.cpp
@@ -260,6 +260,7 @@
switch (os) {
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
llvm_unreachable("Include management is handled in the driver.");
@@ -412,6 +413,7 @@
switch (os) {
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
llvm_unreachable("Include management is handled in the driver.");
break;
@@ -460,6 +462,7 @@
break; // Everything else continues to use this routine's logic.
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
return;
Index: lib/Driver/ToolChains/Hurd.h
===
--- lib/Driver/ToolChains/Hurd.h
+++ lib/Driver/ToolChains/Hurd.h
@@ -0,0 +1,46 @@
+//===--- Hurd.h - Hurd ToolChain Implementations --*- C++ -*-===//
+//
+// The LLVM Compiler
[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/CMakeLists.txt
lib/Driver/Driver.cpp
lib/Driver/ToolChains/Clang.cpp
lib/Driver/ToolChains/Gnu.cpp
lib/Driver/ToolChains/Hurd.cpp
lib/Driver/ToolChains/Hurd.h
lib/Frontend/InitHeaderSearch.cpp
test/Driver/Inputs/basic_hurd_tree/include/.keep
test/Driver/Inputs/basic_hurd_tree/lib/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/lib32/.keep
test/Driver/Inputs/basic_hurd_tree/usr/include/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/usr/lib/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/usr/lib32/.keep
test/Driver/hurd.c
Index: test/Driver/hurd.c
===
--- test/Driver/hurd.c
+++ test/Driver/hurd.c
@@ -0,0 +1,20 @@
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=i386-pc-gnu \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN: | FileCheck --check-prefix=CHECK %s
+// CHECK-NOT: warning:
+// CHECK: "{{.*}}clang{{(.exe)?}}"
+// CHECK: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK: "-dynamic-linker" "/lib/ld.so"
+// CHECK: "crtbegin.o"
+// CHECK: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/lib"
+// CHECK: "-L[[SYSROOT]]/usr/lib"
Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderSearch.cpp
+++ lib/Frontend/InitHeaderSearch.cpp
@@ -260,6 +260,7 @@
switch (os) {
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
llvm_unreachable("Include management is handled in the driver.");
@@ -412,6 +413,7 @@
switch (os) {
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
llvm_unreachable("Include management is handled in the driver.");
break;
@@ -460,6 +462,7 @@
break; // Everything else continues to use this routine's logic.
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
return;
Index: lib/Driver/ToolChains/Hurd.h
===
--- lib/Driver/ToolChains/Hurd.h
+++ lib/Driver/ToolChains/Hurd.h
@@ -0,0 +1,46 @@
+//===--- Hurd.h - Hurd ToolChain Implementations --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
+#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
+
+#include "Gnu.h"
+#include "clang/Driver/ToolChain.h"
+
+namespace clang {
+namespace driver {
+namespace toolchains {
+
+class LLVM_LIBRARY_VISIBILITY Hurd : public Generic_ELF {
+public:
+ Hurd(const Driver , const llvm::Triple ,
+ const llvm::opt::ArgList );
+
+ bool HasNativeLLVMSupport() const override;
+
+ void
+ AddClangSystemIncludeArgs(const llvm::opt::ArgList ,
+llvm::opt::ArgStringList ) const override;
+
+ virtual std::string computeSysRoot() const;
+
+ virtual std::string getDynamicLinker(const llvm::opt::ArgList ) const;
+
+ std::vector ExtraOpts;
+
+protected:
+ Tool *buildAssembler() const override;
+ Tool *buildLinker() const override;
+};
+
+} // end namespace toolchains
+} // end namespace driver
+} // end namespace clang
+
+#endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
Index: lib/Driver/ToolChains/Hurd.cpp
===
--- lib/Driver/ToolChains/Hurd.cpp
+++ lib/Driver/ToolChains/Hurd.cpp
@@ -0,0 +1,169 @@
+//===--- Hurd.cpp - Hurd ToolChain Implementations *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Hurd.h"
+#include "CommonArgs.h"
+#include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
+#include "clang/Driver/Options.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
+
+using namespace clang::driver;
[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.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 root, add its parent library paths to
aaron.ballman wrote:
> What GCC logic above?
>
> we currently -> we are currently
That was from Linux.cpp. I have not yet included the gcc pieces above, so the
comment doesn't make sense yet indeed.
Comment at: lib/Driver/ToolChains/Hurd.cpp:120
+
+ llvm_unreachable("unsupported architecture");
+}
aaron.ballman wrote:
> This doesn't look unreachable to me?
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.
Comment at: lib/Driver/ToolChains/Hurd.h:31-33
+ virtual std::string computeSysRoot() const;
+
+ virtual std::string getDynamicLinker(const llvm::opt::ArgList ) const;
aaron.ballman wrote:
> Why are these virtual functions?
>
> `getDynamicLinker()` is implemented but never called in this patch -- is it
> needed?
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.
Comment at: lib/Driver/ToolChains/Hurd.h:35
+
+ std::vector ExtraOpts;
+
aaron.ballman wrote:
> This appears to be entirely unused.
Again, it is used by Gnu.cpp's ConstructJob
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.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ) const override {
+// Hurd defines; list based off of gcc output
+DefineStd(Builder, "unix", Opts);
Missing full stop at the end of the comment.
Comment at: lib/Driver/ToolChains/Hurd.cpp:65-66
+
+Hurd::Hurd(const Driver , const llvm::Triple ,
+ const ArgList )
+: Generic_ELF(D, Triple, Args) {
Formatting looks off here as well.
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 root, add its parent library paths to
What GCC logic above?
we currently -> we are currently
Comment at: lib/Driver/ToolChains/Hurd.cpp:120
+
+ llvm_unreachable("unsupported architecture");
+}
This doesn't look unreachable to me?
Comment at: lib/Driver/ToolChains/Hurd.cpp:158
+ // target triple.
+
+ if (getTriple().getArch() == llvm::Triple::x86) {
Spurious newline.
Comment at: lib/Driver/ToolChains/Hurd.cpp:161
+std::string Path = SysRoot + "/usr/include/i386-gnu";
+if (D.getVFS().exists(Path)) {
+ addExternCSystemInclude(DriverArgs, CC1Args, Path);
Can elide braces.
Comment at: lib/Driver/ToolChains/Hurd.h:22-23
+public:
+ Hurd(const Driver , const llvm::Triple ,
+ const llvm::opt::ArgList );
+
Formatting looks off here; did clang-format produce this?
Comment at: lib/Driver/ToolChains/Hurd.h:31-33
+ virtual std::string computeSysRoot() const;
+
+ virtual std::string getDynamicLinker(const llvm::opt::ArgList ) const;
Why are these virtual functions?
`getDynamicLinker()` is implemented but never called in this patch -- is it
needed?
Comment at: lib/Driver/ToolChains/Hurd.h:35
+
+ std::vector ExtraOpts;
+
This appears to be entirely unused.
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 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 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)? 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
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://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
lib/Driver/ToolChains/Gnu.cpp
lib/Driver/ToolChains/Hurd.cpp
lib/Driver/ToolChains/Hurd.h
lib/Frontend/InitHeaderSearch.cpp
test/Driver/Inputs/basic_hurd_tree/include/.keep
test/Driver/Inputs/basic_hurd_tree/lib/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/lib32/.keep
test/Driver/Inputs/basic_hurd_tree/usr/include/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/usr/lib/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/usr/lib32/.keep
test/Driver/hurd.c
Index: test/Driver/hurd.c
===
--- test/Driver/hurd.c
+++ test/Driver/hurd.c
@@ -0,0 +1,20 @@
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=i386-pc-gnu \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN: | FileCheck --check-prefix=CHECK %s
+// CHECK-NOT: warning:
+// CHECK: "{{.*}}clang{{(.exe)?}}"
+// CHECK: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK: "-dynamic-linker" "/lib/ld.so"
+// CHECK: "crtbegin.o"
+// CHECK: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/lib"
+// CHECK: "-L[[SYSROOT]]/usr/lib"
Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderSearch.cpp
+++ lib/Frontend/InitHeaderSearch.cpp
@@ -260,6 +260,7 @@
switch (os) {
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
llvm_unreachable("Include management is handled in the driver.");
@@ -412,6 +413,7 @@
switch (os) {
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
llvm_unreachable("Include management is handled in the driver.");
break;
@@ -460,6 +462,7 @@
break; // Everything else continues to use this routine's logic.
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
return;
Index: lib/Driver/ToolChains/Hurd.h
===
--- lib/Driver/ToolChains/Hurd.h
+++ lib/Driver/ToolChains/Hurd.h
@@ -0,0 +1,46 @@
+//===--- Hurd.h - Hurd ToolChain Implementations --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
+#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
+
+#include "Gnu.h"
+#include "clang/Driver/ToolChain.h"
+
+namespace clang {
+namespace driver {
+namespace toolchains {
+
+class LLVM_LIBRARY_VISIBILITY Hurd : public Generic_ELF {
+public:
+ Hurd(const Driver , const llvm::Triple ,
+ const llvm::opt::ArgList );
+
+ bool HasNativeLLVMSupport() const override;
+
+ void
+ AddClangSystemIncludeArgs(const llvm::opt::ArgList ,
+llvm::opt::ArgStringList ) const override;
+
+ virtual std::string computeSysRoot() const;
+
+ virtual std::string getDynamicLinker(const llvm::opt::ArgList ) const;
+
+ std::vector ExtraOpts;
+
+protected:
+ Tool *buildAssembler() const override;
+ Tool *buildLinker() const override;
+};
+
+} // end namespace toolchains
+} // end namespace driver
+} // end namespace clang
+
+#endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
Index: lib/Driver/ToolChains/Hurd.cpp
===
--- lib/Driver/ToolChains/Hurd.cpp
+++ lib/Driver/ToolChains/Hurd.cpp
@@ -0,0 +1,172 @@
+//===--- Hurd.cpp - Hurd ToolChain Implementations *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Hurd.h"
+#include "CommonArgs.h"
+#include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
+#include "clang/Driver/Options.h"
[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 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. 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
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 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
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
lib/Driver/ToolChains/Gnu.cpp
lib/Driver/ToolChains/Hurd.cpp
lib/Driver/ToolChains/Hurd.h
lib/Frontend/InitHeaderSearch.cpp
test/Driver/Inputs/basic_hurd_tree/include/.keep
test/Driver/Inputs/basic_hurd_tree/lib/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/usr/include/i386-gnu/.keep
test/Driver/Inputs/basic_hurd_tree/usr/lib/.keep
test/Driver/hurd.c
Index: test/Driver/hurd.c
===
--- test/Driver/hurd.c
+++ test/Driver/hurd.c
@@ -0,0 +1,16 @@
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=i386-pc-gnu \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN: | FileCheck --check-prefix=CHECK %s
+// CHECK-NOT: warning:
+// CHECK: "{{.*}}clang{{(.exe)?}}"
+// CHECK: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK: "crtbegin.o"
+// CHECK: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/lib"
+// CHECK: "-L[[SYSROOT]]/usr/lib"
Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderSearch.cpp
+++ lib/Frontend/InitHeaderSearch.cpp
@@ -260,6 +260,7 @@
switch (os) {
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
llvm_unreachable("Include management is handled in the driver.");
@@ -412,6 +413,7 @@
switch (os) {
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
llvm_unreachable("Include management is handled in the driver.");
break;
@@ -460,6 +462,7 @@
break; // Everything else continues to use this routine's logic.
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
return;
Index: lib/Driver/ToolChains/Hurd.h
===
--- lib/Driver/ToolChains/Hurd.h
+++ lib/Driver/ToolChains/Hurd.h
@@ -0,0 +1,46 @@
+//===--- Hurd.h - Hurd ToolChain Implementations --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
+#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
+
+#include "Gnu.h"
+#include "clang/Driver/ToolChain.h"
+
+namespace clang {
+namespace driver {
+namespace toolchains {
+
+class LLVM_LIBRARY_VISIBILITY Hurd : public Generic_ELF {
+public:
+ Hurd(const Driver , const llvm::Triple ,
+ const llvm::opt::ArgList );
+
+ bool HasNativeLLVMSupport() const override;
+
+ void
+ AddClangSystemIncludeArgs(const llvm::opt::ArgList ,
+llvm::opt::ArgStringList ) const override;
+
+ virtual std::string computeSysRoot() const;
+
+ virtual std::string getDynamicLinker(const llvm::opt::ArgList ) const;
+
+ std::vector ExtraOpts;
+
+protected:
+ Tool *buildAssembler() const override;
+ Tool *buildLinker() const override;
+};
+
+} // end namespace toolchains
+} // end namespace driver
+} // end namespace clang
+
+#endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
Index: lib/Driver/ToolChains/Hurd.cpp
===
--- lib/Driver/ToolChains/Hurd.cpp
+++ lib/Driver/ToolChains/Hurd.cpp
@@ -0,0 +1,172 @@
+//===--- Hurd.cpp - Hurd ToolChain Implementations *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Hurd.h"
+#include "CommonArgs.h"
+#include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
+#include "clang/Driver/Options.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
+
+using namespace clang::driver;
+using namespace clang::driver::toolchains;
+using namespace clang;
+using namespace llvm::opt;
+
+using tools::addPathIfExists;
+
+/// Get our best guess at the multiarch triple for a target.
+///
+/// Debian-based systems are starting to use a multiarch setup where they use
+/// a target-triple directory in the library and header search
[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 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. 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
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 `__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.
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__`.
Comment at: lib/Driver/ToolChains/Hurd.cpp:78
+
+ return std::string();
+}
kristina wrote:
> 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.
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.
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 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 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 before when submitting revisions. 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
rjmccall added inline comments. Comment at: lib/Driver/ToolChains/Hurd.cpp:84 + const Driver = getDriver(); + std::string SysRoot = computeSysRoot(); + kristina wrote: > Move semantics? Or is this guaranteed elision (I'm not sure)? 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. 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 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 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. 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; + Reference to a local variable? 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 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, that would also save you from needing to resort to using `strlen` later (sorry for previous comment, I didn't mean `strcpy`). Comment at: lib/Driver/Driver.cpp:400 + + S.replace(pos, strlen(Other), Replace); +} 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. 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 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 ___ 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 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:416 + std::string T = TargetTriple; + replaceString(T, "-unknown-gnu", "-unknown-hurd-gnu"); + replaceString(T, "-pc-gnu", "-pc-hurd-gnu"); Please comment the reasoning behind this (ie. the discussion in D54378) for any other maintainers. 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:392 +static void replaceString(std::string , + const char *Other, Same as previous comment regarding this type of function. 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 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)
`__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.
Comment at: lib/Driver/ToolChains/Hurd.cpp:72
+ const ArgList )
+: Generic_ELF(D, Triple, Args) { }
+
No space here.
Comment at: lib/Driver/ToolChains/Hurd.cpp:78
+
+ return std::string();
+}
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.
Comment at: lib/Driver/ToolChains/Hurd.cpp:84
+ const Driver = getDriver();
+ std::string SysRoot = computeSysRoot();
+
Move semantics? Or is this guaranteed elision (I'm not sure)?
Comment at: lib/Driver/ToolChains/Hurd.cpp:118
+ const StringRef X86MultiarchIncludeDirs[] = {
+ "/usr/include/i386-gnu"};
+
Until needed I wouldn't use an array here, also drop const.
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 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 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) 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
