ld-wrapper does not handle response files

  • Done
  • quality assurance status badge
Details
3 participants
  • Federico Beffa
  • Federico Beffa
  • Ludovic Courtès
Owner
unassigned
Submitted by
Federico Beffa
Severity
normal
F
F
Federico Beffa wrote on 26 Feb 2017 17:58
gcc-wrapper doesn't handle response files
(address . bug-guix@gnu.org)
E1ci294-0001p4-En@lupo
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
L
L
Ludovic Courtès wrote on 6 Mar 2017 17:22
(name . Federico Beffa)(address . beffa@fbengineering.ch)(address . 25882@debbugs.gnu.org)
87o9xepe7l.fsf@gnu.org
Federico Beffa <beffa@fbengineering.ch> skribis:

Toggle quote (11 lines)
> 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’.
F
F
Federico Beffa wrote on 6 Mar 2017 18:16
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 25882@debbugs.gnu.org)
871suajpfh.fsf@lupo.i-did-not-set--mail-host-address--so-tickle-me
ludo@gnu.org (Ludovic Courtès) writes:

Toggle quote (18 lines)
> 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
L
L
Ludovic Courtès wrote on 7 Mar 2017 11:53
(name . Federico Beffa)(address . beffa@ieee.org)(address . 25882@debbugs.gnu.org)
87r329fjcq.fsf@gnu.org
Federico Beffa <beffa@ieee.org> skribis:

Toggle quote (27 lines)
> 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’.
Toggle diff (51 lines)
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?
F
F
Federico Beffa wrote on 8 Mar 2017 13:57
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 25882@debbugs.gnu.org)
878togudsp.fsf@lupo.i-did-not-set--mail-host-address--so-tickle-me
ludo@gnu.org (Ludovic Courtès) writes:

Toggle quote (4 lines)
> 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:

Toggle snippet (14 lines)
'@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.

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
L
L
Ludovic Courtès wrote on 11 Mar 2017 14:47
(name . Federico Beffa)(address . beffa@ieee.org)(address . 25882@debbugs.gnu.org)
87shmk6i2t.fsf@gnu.org
Federico Beffa <beffa@ieee.org> skribis:

Toggle quote (23 lines)
> 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’.
Toggle diff (67 lines)
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?
L
L
Ludovic Courtès wrote on 11 Mar 2017 14:49
control message for bug #25882
(address . control@debbugs.gnu.org)
87r3237wkz.fsf@gnu.org
retitle 25882 ld-wrapper does not handle response files
L
L
Ludovic Courtès wrote on 11 Mar 2017 14:49
(address . control@debbugs.gnu.org)
87pohn7wkl.fsf@gnu.org
tags 25882 patch
F
F
Federico Beffa wrote on 12 Mar 2017 12:36
Re: bug#25882: gcc-wrapper doesn't handle response files
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 25882@debbugs.gnu.org)
871su2sp4l.fsf@lupo.i-did-not-set--mail-host-address--so-tickle-me
ludo@gnu.org (Ludovic Courtès) writes:

Toggle quote (4 lines)
> 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
L
L
Ludovic Courtès wrote on 25 May 2017 14:42
(name . Federico Beffa)(address . beffa@ieee.org)(address . 25882-done@debbugs.gnu.org)
87poexxg3r.fsf@gnu.org
Hi Federico,

Federico Beffa <beffa@ieee.org> skribis:

Toggle quote (9 lines)
> 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’.
Closed
?