bug#25882: gcc-wrapper doesn't handle response files

bug#25882: gcc-wrapper doesn't handle response files

[Federico Beffa]

gcc-wrapper doesn't handle compiler/linker flags passed through
response files.

One package which recently started using such files is GHC (I believe
since 7.10.3).  For this reason we currently need to patch it.
However, the problem is with our tool chain wrapper and not with GHC
itself.

See discussion at
https://lists.gnu.org/archive/html/guix-devel/2017-01/msg01981.html

bug#25882: gcc-wrapper doesn't handle response files

[Ludovic Courtès]

Federico Beffa <beffa@fbengineering.ch> skribis:

> gcc-wrapper doesn't handle compiler/linker flags passed through
> response files.
>
> One package which recently started using such files is GHC (I believe
> since 7.10.3).  For this reason we currently need to patch it.
> However, the problem is with our tool chain wrapper and not with GHC
> itself.
>
> See discussion at
> https://lists.gnu.org/archive/html/guix-devel/2017-01/msg01981.html

Given that the GHC patch is so small, I have a slight preference for
keeping ld-wrapper unchanged and using the GHC patch.  To put it
differently, the GHC patch is smaller and less error-prone than the
changes that would need to be made in ld-wrapper.

WDYT?

Thanks,
Ludo’.

bug#25882: gcc-wrapper doesn't handle response files

[Federico Beffa]

ludo@gnu.org (Ludovic Courtès) writes:

> Federico Beffa <beffa@fbengineering.ch> skribis:
>
>> gcc-wrapper doesn't handle compiler/linker flags passed through
>> response files.
>>
>> One package which recently started using such files is GHC (I believe
>> since 7.10.3).  For this reason we currently need to patch it.
>> However, the problem is with our tool chain wrapper and not with GHC
>> itself.
>>
>> See discussion at
>> https://lists.gnu.org/archive/html/guix-devel/2017-01/msg01981.html
>
> Given that the GHC patch is so small, I have a slight preference for
> keeping ld-wrapper unchanged and using the GHC patch.  To put it
> differently, the GHC patch is smaller and less error-prone than the
> changes that would need to be made in ld-wrapper.

I don't think that it is a good idea because any upstream change around
that code will break our package again and, going forward, we may find
other pieces of software making use of this gcc feature.  The patch is
small, but the effort to find it wasn't.

I like to fix things where the problem is, not working around it.

Fede

bug#25882: gcc-wrapper doesn't handle response files

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

Federico Beffa <beffa@ieee.org> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Federico Beffa <beffa@fbengineering.ch> skribis:
>>
>>> gcc-wrapper doesn't handle compiler/linker flags passed through
>>> response files.
>>>
>>> One package which recently started using such files is GHC (I believe
>>> since 7.10.3).  For this reason we currently need to patch it.
>>> However, the problem is with our tool chain wrapper and not with GHC
>>> itself.
>>>
>>> See discussion at
>>> https://lists.gnu.org/archive/html/guix-devel/2017-01/msg01981.html
>>
>> Given that the GHC patch is so small, I have a slight preference for
>> keeping ld-wrapper unchanged and using the GHC patch.  To put it
>> differently, the GHC patch is smaller and less error-prone than the
>> changes that would need to be made in ld-wrapper.
>
> I don't think that it is a good idea because any upstream change around
> that code will break our package again and, going forward, we may find
> other pieces of software making use of this gcc feature.  The patch is
> small, but the effort to find it wasn't.
>
> I like to fix things where the problem is, not working around it.

On closer inspection, it’s an easy change to make.

Could you test the attached patch with GHC?

The way I would test it without rebuilding the world is by copying the
new ld-wrapper.in to ld-wrapper2.in (and thus keeping ld-wrapper.in
unchanged), and then adding it as an input to GHC (via
‘make-ld-wrapper’).  Commit 77db91addc57faa000db05563820f57a9ffdedfc
might serve as an example.

Thanks,
Ludo’.


[-- Attachment #2: Type: text/x-patch, Size: 2107 bytes --]

diff --git a/gnu/packages/ld-wrapper.in b/gnu/packages/ld-wrapper.in
index ebfd8332c..ff086154a 100644
--- a/gnu/packages/ld-wrapper.in
+++ b/gnu/packages/ld-wrapper.in
@@ -15,7 +15,7 @@ main="(@ (gnu build-support ld-wrapper) ld-wrapper)"
 exec @GUILE@ -c "(load-compiled \"@SELF@.go\") (apply $main (cdr (command-line)))" "$@"
 !#
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2012, 2013, 2014, 2015, 2016 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017 Ludovic Courtès <ludo@gnu.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -35,6 +35,7 @@ exec @GUILE@ -c "(load-compiled \"@SELF@.go\") (apply $main (cdr (command-line))
 (define-module (gnu build-support ld-wrapper)
   #:use-module (srfi srfi-1)
   #:use-module (ice-9 match)
+  #:autoload   (ice-9 rdelim) (read-string)
   #:export (ld-wrapper))
 
 ;;; Commentary:
@@ -222,9 +223,28 @@ impure library ~s~%"
               '()
               library-files))
 
+(define (expand-arguments args)
+  ;; Expand ARGS such that "response file" arguments, such as "@args.txt", are
+  ;; expanded.  See 'expandargv' in libiberty.
+  (define (response-file-arguments file)
+    (when %debug?
+      (format (current-error-port)
+              "ld-wrapper: reading arguments from '~a'~%" file))
+    (string-tokenize (call-with-input-file file read-string)))
+
+  (fold-right (lambda (arg result)
+                (if (string-prefix? "@" arg)
+                    (let ((file (string-drop arg 1)))
+                      (append (response-file-arguments file)
+                              result))
+                    (cons arg result)))
+              '()
+              args))
+
 (define (ld-wrapper . args)
   ;; Invoke the real `ld' with ARGS, augmented with `-rpath' switches.
-  (let* ((path (library-search-path args))
+  (let* ((args (expand-arguments args))
+         (path (library-search-path args))
          (libs (library-files-linked args path))
          (args (append args (rpath-arguments libs))))
     (when %debug?

bug#25882: gcc-wrapper doesn't handle response files

[Federico Beffa]

ludo@gnu.org (Ludovic Courtès) writes:

> On closer inspection, it’s an easy change to make.
>
> Could you test the attached patch with GHC?

Thanks for the patch.  I've tested it with GHC 8.0.2 and seems to work
for it.  However, this is an excerpt of the description of the use of
response files from the GCC manual:

--8<---------------cut here---------------start------------->8---
'@FILE'
     Read command-line options from FILE.  The options read are inserted
     in place of the original @FILE option.  If FILE does not exist, or
     cannot be read, then the option will be treated literally, and not
     removed.

     Options in FILE are separated by whitespace.  A whitespace
     character may be included in an option by surrounding the entire
     option in either single or double quotes.  Any character (including
     a backslash) may be included by prefixing the character to be
     included with a backslash.  The FILE may itself contain additional
     @FILE options; any such options will be processed recursively.
--8<---------------cut here---------------end--------------->8---

The patch doesn't seems to handle several things, such as missing files,
recursion and use of quotes.  I would suggest to try to match the
expected behavior in its entirety (or at least a larger part thereof).

Fede

bug#25882: gcc-wrapper doesn't handle response files

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

Federico Beffa <beffa@ieee.org> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> On closer inspection, it’s an easy change to make.
>>
>> Could you test the attached patch with GHC?
>
> Thanks for the patch.  I've tested it with GHC 8.0.2 and seems to work
> for it.  However, this is an excerpt of the description of the use of
> response files from the GCC manual:
>
> '@FILE'
>      Read command-line options from FILE.  The options read are inserted
>      in place of the original @FILE option.  If FILE does not exist, or
>      cannot be read, then the option will be treated literally, and not
>      removed.
>
>      Options in FILE are separated by whitespace.  A whitespace
>      character may be included in an option by surrounding the entire
>      option in either single or double quotes.  Any character (including
>      a backslash) may be included by prefixing the character to be
>      included with a backslash.  The FILE may itself contain additional
>      @FILE options; any such options will be processed recursively.

Oh, nice, I hadn’t seen this doc.

The attached version adds handling of unreadable files and recursion.
It does not address parsing of quote-delimited options though, but I’m
tempted to punt on that one.

Thoughts?

Thanks,
Ludo’.


[-- Attachment #2: Type: text/x-patch, Size: 2851 bytes --]

diff --git a/gnu/packages/ld-wrapper.in b/gnu/packages/ld-wrapper.in
index ebfd8332c..82bd2196c 100644
--- a/gnu/packages/ld-wrapper.in
+++ b/gnu/packages/ld-wrapper.in
@@ -15,7 +15,7 @@ main="(@ (gnu build-support ld-wrapper) ld-wrapper)"
 exec @GUILE@ -c "(load-compiled \"@SELF@.go\") (apply $main (cdr (command-line)))" "$@"
 !#
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2012, 2013, 2014, 2015, 2016 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017 Ludovic Courtès <ludo@gnu.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -35,6 +35,7 @@ exec @GUILE@ -c "(load-compiled \"@SELF@.go\") (apply $main (cdr (command-line))
 (define-module (gnu build-support ld-wrapper)
   #:use-module (srfi srfi-1)
   #:use-module (ice-9 match)
+  #:autoload   (ice-9 rdelim) (read-string)
   #:export (ld-wrapper))
 
 ;;; Commentary:
@@ -222,9 +223,44 @@ impure library ~s~%"
               '()
               library-files))
 
+(define (expand-arguments args)
+  ;; Expand ARGS such that "response file" arguments, such as "@args.txt", are
+  ;; expanded (info "(gcc) Overall Options").
+  (define (response-file-arguments file)
+    (when %debug?
+      (format (current-error-port)
+              "ld-wrapper: attempting to read arguments from '~a'~%" file))
+
+    ;; FIXME: Options can contain whitespace if they are protected by single
+    ;; or double quotes; this is not implemented here.
+    (string-tokenize (call-with-input-file file read-string)))
+
+  (define result
+    (fold-right (lambda (arg result)
+                  (if (string-prefix? "@" arg)
+                      (let ((file (string-drop arg 1)))
+                        (append (catch 'system-error
+                                  (lambda ()
+                                    (response-file-arguments file))
+                                  (lambda args
+                                    ;; FILE doesn't exist or cannot be read so
+                                    ;; leave ARG as is.
+                                    (list arg)))
+                                result))
+                      (cons arg result)))
+                '()
+                args))
+
+  ;; If there are "@" arguments in RESULT *and* we can expand them (they don't
+  ;; refer to nonexistent files), then recurse.
+  (if (equal? result args)
+      result
+      (expand-arguments result)))
+
 (define (ld-wrapper . args)
   ;; Invoke the real `ld' with ARGS, augmented with `-rpath' switches.
-  (let* ((path (library-search-path args))
+  (let* ((args (expand-arguments args))
+         (path (library-search-path args))
          (libs (library-files-linked args path))
          (args (append args (rpath-arguments libs))))
     (when %debug?

bug#25882: gcc-wrapper doesn't handle response files

[Federico Beffa]

ludo@gnu.org (Ludovic Courtès) writes:

> The attached version adds handling of unreadable files and recursion.
> It does not address parsing of quote-delimited options though, but I’m
> tempted to punt on that one.

My worry is that not handling them may lead to hard to debug, guix
specific errors.

Fede

bug#25882: gcc-wrapper doesn't handle response files

[Ludovic Courtès]

Hi Federico,

Federico Beffa <beffa@ieee.org> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> The attached version adds handling of unreadable files and recursion.
>> It does not address parsing of quote-delimited options though, but I’m
>> tempted to punt on that one.
>
> My worry is that not handling them may lead to hard to debug, guix
> specific errors.

Since we’re about to freeze ‘core-updates’ and I’d like to make progress
on this front, I pushed the ld-wrapper change as commit
696487d665a616dfdd09272a7bff0bea0e19375d.

It’s not perfect as discussed earlier, but it’s known to be “good
enough” with GHC and Chromium, which are the only users of this I know
of.

Thanks,
Ludo’.