[PATCH] D40129: [ELF] Fall back to search dirs for linker scripts specified with -T

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Thanks for this patch, few comments/suggestions below.



================
Comment at: ELF/DriverUtils.cpp:220
+// https://sourceware.org/binutils/docs/ld/Options.html#Options
+// If scriptfile does not exist in the current directory, ld looks for it in
+// the directories specified by any preceding ‘-L’ options. Multiple ‘-T’
----------------
I think should be `script file` as it is not a single word.
btw, can we just copy paste the GNU spec ?

I think we usually just describe LLD behavior and mention it agrees/disagrees with gnu linkers.
I do not think putting URLs here is very useful in that particular case as behavior is simple.
So I would rewrite this so that will something that just describes the behavior.


================
Comment at: test/ELF/linkerscript/linker-script-in-search-path.s:1
+# Check that we fall back to search paths if a linker script was not found
+# This behaviour matches ld.bfd and various projects appear to rely on this
----------------
Could you put this below REQUIRES ? I think that is more consistent with other tests,
where REQURES is usually a first line. Probably that is not always true, but anyways..


================
Comment at: test/ELF/linkerscript/linker-script-in-search-path.s:13
+# If the linker script specified with -T is missing we should emit an error
+# RUN: not ld.lld -Tfoo.script %t.o 2>&1 | FileCheck %s -check-prefix ERROR
+
----------------
Please put ERROR check line right after this one, that way it allows keep check calls and check lines
be close to each other and look more isolated and convinent to read:

```
# If the linker script specified with -T is missing we should emit an error
# RUN: not ld.lld -Tfoo.script %t.o 2>&1 | FileCheck %s -check-prefix ERROR
# ERROR: error: cannot find linker script foo.script
```

The same for `CHECK`


================
Comment at: test/ELF/linkerscript/linker-script-in-search-path.s:23
+
+.globl _start
+_start:
----------------
Does not seem you need any code here.


================
Comment at: test/ELF/linkerscript/linker-script-in-search-path.s:26
+  ret
\ No newline at end of file

----------------
You need new line here, that is consistent with other testcases.


https://reviews.llvm.org/D40129