Always use -Xlinker for -rpath

Currently we use `-Wl` which takes a list of
comma-separated options. Unfortunately that
breaks when you use it with `-rpath` and
a path that has commas in them.
Buck, the build system, produces paths with
commas in them.

`-Xlinker` doesn't have this disadvantage
and as far as I can tell is supported by
both `gcc` and `clang`. Anecdotally `nvcc`
supports `-Xlinker`, but not `-Wl`.

Test Plan: ./validate, harbourmaster

Reviewers: nomeata, simonmar, austin, bgamari, hvr

Reviewed By: simonmar, bgamari

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D2971

10 files changed, 93 insertions, 8 deletions

diff --git a/compiler/ghci/Linker.hs b/compiler/ghci/Linker.hs
index 9252489617..76c1cdafa2 100644
--- a/compiler/ghci/Linker.hs
+++ b/compiler/ghci/Linker.hs
@@ -880,18 +880,23 @@ dynLoadObjs hsc_env pls objs = do
                         concatMap
                             (\(lp, l) ->
                                  [ Option ("-L" ++ lp)
-                                 , Option ("-Wl,-rpath")
-                                 , Option ("-Wl," ++ lp)
+                                 , Option "-Xlinker"
+                                 , Option "-rpath"
+                                 , Option "-Xlinker"
+                                 , Option lp
                                  , Option ("-l" ++  l)
                                  ])
                             (temp_sos pls)
                         ++ concatMap
                              (\lp ->
                                  [ Option ("-L" ++ lp)
-                                 , Option ("-Wl,-rpath")
-                                 , Option ("-Wl," ++ lp)
+                                 , Option "-Xlinker"
+                                 , Option "-rpath"
+                                 , Option "-Xlinker"
+                                 , Option lp
                                  ])
                              minus_big_ls
+                        -- See Note [-Xlinker -rpath vs -Wl,-rpath]
                         ++ map (\l -> Option ("-l" ++ l)) minus_ls,
                       -- Add -l options and -L options from dflags.
                       --
diff --git a/compiler/main/DriverPipeline.hs b/compiler/main/DriverPipeline.hs
index a54e05c1fb..08af37cdda 100644
--- a/compiler/main/DriverPipeline.hs
+++ b/compiler/main/DriverPipeline.hs
@@ -1742,6 +1742,20 @@ getHCFilePackages filename =
 -- read any interface files), so the user must explicitly specify all
 -- the packages.
 
+{-
+Note [-Xlinker -rpath vs -Wl,-rpath]
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+-Wl takes a comma-separated list of options which in the case of
+-Wl,-rpath -Wl,some,path,with,commas parses the the path with commas
+as separate options.
+Buck, the build system, produces paths with commas in them.
+
+-Xlinker doesn't have this disadvantage and as far as I can tell
+it is supported by both gcc and clang. Anecdotally nvcc supports
+-Xlinker, but not -Wl.
+-}
+
 linkBinary :: DynFlags -> [FilePath] -> [InstalledUnitId] -> IO ()
 linkBinary = linkBinary' False
 
@@ -1770,8 +1784,9 @@ linkBinary' staticLink dflags o_files dep_packages = do
                             then "$ORIGIN" </>
                                  (l `makeRelativeTo` full_output_fn)
                             else l
+                  -- See Note [-Xlinker -rpath vs -Wl,-rpath]
                   rpath = if gopt Opt_RPath dflags
-                          then ["-Wl,-rpath",      "-Wl," ++ libpath]
+                          then ["-Xlinker", "-rpath", "-Xlinker", libpath]
                           else []
                   -- Solaris 11's linker does not support -rpath-link option. It silently
                   -- ignores it and then complains about next option which is -l<some
@@ -1781,7 +1796,7 @@ linkBinary' staticLink dflags o_files dep_packages = do
                   -- elf_begin: I/O error: region read: Is a directory
                   rpathlink = if (platformOS platform) == OSSolaris2
                               then []
-                              else ["-Wl,-rpath-link", "-Wl," ++ l]
+                              else ["-Xlinker", "-rpath-link", "-Xlinker", l]
               in ["-L" ++ l] ++ rpathlink ++ rpath
          | osMachOTarget (platformOS platform) &&
            dynLibLoader dflags == SystemDependent &&
@@ -1791,7 +1806,7 @@ linkBinary' staticLink dflags o_files dep_packages = do
                             then "@loader_path" </>
                                  (l `makeRelativeTo` full_output_fn)
                             else l
-              in ["-L" ++ l] ++ ["-Wl,-rpath", "-Wl," ++ libpath]
+              in ["-L" ++ l] ++ ["-Xlinker", "-rpath", "-Xlinker", libpath]
          | otherwise = ["-L" ++ l]
 
     let dead_strip = if osSubsectionsViaSymbols (platformOS platform)
diff --git a/compiler/main/SysTools.hs b/compiler/main/SysTools.hs
index ea3c461327..5bd9fd1f47 100644
--- a/compiler/main/SysTools.hs
+++ b/compiler/main/SysTools.hs
@@ -1588,7 +1588,8 @@ linkDynLib dflags0 o_files dep_packages
              osMachOTarget (platformOS (targetPlatform dflags)) ) &&
            dynLibLoader dflags == SystemDependent &&
            WayDyn `elem` ways dflags
-            = ["-L" ++ l, "-Wl,-rpath", "-Wl," ++ l]
+            = ["-L" ++ l, "-Xlinker", "-rpath", "-Xlinker", l]
+              -- See Note [-Xlinker -rpath vs -Wl,-rpath]
          | otherwise = ["-L" ++ l]
 
     let lib_paths = libraryPaths dflags
diff --git a/testsuite/driver/extra_files.py b/testsuite/driver/extra_files.py
index 217e5d437a..3f2cf5cc4b 100644
--- a/testsuite/driver/extra_files.py
+++ b/testsuite/driver/extra_files.py
@@ -428,6 +428,7 @@ extra_src_files = {
   'parser.prog001': ['Read006.hs', 'Read007.hs'],
   'pat-syn-bundle': ['Bundle1.hs', 'BundleInternal1.hs'],
   'pat-syn-trans-bundle': ['Bundle.hs', 'BundleInternal.hs', 'TransBundle.hs'],
+  'path_with_commas': ['test.pkg', 'Main.hs', 'Dummy.hs'],
   'pkg02': ['A.hs', 'Foreign.hs'],
   'plugins01': ['simple-plugin/'],
   'plugins02': ['simple-plugin/'],
diff --git a/testsuite/tests/th/TH_linker/Dummy.hs b/testsuite/tests/th/TH_linker/Dummy.hs
new file mode 100644
index 0000000000..9be471c397
--- /dev/null
+++ b/testsuite/tests/th/TH_linker/Dummy.hs
@@ -0,0 +1 @@
+module Dummy where
diff --git a/testsuite/tests/th/TH_linker/Main.hs b/testsuite/tests/th/TH_linker/Main.hs
new file mode 100644
index 0000000000..0530199575
--- /dev/null
+++ b/testsuite/tests/th/TH_linker/Main.hs
@@ -0,0 +1,7 @@
+{-# LANGUAGE TemplateHaskell #-}
+module Main where
+
+import Language.Haskell.TH
+
+main :: IO ()
+main = putStrLn $(return $ LitE $ StringL "hello")
diff --git a/testsuite/tests/th/TH_linker/Makefile b/testsuite/tests/th/TH_linker/Makefile
new file mode 100644
index 0000000000..84f7760765
--- /dev/null
+++ b/testsuite/tests/th/TH_linker/Makefile
@@ -0,0 +1,40 @@
+TOP=../../..
+include $(TOP)/mk/boilerplate.mk
+include $(TOP)/mk/test.mk
+
+DIR='test,path'
+PKGCONF=$(DIR)/local.package.conf
+LOCAL_GHC_PKG = '$(GHC_PKG)' --no-user-package-db -f $(PKGCONF)
+
+ifeq "$(WINDOWS)" "YES"
+DLL = lib$1.dll
+else ifeq "$(DARWIN)" "YES"
+DLL = lib$1.dylib
+else
+DLL = lib$1.so
+endif
+
+ifeq "$(WINDOWS)" "YES"
+EXE = $1.exe
+else ifeq "$(DARWIN)" "YES"
+EXE = $1
+else
+EXE = $1
+endif
+
+# Check if paths with commas work
+# Previously, the linker would use -Wl,-rpath -Wl,test,path to pass the
+# path and that would treat test and path as 2 different arguments
+path_with_commas :
+	@rm -rf $(DIR)
+	mkdir -p $(DIR)
+	$(LOCAL_GHC_PKG) init $(PKGCONF)
+	# Create shared library 'foo' from Dummy.hs
+	'$(TEST_HC)' $(TEST_HC_OPTS) -shared -dynamic Dummy.hs -odir $(DIR) -hidir $(DIR) -o $(DIR)/$(call DLL,foo) > /dev/null
+	# Make a package that forces trying to load it
+	$(LOCAL_GHC_PKG) register --force test.pkg
+
+	@rm -rf Main.{hi,o} Main
+	# Because we compile with -package testpkg, TH has to load 'foo'
+	'$(TEST_HC)' $(TEST_HC_OPTS) $(ghcThWayFlags) -package-db $(PKGCONF) -package testpkg Main.hs
+	./$(call EXE,Main)
diff --git a/testsuite/tests/th/TH_linker/all.T b/testsuite/tests/th/TH_linker/all.T
new file mode 100644
index 0000000000..8feac991ba
--- /dev/null
+++ b/testsuite/tests/th/TH_linker/all.T
@@ -0,0 +1,4 @@
+test('path_with_commas',
+     ignore_stderr,
+     run_command,
+     ['$MAKE -s --no-print-directory path_with_commas'])
diff --git a/testsuite/tests/th/TH_linker/path_with_commas.stdout b/testsuite/tests/th/TH_linker/path_with_commas.stdout
new file mode 100644
index 0000000000..0621c2410a
--- /dev/null
+++ b/testsuite/tests/th/TH_linker/path_with_commas.stdout
@@ -0,0 +1,4 @@
+Reading package info from "test.pkg" ... done.
+[1 of 1] Compiling Main             ( Main.hs, Main.o )
+Linking Main ...
+hello
diff --git a/testsuite/tests/th/TH_linker/test.pkg b/testsuite/tests/th/TH_linker/test.pkg
new file mode 100644
index 0000000000..2fc82cba43
--- /dev/null
+++ b/testsuite/tests/th/TH_linker/test.pkg
@@ -0,0 +1,7 @@
+name: testpkg
+version: 1.2.3.4
+id: testpkg-1.2.3.4-XXX
+key: testpkg-1.2.3.4-XXX
+exposed: True
+library-dirs: "${pkgroot}"
+extra-libraries: foo