pirama updated this revision to Diff 88864.
pirama added a comment.
Fix typo.
https://reviews.llvm.org/D30015
Files:
include/clang/Driver/ToolChain.h
lib/Driver/ToolChain.cpp
lib/Driver/Tools.cpp
test/Driver/Inputs/resource_dir_with_arch_subdir/lib/linux/aarch64/.keep
Hahnfeld added inline comments.
Comment at: test/Driver/arch-specific-libdir-rpath.c:6
+// -rpath only gets added during native compilation
+// REQUIRES: native
+//
mgorny wrote:
> Hahnfeld wrote:
> > pirama wrote:
> > > pirama wrote:
> > > > I feel this test is
mgorny added a comment.
Thanks. The -L tests look good, -rpath is not perfect but I don't think you can
improve it without additional changes to the Driver.
Comment at: test/Driver/arch-specific-libdir-rpath.c:6
+// -rpath only gets added during native compilation
+//
Hahnfeld added a comment.
Please adapt the title and summary for the more general changes this has
evolved to.
Comment at: lib/Driver/Tools.cpp:284
+// If we are not cross-compling, add '-rpath' with architecture-specific
+// library path so libraries lined from this
pirama added inline comments.
Comment at: test/Driver/arch-specific-libdir-rpath.c:6
+// -rpath only gets added during native compilation
+// REQUIRES: native
+//
pirama wrote:
> I feel this test is fragile. Any idea how to further restrict and require
> that
pirama updated this revision to Diff 88799.
pirama added a comment.
Stricter tests.
https://reviews.llvm.org/D30015
Files:
include/clang/Driver/ToolChain.h
lib/Driver/ToolChain.cpp
lib/Driver/Tools.cpp
test/Driver/Inputs/resource_dir_with_arch_subdir/lib/linux/aarch64/.keep
mgorny added inline comments.
Comment at: test/Driver/arch-specific-libdir.c:6
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_arch_subdir \
+// RUN: | FileCheck --check-prefix=CHECK-ARCHDIR %s
+//
Please be more specific in the tests, i.e. check if the
pirama added inline comments.
Comment at: test/Driver/arch-specific-libdir-rpath.c:6
+// -rpath only gets added during native compilation
+// REQUIRES: native
+//
I feel this test is fragile. Any idea how to further restrict and require that
the default target
pirama updated this revision to Diff 88768.
pirama added a comment.
- Arch-subdir is now always added to -L
- It is added to -rpath during native compilation.
- Tests have been updated
https://reviews.llvm.org/D30015
Files:
include/clang/Driver/ToolChain.h
lib/Driver/ToolChain.cpp
Hahnfeld added a comment.
Maybe something like the following (without tests):
diff --git a/include/clang/Driver/ToolChain.h
b/include/clang/Driver/ToolChain.h
index ffb0d60..0f3507d 100644
--- a/include/clang/Driver/ToolChain.h
+++ b/include/clang/Driver/ToolChain.h
@@ -299,6 +299,11
Hahnfeld added a comment.
In https://reviews.llvm.org/D30015#678294, @pirama wrote:
> I am fine to do this more generally, but not sure of the scope. Should this
> be always added or only if a runtime (sanitizer or openmp or xray) is
> requested?
I think always. From my point of view, this
pirama added a comment.
I am fine to do this more generally, but not sure of the scope. Should this be
always added or only if a runtime (sanitizer or openmp or xray) is requested?
https://reviews.llvm.org/D30015
___
cfe-commits mailing list
Hahnfeld added a comment.
I still think this should not be done for OpenMP only and hence move out of
`addOpenMPRuntime`.
(Why the heck are there //two// places to add `-lomp`?!? I'll clean that up
later...)
https://reviews.llvm.org/D30015
___
pirama added a subscriber: openmp-commits.
pirama marked an inline comment as done.
pirama added inline comments.
Comment at: lib/Driver/Tools.cpp:3267
+ if (llvm::sys::fs::is_directory(CandidateLibPath))
+CmdArgs.push_back(Args.MakeArgString("-L" + CandidateLibPath));
+
pirama updated this revision to Diff 88663.
pirama added a comment.
Use getArchTypeName() instead of getArchName().
https://reviews.llvm.org/D30015
Files:
include/clang/Driver/ToolChain.h
lib/Driver/ToolChain.cpp
lib/Driver/Tools.cpp
mgorny added inline comments.
Comment at: lib/Driver/Tools.cpp:3267
+ if (llvm::sys::fs::is_directory(CandidateLibPath))
+CmdArgs.push_back(Args.MakeArgString("-L" + CandidateLibPath));
+
Don't you also need rpath for it? Or is this purely for static
mgorny added inline comments.
Comment at: lib/Driver/ToolChain.cpp:327
+ llvm::sys::path::append(Path, "lib", OSLibName,
+ getArchName());
+ return Path.str();
I would suggest using arch type, to avoid e.g. i386/i486/i586 mess. It's
pirama updated this revision to Diff 88662.
pirama added a comment.
Changed the name of the utility that builds the arch-specific directory.
https://reviews.llvm.org/D30015
Files:
include/clang/Driver/ToolChain.h
lib/Driver/ToolChain.cpp
lib/Driver/Tools.cpp
Hahnfeld added a subscriber: mgorny.
Hahnfeld added a comment.
Yes, that's the current state. We had some discussion on cfe-dev on that
matter: http://lists.llvm.org/pipermail/cfe-dev/2017-January/052512.html
Could you slightly adapt your patch so that other runtime libraries can follow
this
pirama added a comment.
In https://reviews.llvm.org/D30015#678243, @Hahnfeld wrote:
> Why is this only for OpenMP? I imagine this should be done for **all**
> runtime libraries
The other runtime libraries have the arch included in the library's name and
the driver links the correct file.
Hahnfeld added a comment.
Why is this only for OpenMP? I imagine this should be done for **all** runtime
libraries
https://reviews.llvm.org/D30015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
srhines added inline comments.
Comment at: include/clang/Driver/ToolChain.h:304
+ // Returns /lib//. When -fopenmp is specified,
+ // this directory is added to the linker search path if it exists
+ const std::string getOpenMPLibPath() const;
Add a period at
danalbert accepted this revision.
danalbert added a comment.
This revision is now accepted and ready to land.
LGTM, but should probably get signoff from someone else as well.
https://reviews.llvm.org/D30015
___
cfe-commits mailing list
23 matches
Mail list logo
